From ef4fd1227a6cb822b80f1b2314ee4d538d053fe6 Mon Sep 17 00:00:00 2001 From: Malte Ubl Date: Wed, 15 Apr 2020 15:11:05 -0700 Subject: [PATCH] Fix nasty race condition in AMP video (#27740) Under this scenario a video might not fall back to the origin on pre-render. Possible fix for #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 #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. ## Additional changes: - Change preload to also preload non-cache sources - Clarify preconnect docs about the security invariants. --- extensions/amp-video/0.1/amp-video.js | 58 +++++++++++------- .../amp-video/0.1/test/test-amp-video.js | 61 ++++++++++++++----- src/preconnect.js | 13 ++++ 3 files changed, 97 insertions(+), 35 deletions(-) diff --git a/extensions/amp-video/0.1/amp-video.js b/extensions/amp-video/0.1/amp-video.js index 305c4bc92e5e..6644443d1ab6 100644 --- a/extensions/amp-video/0.1/amp-video.js +++ b/extensions/amp-video/0.1/amp-video.js @@ -21,7 +21,6 @@ import {VisibilityState} from '../../../src/visibility-state'; import { childElementByTag, childElementsByTag, - elementByTag, fullscreenEnter, fullscreenExit, insertAfterOrAtStart, @@ -104,15 +103,13 @@ class AmpVideo extends AMP.BaseElement { * @override */ preconnectCallback(opt_onLayout) { - const videoSrc = this.getVideoSourceForPreconnect_(); - if (videoSrc) { - this.getUrlService_().assertHttpsUrl(videoSrc, this.element); + this.getVideoSourcesForPreconnect_().forEach((videoSrc) => { Services.preconnectFor(this.win).url( this.getAmpDoc(), videoSrc, opt_onLayout ); - } + }); } /** @@ -168,21 +165,26 @@ class AmpVideo extends AMP.BaseElement { /** * @private - * @return {?string} + * @return {!Array} */ - getVideoSourceForPreconnect_() { - if (this.getAmpDoc().getVisibilityState() === VisibilityState.PRERENDER) { - const source = this.getFirstCachedSource_(); - return (source && source.getAttribute('src')) || null; + getVideoSourcesForPreconnect_() { + const videoSrc = this.element.getAttribute('src'); + if (videoSrc) { + return [videoSrc]; } - let videoSrc = this.element.getAttribute('src'); - if (!videoSrc) { - const source = elementByTag(this.element, 'source'); - if (source) { - videoSrc = source.getAttribute('src'); + const srcs = []; + childElementsByTag(this.element, 'source').forEach((source) => { + const src = source.getAttribute('src'); + if (src) { + srcs.push(src); } - } - return videoSrc; + // We also want to preconnect to the origin src to make fallback faster. + const origSrc = source.getAttribute('amp-orig-src'); + if (origSrc) { + srcs.push(origSrc); + } + }); + return srcs; } /** @override */ @@ -322,23 +324,37 @@ class AmpVideo extends AMP.BaseElement { // If we are in prerender mode, only propagate cached sources and then // when document becomes visible propagate origin sources and other children // If not in prerender mode, propagate everything. + let pendingOriginPromise; if (this.getAmpDoc().getVisibilityState() == VisibilityState.PRERENDER) { if (!this.element.hasAttribute('preload')) { this.video_.setAttribute('preload', 'auto'); } - this.getAmpDoc() + pendingOriginPromise = this.getAmpDoc() .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. + return Services.timerFor(this.win) + .promise(1) + .then(() => this.loadPromise(this.video_)); }); } else { this.propagateLayoutChildren_(); } // loadPromise for media elements listens to `loadedmetadata`. - const promise = this.loadPromise(this.video_).then(() => { - this.element.dispatchCustomEvent(VideoEvents.LOAD); - }); + const promise = this.loadPromise(this.video_) + .then(null, (reason) => { + if (pendingOriginPromise) { + return pendingOriginPromise; + } + throw reason; + }) + .then(() => { + this.element.dispatchCustomEvent(VideoEvents.LOAD); + }); // Resolve layoutCallback right away if the video won't preload. if (this.element.getAttribute('preload') === 'none') { diff --git a/extensions/amp-video/0.1/test/test-amp-video.js b/extensions/amp-video/0.1/test/test-amp-video.js index dd6cf207e75d..05a093628682 100644 --- a/extensions/amp-video/0.1/test/test-amp-video.js +++ b/extensions/amp-video/0.1/test/test-amp-video.js @@ -42,7 +42,12 @@ describes.realWin( return '//someHost/foo.' + filetype.slice(filetype.indexOf('/') + 1); // assumes no optional params } - async function getVideo(attributes, children, opt_beforeLayoutCallback) { + async function getVideo( + attributes, + children, + opt_beforeLayoutCallback, + opt_noLayout + ) { const v = doc.createElement('amp-video'); for (const key in attributes) { v.setAttribute(key, attributes[key]); @@ -55,6 +60,9 @@ describes.realWin( doc.body.appendChild(v); await v.build(); + if (opt_noLayout) { + return; + } if (opt_beforeLayoutCallback) { opt_beforeLayoutCallback(v); } @@ -825,7 +833,7 @@ describes.realWin( }); }); - describe('should preconnect to the first cached source', () => { + describe('should preconnect to all sources', () => { let preconnect; beforeEach(() => { @@ -834,14 +842,23 @@ describes.realWin( }); it('no cached source', async () => { - await getVideo({ - src: 'https://example.com/video.mp4', - poster: 'https://example.com/poster.jpg', - width: 160, - height: 90, - }); + await getVideo( + { + src: 'https://example.com/video.mp4', + poster: 'https://example.com/poster.jpg', + width: 160, + height: 90, + }, + null, + null, + /* opt_noLayout */ true + ); - expect(preconnect.url).to.not.have.been.called; + expect(preconnect.url).to.have.been.calledOnce; + expect(preconnect.url.getCall(0)).to.have.been.calledWith( + env.sandbox.match.object, // AmpDoc + 'https://example.com/video.mp4' + ); }); it('cached source', async () => { @@ -861,19 +878,25 @@ describes.realWin( width: 160, height: 90, }, - [cachedSource] + [cachedSource], + null, + /* opt_noLayout */ true ); - expect(preconnect.url).to.have.been.calledOnce; + expect(preconnect.url).to.have.been.calledTwice; expect(preconnect.url.getCall(0)).to.have.been.calledWith( env.sandbox.match.object, // AmpDoc 'https://example-com.cdn.ampproject.org/m/s/video.mp4' ); + expect(preconnect.url.getCall(1)).to.have.been.calledWith( + env.sandbox.match.object, // AmpDoc + 'https://example.com/video.mp4' + ); }); it('mixed sources', async () => { const source = doc.createElement('source'); - source.setAttribute('src', 'video.mp4'); + source.setAttribute('src', 'https://example.com/video.mp4'); const cachedSource = doc.createElement('source'); cachedSource.setAttribute( @@ -890,13 +913,23 @@ describes.realWin( width: 160, height: 90, }, - [source, cachedSource] + [source, cachedSource], + null, + /* opt_noLayout */ true ); - expect(preconnect.url).to.have.been.calledOnce; + expect(preconnect.url).to.have.been.calledThrice; expect(preconnect.url.getCall(0)).to.have.been.calledWith( + env.sandbox.match.object, // AmpDoc + 'https://example.com/video.mp4' + ); + expect(preconnect.url.getCall(1)).to.have.been.calledWith( env.sandbox.match.object, // AmpDoc 'https://example-com.cdn.ampproject.org/m/s/video.mp4' ); + expect(preconnect.url.getCall(2)).to.have.been.calledWith( + env.sandbox.match.object, // AmpDoc + 'https://example.com/video.mp4' + ); }); }); diff --git a/src/preconnect.js b/src/preconnect.js index 5ad433dcc285..2a50b0d67306 100644 --- a/src/preconnect.js +++ b/src/preconnect.js @@ -112,6 +112,13 @@ export class PreconnectService { /** * Preconnects to a URL. Always also does a dns-prefetch because * browser support for that is better. + * + * It is safe to call this method during prerender with any value, + * because no action will be performed until the doc is visible. + * + * It is safe to call this method with non-HTTP(s) URLs as other URLs + * are skipped. + * * @param {!./service/ampdoc-impl.AmpDoc} ampdoc * @param {string} url * @param {boolean=} opt_alsoConnecting Set this flag if you also just @@ -193,6 +200,12 @@ export class PreconnectService { * Asks the browser to preload a URL. Always also does a preconnect * because browser support for that is better. * + * It is safe to call this method during prerender with any value, + * because no action will be performed until the doc is visible. + * + * It is safe to call this method with non-HTTP(s) URLs as other URLs + * are skipped. + * * @param {!./service/ampdoc-impl.AmpDoc} ampdoc * @param {string} url * @param {string=} opt_preloadAs