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 for #25007 into #25319 (Approved) #25367

Closed
dvoytenko opened this issue Nov 1, 2019 · 3 comments
Closed

🌸 Cherry-pick request for #25007 into #25319 (Approved) #25367

dvoytenko opened this issue Nov 1, 2019 · 3 comments
Assignees
Labels
Cherry-pick: Beta Cherry-pick: Experimental Type: Release Used to track AMP releases from canary to production

Comments

@dvoytenko
Copy link
Contributor

dvoytenko commented Nov 1, 2019

Cherry-pick request

Issue PR Production? RC? Release issue
#25007 #25321 NO YES #25319
#25007 #25357 NO YES #25319

Why does this issue meet the cherry-pick criteria?

The fixes unbreak a4a ads in shadow mode, which has significant impact.

Mini-postmortem

The reason for this bug is an incorrect use of the top level window.document API which has different meaning in shadow docs, FIE, scoped services, etc.

Summary

We received the report in #25007 that A4A ads would not render at all into PWA shell. This is a regression, but is unclear when exactly it started. The issue got attention from representatives from multiple companies, indicating it was a problem breaking all users of PWA model.

Users affected Impact
All users of A4A ads served into PWA Ads would not render at all.

Root Causes

  1. Incorrect use of window.document API instead of ampdoc.

Action Items

Action Item Type Owner PR #
Prohibit window.document API Investigate @dvoytenko #25418
Investigate executing realWin unit tests in shadow mode Investigate @dvoytenko #25419

Lessons Learned

Things that went well

  • The issue was debugged and resolved

Things that went wrong

  • A fairly deep-in-the-core bug. We need better tools to prevent such bugs from happening.

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

@dvoytenko dvoytenko added the Type: Release Used to track AMP releases from canary to production label Nov 1, 2019
@dvoytenko dvoytenko changed the title 🌸 Cherry-pick request for #25007 into #1910291709300 (Pending) 🌸 Cherry-pick request for #25007 into #1910302207210 (Pending) Nov 1, 2019
@dvoytenko dvoytenko changed the title 🌸 Cherry-pick request for #25007 into #1910302207210 (Pending) 🌸 Cherry-pick request for #25007 into #25319 (Pending) Nov 1, 2019
@cramforce
Copy link
Member

Approved

@jridgewell jridgewell changed the title 🌸 Cherry-pick request for #25007 into #25319 (Pending) 🌸 Cherry-pick request for #25007 into #25319 (Approved) Nov 4, 2019
@jridgewell jridgewell assigned dvoytenko and unassigned jridgewell Nov 4, 2019
@jridgewell
Copy link
Contributor

/to @dvoytenko for postmortem.

@dvoytenko
Copy link
Contributor Author

postmortem posted. /ht @calebcordry

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

4 participants