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

🌸 Cherry-pick request into #25222 (Approved) #25258

Closed
jridgewell opened this issue Oct 25, 2019 · 9 comments
Closed

🌸 Cherry-pick request into #25222 (Approved) #25258

jridgewell opened this issue Oct 25, 2019 · 9 comments
Assignees
Labels
Cherry-pick: Beta Cherry-pick: Experimental Type: Release Used to track AMP releases from canary to production

Comments

@jridgewell
Copy link
Contributor

jridgewell commented Oct 25, 2019

Cherry-pick request

Issue PR Production? RC? Release issue
#25245 #25245 No Yes #25222

Why does this issue meet the cherry-pick criteria?

All AMP pages are broken in IE 11 due to a bug in the Custom Elements polyfill.

Mini-postmortem

Summary

The new Custom Elements v1 polyfill contained a bug that prevented creating custom element instances in IE 11.

Users affected Impact
All users of IE11 Complete break of page, no images, videos, ads, etc. Text would work

Root Causes

Custom Elements must extend from the HTMLElement class (required by the CE spec). The new polyfill injected a brand new function HTMLElementPolyfill as a subclass of HTMLElement, and overwrite window.HTMLElement = HTMLElementPolyfill. With proper subclasses, the prototype of both the Class' constructor function and the associated prototype must point to the superclass. Meaning, HTMLElementPolyfill.__proto__ === HTMLElement and HTMLElementPolyfill.prototype.__proto__ === HTMLElement.prototype. This is how subclassing works in ES6 browsers.

When you transpile a subclass, generally, you must have the equivalent of a super() call in the subclass constructor. This is usually compiled to superClass.call(this) as the ES5 way of doing subclass construction.

In IE11, HTMLElement is an exotic host object, and does not extend from the Function class. Because it doesn't extend from Function, HTMLElement.call does not exist (it's defined on Function class). So, when we transpiled super() to HTMLElement.call(this), we got an error.

Action Items

Action Item Type Owner PR #
Add crossorigin=anonymous Mitigate @jridgewell #24731
Add regression test Prevent @jridgewell #25294

Lessons Learned

  • IE11 has a lot of nonstandard behaviors.
  • Cross origin errors are not reported without crossorigin=anonymous on the scripts

Things that went well

  • Ads metrics drop alerted us to the issue within 24hours

Things that went wrong

  • The error reports were silently dropped by the browser because they originated from a crossorigin script.

/cc @ampproject/wg-approvers @ampproject/cherry-pick-approvers

@jridgewell jridgewell added the Type: Release Used to track AMP releases from canary to production label Oct 25, 2019
@zhouyx
Copy link
Contributor

zhouyx commented Oct 25, 2019

I am the release engineer this week. Let me know when this is ready.

@jridgewell
Copy link
Contributor Author

PR is already merged, we just need cherry-pick approval.

@dvoytenko
Copy link
Contributor

Approved

@zhouyx zhouyx changed the title 🌸 Cherry-pick request into #25222 (Pending) 🌸 Cherry-pick request into #25222 (Approved) Oct 25, 2019
@cramforce
Copy link
Member

Approved

@zhouyx
Copy link
Contributor

zhouyx commented Oct 26, 2019

Cherry-pick to canary and RC completed. Assign back to @jridgewell for postmortem

@jridgewell
Copy link
Contributor Author

Postmortem published, closing.

@dreamofabear
Copy link

dreamofabear commented Oct 28, 2019

@jridgewell Is there a regression test we can add or opt-in for IE for CE registration?

@jridgewell
Copy link
Contributor Author

Integration tests would be all that's necessary, if we enabled them.

@dreamofabear
Copy link

Yea, enabling something trivial on IE (e.g. single amp-img integration test) will do.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Cherry-pick: Beta Cherry-pick: Experimental Type: Release Used to track AMP releases from canary to production
Projects
None yet
Development

No branches or pull requests

6 participants