Fix leaking windows message event listener #422
Conversation
src/utils.ts
Outdated
| @@ -42,7 +42,7 @@ export const runIframe = ( | |||
| timeoutInSeconds: number = DEFAULT_AUTHORIZE_TIMEOUT_IN_SECONDS | |||
| ) => { | |||
| return new Promise<AuthenticationResult>((res, rej) => { | |||
| var iframe = window.document.createElement('iframe'); | |||
| let iframe = window.document.createElement('iframe'); | |||
There was a problem hiding this comment.
Seems like this lib's style would prefer const here
src/utils.ts
Outdated
| const timeoutSetTimeoutId = setTimeout(() => { | ||
| rej(TIMEOUT_ERROR); | ||
| removeIframe(); | ||
| window.removeEventListener('message', iframeEventHandler, false); |
There was a problem hiding this comment.
seems like this could just be in removeIframe both here and in the handler and it'd be slightly simpler.
|
@yinzara Please let me know if you'd like to continue with this PR. There are a few unit tests that look to be broken - could you evaluate and fix? Thanks. |
stevehobbsdev
force-pushed
the
fix/popup-message-listener-cleanup
branch
2 times, most recently
from
68cebe3
to
0fc35ec
Compare
stevehobbsdev
force-pushed
the
fix/popup-message-listener-cleanup
branch
from
68e3fb2
to
200023a
Compare
stevehobbsdev
force-pushed
the
fix/popup-message-listener-cleanup
branch
from
200023a
to
d630dde
Compare
|
@adamjmcgrath I have fixed up this PR, could you please take a pass over? |
| clearTimeout(timeoutSetTimeoutId); | ||
| window.removeEventListener('message', iframeEventHandler, false); |
There was a problem hiding this comment.
I think the danger of removing this one would be that, if you did 2 successful getTokenSilently({ ignoreCache: true }) within CLEANUP_IFRAME_TIMEOUT_IN_SECONDS seconds of each other, the first iframeEventHandler would still be around and handle the second getTokenSilently's postMessage
There was a problem hiding this comment.
Yes, good catch - I'll reinstate it.
| clearTimeout(timeoutId); | ||
| window.removeEventListener('message', popupEventListener, false); |
There was a problem hiding this comment.
This one is fine because you're removing the event listener in the successful and unsuccessful (timed out) cases
In both "loginWithPopup" and "loginWithRedirect", window.addEventListener('message',... is called.
There are issues with the cleanup of this to properly remove it from the window. In the case of timeouts, it is not being removed properly. For loginWithPopup, it wasn't being removed at all causing leaking resources the more times its called.