New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
inabox: add window.AMP.inaboxUnregisterIframe(iframe) #13795
inabox: add window.AMP.inaboxUnregisterIframe(iframe) #13795
Conversation
measurableFrame: !HTMLIFrameElement, | ||
observeUnregisterFn: (!UnlistenDef|undefined), | ||
}} */ | ||
let AdFrameDef; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can this be a const?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
typedefs are declared as let (they don't actually hold state)
@@ -403,4 +404,51 @@ describes.realWin('inabox-host:messaging', {}, env => { | |||
expect(host.getFrameElement_(sourceMock, sentinel)).to.be.null; | |||
}); | |||
}); | |||
|
|||
describe('unregisterIframe', () => { | |||
it('unregisters frames', () => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: this test is kinda hard to look at. maybe add some line breaks or comments to separate setup / test / expectation verification?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
* inabox: add window.AMP.inaboxUnregisterIframe(iframe) * fix test error * fix test and add more coverage * PR feedback
In ampproject#13795 we tried to fix a memory leak but missed that PositionObserver.observe(...) wasn't passing back the unregister function from Observable.add(), and so we weren't removing the listener. After this change we still leak on refresh: with examples/inabox.host.memory.html I measure us as leaking 200k in 60 refreshes without this PR, vs 30k with it. I'm still trying to figure out where else we leak, but getting the per-refresh leak down from 3k to 0.5k is still worth shipping.
In #13795 we tried to fix a memory leak but missed that PositionObserver.observe(...) wasn't passing back the unregister function from Observable.add(), and so we weren't removing the listener. After this change we still leak on refresh: with examples/inabox.host.memory.html I measure us as leaking 200k in 60 refreshes without this PR, vs 30k with it. I'm still trying to figure out where else we leak, but getting the per-refresh leak down from 3k to 0.5k is still worth shipping.
Clone of #13776 as Jeff OOTO. Modified to centralize state in single map to ease cleanup and future additions.
The current API for inabox is that when you want to remove a frame you delete it
from ampInaboxIframes. This means there isn't a way for inabox to run any
additional cleanup on frame removal, and currently there's a memory leak where
references to iframes (or their descendents) are maintained indefinitely in
InaboxMessagingHost.iframeMap_ and in the position observation listeners.
This changes the API from 'delete from a list of frames' to 'call a function',
which allows the function to do arbitrary cleanup like removing references.