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
Fix NPE caused by race condition when AMP page state changes before amp-analytics finishes initialization #18400
Conversation
…mp-analytics finishes initialization
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.
Should we have a test for this?
@@ -178,8 +182,13 @@ export class AmpAnalytics extends AMP.BaseElement { | |||
return false; | |||
} | |||
|
|||
// Page was unloaded - free up owned resources. | |||
this.transport_.deleteIframeTransport(); | |||
if (this.iniPromise_) { |
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.
Hmmmm, I feel we should stop initiating iframeTransport once unlaid out, instead of wait for the promise to resolve and delete the iframe again.
I am ok to have this as a temporary solution but longterm this should be considered together with #18231
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.
This can be tricky to implement, as promise cannot be canceled.
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.
even if we could cancel a promise, to support the following lifecycle:
active -> inactive : unlayoutCallback
inactive -> active : resumeCallback
we have to initialize everything in resumeCallback as well.
Test added |
…mp-analytics finishes initialization (#18400) * Fix NPE caused by race condition when AMP page state changes before amp-analytics finishes initialization * add tests
…mp-analytics finishes initialization (ampproject#18400) * Fix NPE caused by race condition when AMP page state changes before amp-analytics finishes initialization * add tests
…mp-analytics finishes initialization (ampproject#18400) * Fix NPE caused by race condition when AMP page state changes before amp-analytics finishes initialization * add tests
Fixes #18395