Skip to content
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

Revert "analytics: bail out for detached targets" #32892

Merged
merged 1 commit into from Feb 25, 2021

Conversation

samouri
Copy link
Member

@samouri samouri commented Feb 25, 2021

Reverts #32637

Potential "fix" to ampproject/error-reporting#54.
Setting this up in case we want to eventually merge, we'll have the CI predone.

@amp-owners-bot amp-owners-bot bot requested a review from zhouyx February 25, 2021 00:33
@samouri samouri requested review from calebcordry and erwinmombay and removed request for zhouyx February 25, 2021 00:55
@samouri
Copy link
Member Author

samouri commented Feb 25, 2021

Did more digging. I was very wrong. Most of the flows do work when the node is detached. Both element.ownerDocument and element.ownerDocument.defaultView both work on detached nodes.

The bug I was trying to address was this: Screen Shot 2021-02-10 at 11 04 33 AM.

I traced through the mangled names in amp-form and amp-analytics, the failure flow is this:

  1. triggerAnalyticsEvent(...)
  2. this.findRoot(target)
  3. getParentWindowFrameElement(target)
  4. getTopWindow((target.ownerDocument||target).defaultView)) --> win.__AMP_TOP

Other notes:

  • The error we are seeing is that we are trying to read a property on undefined. Working backwards this means that the .defaultView is evaluating to undefined.
  • When called on a Document, defaultView can never be undefined. It is always either Window|null. This means that target.ownerDocument||target evaluates to a non Document.
  • element.ownerDocument can only be null if the element is itself a document, which is exactly what this is expression is built for. This means that target must not have been a Node.

Since the original case was calling analytics on a HTMLIframeElement thats also impossible to ever happen. Except during the one commit where I was testing out targeting an AmpDoc which I reverted before merging 🤕. I didn't realize the console error was newly introduced, and after a closer look at the original error the followup fix should have been somewhere in ampdoc-impl.getAmpdocIfAvailable

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants