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

Extension reload breaks waitForExtension #25436

Closed
jpettitt opened this issue Nov 5, 2019 · 6 comments · Fixed by #25441
Closed

Extension reload breaks waitForExtension #25436

jpettitt opened this issue Nov 5, 2019 · 6 comments · Fixed by #25441

Comments

@jpettitt
Copy link
Contributor

jpettitt commented Nov 5, 2019

When an extension depends on another extension for a service it ends up in waitForExtension if the provider hasn't yet loaded. We've been seeing timeouts from waitForExtension that shouldn't be happening.

This problem is that if v0.js detects a version mismatch (v0.js version differs from an extension version) it automatically loaded the correct version of the extension to match v0 9getExtensionScript_). When it does this it creates a new extension holder which leaves waitForExtension waiting on a load that will never happen.

This only happens on publisher sites due to the AMP cache automatically syncing versions and it only happens if the extension in cache mismatches v0.js.

See deletion of the extension holder https://github.com/ampproject/amphtml/blame/40fa29d8daacd92fbbcfda825f4b7f89dab25a34/src/service/extensions-impl.js#L255

@jpettitt
Copy link
Contributor Author

jpettitt commented Nov 5, 2019

cc @choumx @jridgewell @i-like-robots also tracked in Google b/143942489

force fail example https://github.com/i-like-robots/broken-amp-versions-test-case

@dvoytenko
Copy link
Contributor

Got repro. I'm fixing this right now.

@dvoytenko
Copy link
Contributor

I'm moving this to P0.

@dvoytenko
Copy link
Contributor

/cc @erwinmombay

@dvoytenko
Copy link
Contributor

The bug is in this line. The regex is too allowing and matches both amp-subscriptions-0.1 and amp-subscriptions-google-0.1. It was introduced by proxy in the #24730.

/cc @choumx

@dvoytenko
Copy link
Contributor

The race condition indicated by @jpettitt is also an issue, but less likely to occur. Will be fixed in the same pull request.

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

Successfully merging a pull request may close this issue.

2 participants