-
Notifications
You must be signed in to change notification settings - Fork 3.9k
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
Friendly Frames Embed Error #6543
Comments
I'll take a look, but first impression is that it's not related to #6241. |
@jridgewell Is there any actual error here? What's the browser? |
No error message, all coming from iOS Safari 10.1.1. |
The no error message will likely be fixed by #6428 once we push out canary. |
Might be. Is this a new error? How many of these errors are there? From what I can tell, this happens when |
/to @avimehta Is it possible we keep a reference to a friendly iframe elements somewhere in analytics after an embed is removed? |
yes, we do that afaik. Aren't ads iframes removed if they go out of the view and beyond some threshold? visibility code keeps references around afaik. |
They're very infrequent, and I'm unsure if they're new. |
I doubt a4a iframes get removed. But let's confirm if this is the case first. If indeed the case - it's a minor issue since no prod features are affected. From what I see, the error starts in the
And then eventually fails in
Whether it's a trigger on "visible" or "hidden" - I can't tell yet. |
So, if it's a |
Ok. I'm almost certain that this error is benign. @avimehta what's your opinion? |
I actually found a way to repro this 100% of the time in the a4a.amp.html sample. Simply calling |
I have partial fix. A fuller fix would likely take some deeper refactoring to disconnect all events from the destroyed analytics tag. |
Relevant line is
amphtml/src/service.js
Line 338 in 0310284
Is this fixed by #6241?
The text was updated successfully, but these errors were encountered: