Fix nasty race condition in AMP video #27740
Conversation
Under this scenario a video might not fall back to the origin on pre-render. Possible fix for ampproject#27109 Reproduces by navigating (locally to) http://localhost:8000/proxy/s/stories.nonblocking.io/waffles/?usqp=mq331AQFKAGwASA%3D&_js_v=0.1#referrer=https%3A%2F%2Fwww.google.com&visibilityState=prerender&cap=swipe&share=https%3A%2F%2Funumbox.com%2Fwebsite-development-trends-2020-amp-story%2F waiting for the pre-render load to fail, and then executing `AMP.viewer.receiveMessage('visibilitychange', {'state': 'visible'})` in the console. If this if not a fix for ampproject#27109, then it might be the same race, but elsewhere. The problem is that you cannot wait for `loadPromise` on a video element that has failed a source and has just had a new source provided.
There was a problem hiding this comment.
Wow, great catch! (#27109 is yet another different story specific bug 🤦)
If I understand correctly:
- If the load from the cache succeeds,
layoutCallbackresolves right away - If the load from the cache fails, it'll wait for the ampdoc to get visible, and
layoutCallbackwill resolve/reject based on these new sources
Did you consider the alternative of never resolving the layoutCallback during prerender (even if the cached source loads), but always wait for the non cached sources to be applied before getting the loadPromise?
I don't know if it's better, but there's value in keeping the layoutCallback behavior consistent across cached and non cached prerendering, and that'd allow us to get rid of the timer.
| }, | ||
| (reason) => { | ||
| if (pendingOriginPromise) { | ||
| this.video_[MEDIA_LOAD_FAILURE_SRC_PROPERTY] = undefined; |
There was a problem hiding this comment.
You need to keep this property set, in case the origin source also 404'd.
| .whenFirstVisible() | ||
| .then(() => { | ||
| this.propagateLayoutChildren_(); | ||
| // We need to yield to the event queue before listing for loadPromise | ||
| // because this element may still be in error state from the pre-render | ||
| // load. |
There was a problem hiding this comment.
My kingdom for a way to know for sure when the media element ran its media selection process...
- Change preload to also preload non-cache sources - Clarify preconnect docs about the security invariants.
|
While some simplification might be possible the general order of So, while I was fixing the tests, I noticed we has special logic to only preconnect to the cache. However, that is not needed. We never preconnect to anything in prerender phase. I changed it such that we preconnect to all sources which should make fallbacks faster. While I was at this, I also clarified our docs about these security invariants. |
| }, | ||
| (reason) => { | ||
| if (pendingOriginPromise) { | ||
| return pendingOriginPromise; |
There was a problem hiding this comment.
If this one succeeds, we still want to dispatch this.element.dispatchCustomEvent(VideoEvents.LOAD);
There was a problem hiding this comment.
Good catch. Moved the success handler one further down the chain.
Under this scenario a video might not fall back to the origin on pre-render.
Possible fix for #27109
Reproduces by navigating (locally) to page with a video on page one in prerender mode,
waiting for the prerender load to fail, and then executing
AMP.viewer.receiveMessage('visibilitychange', {'state': 'visible'})in the console.If this if not a fix for #27109, then it might be the same race, but elsewhere. The problem is that you cannot wait for
loadPromiseon a video element that has failed a source and has just had a new source provided, becausecurrentSrchasn't been updated yet.There might be a more elegant fix. Lots of fixes that I thought should have worked, didn't :)
Other changes