From c7cb622c32ffe6e11301ac798c9a278851c488df Mon Sep 17 00:00:00 2001 From: Malte Ubl Date: Mon, 13 Apr 2020 20:27:27 -0700 Subject: [PATCH 1/7] Fix nasty race condition in AMP video 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. --- extensions/amp-video/0.1/amp-video.js | 29 ++++++++++++++++++++++----- 1 file changed, 24 insertions(+), 5 deletions(-) diff --git a/extensions/amp-video/0.1/amp-video.js b/extensions/amp-video/0.1/amp-video.js index 305c4bc92e5e..309c4e26d901 100644 --- a/extensions/amp-video/0.1/amp-video.js +++ b/extensions/amp-video/0.1/amp-video.js @@ -35,7 +35,10 @@ import {htmlFor} from '../../../src/static-template'; import {installVideoManagerForDoc} from '../../../src/service/video-manager-impl'; import {isExperimentOn} from '../../../src/experiments'; import {isLayoutSizeDefined} from '../../../src/layout'; -import {listen} from '../../../src/event-helper'; +import { + listen, + MEDIA_LOAD_FAILURE_SRC_PROPERTY, +} from '../../../src/event-helper'; import {mutedOrUnmutedEvent} from '../../../src/iframe-video'; import { propagateObjectFitStyles, @@ -322,23 +325,39 @@ 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( + () => { + this.element.dispatchCustomEvent(VideoEvents.LOAD); + }, + (reason) => { + if (pendingOriginPromise) { + this.video_[MEDIA_LOAD_FAILURE_SRC_PROPERTY] = undefined; + return pendingOriginPromise; + } + throw reason; + } + ); // Resolve layoutCallback right away if the video won't preload. if (this.element.getAttribute('preload') === 'none') { From 87aaf2edbfffc4c91bf3bdcb00e6953e9913eeda Mon Sep 17 00:00:00 2001 From: Malte Ubl Date: Tue, 14 Apr 2020 11:53:02 -0700 Subject: [PATCH 2/7] Fix lint --- extensions/amp-video/0.1/amp-video.js | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/extensions/amp-video/0.1/amp-video.js b/extensions/amp-video/0.1/amp-video.js index 309c4e26d901..db291867b521 100644 --- a/extensions/amp-video/0.1/amp-video.js +++ b/extensions/amp-video/0.1/amp-video.js @@ -15,6 +15,10 @@ */ import {EMPTY_METADATA} from '../../../src/mediasession-helper'; +import { + MEDIA_LOAD_FAILURE_SRC_PROPERTY, + listen, +} from '../../../src/event-helper'; import {Services} from '../../../src/services'; import {VideoEvents} from '../../../src/video-interface'; import {VisibilityState} from '../../../src/visibility-state'; @@ -35,10 +39,6 @@ import {htmlFor} from '../../../src/static-template'; import {installVideoManagerForDoc} from '../../../src/service/video-manager-impl'; import {isExperimentOn} from '../../../src/experiments'; import {isLayoutSizeDefined} from '../../../src/layout'; -import { - listen, - MEDIA_LOAD_FAILURE_SRC_PROPERTY, -} from '../../../src/event-helper'; import {mutedOrUnmutedEvent} from '../../../src/iframe-video'; import { propagateObjectFitStyles, From d2a8fccba2307848914d454e3f961cfe7514756f Mon Sep 17 00:00:00 2001 From: Malte Ubl Date: Tue, 14 Apr 2020 20:08:11 -0700 Subject: [PATCH 3/7] - Fix tests - Change preload to also preload non-cache sources - Clarify preconnect docs about the security invariants. --- extensions/amp-video/0.1/amp-video.js | 32 ++++-------- .../amp-video/0.1/test/test-amp-video.js | 49 ++++++++++++++----- src/preconnect.js | 13 +++++ 3 files changed, 60 insertions(+), 34 deletions(-) diff --git a/extensions/amp-video/0.1/amp-video.js b/extensions/amp-video/0.1/amp-video.js index db291867b521..b8f709d54b6d 100644 --- a/extensions/amp-video/0.1/amp-video.js +++ b/extensions/amp-video/0.1/amp-video.js @@ -15,17 +15,12 @@ */ import {EMPTY_METADATA} from '../../../src/mediasession-helper'; -import { - MEDIA_LOAD_FAILURE_SRC_PROPERTY, - listen, -} from '../../../src/event-helper'; import {Services} from '../../../src/services'; import {VideoEvents} from '../../../src/video-interface'; import {VisibilityState} from '../../../src/visibility-state'; import { childElementByTag, childElementsByTag, - elementByTag, fullscreenEnter, fullscreenExit, insertAfterOrAtStart, @@ -39,6 +34,7 @@ import {htmlFor} from '../../../src/static-template'; import {installVideoManagerForDoc} from '../../../src/service/video-manager-impl'; import {isExperimentOn} from '../../../src/experiments'; import {isLayoutSizeDefined} from '../../../src/layout'; +import {listen} from '../../../src/event-helper'; import {mutedOrUnmutedEvent} from '../../../src/iframe-video'; import { propagateObjectFitStyles, @@ -107,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 ); - } + }); } /** @@ -173,19 +167,14 @@ class AmpVideo extends AMP.BaseElement { * @private * @return {?string} */ - getVideoSourceForPreconnect_() { - if (this.getAmpDoc().getVisibilityState() === VisibilityState.PRERENDER) { - const source = this.getFirstCachedSource_(); - return (source && source.getAttribute('src')) || null; - } - let videoSrc = this.element.getAttribute('src'); - if (!videoSrc) { - const source = elementByTag(this.element, 'source'); - if (source) { - videoSrc = source.getAttribute('src'); - } + getVideoSourcesForPreconnect_() { + const videoSrc = this.element.getAttribute('src'); + if (videoSrc) { + return [videoSrc]; } - return videoSrc; + return toArray(childElementsByTag(this.element, 'source')).map((source) => + source.getAttribute('src') + ); } /** @override */ @@ -352,7 +341,6 @@ class AmpVideo extends AMP.BaseElement { }, (reason) => { if (pendingOriginPromise) { - this.video_[MEDIA_LOAD_FAILURE_SRC_PROPERTY] = undefined; return pendingOriginPromise; } throw reason; 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..a9494c55adaa 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); } @@ -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,7 +878,9 @@ describes.realWin( width: 160, height: 90, }, - [cachedSource] + [cachedSource], + null, + /* opt_noLayout */ true ); expect(preconnect.url).to.have.been.calledOnce; @@ -873,7 +892,7 @@ describes.realWin( 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,10 +909,16 @@ 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.calledTwice; 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' ); 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 From ccfcabcd97b53751d8c40a7b030c42e245c57514 Mon Sep 17 00:00:00 2001 From: Malte Ubl Date: Tue, 14 Apr 2020 20:22:31 -0700 Subject: [PATCH 4/7] Also preconnec to orig-src --- extensions/amp-video/0.1/amp-video.js | 15 ++++++++++++--- extensions/amp-video/0.1/test/test-amp-video.js | 14 +++++++++++--- 2 files changed, 23 insertions(+), 6 deletions(-) diff --git a/extensions/amp-video/0.1/amp-video.js b/extensions/amp-video/0.1/amp-video.js index b8f709d54b6d..74250feace94 100644 --- a/extensions/amp-video/0.1/amp-video.js +++ b/extensions/amp-video/0.1/amp-video.js @@ -172,9 +172,18 @@ class AmpVideo extends AMP.BaseElement { if (videoSrc) { return [videoSrc]; } - return toArray(childElementsByTag(this.element, 'source')).map((source) => - source.getAttribute('src') - ); + const srcs = []; + childElementsByTag(this.element, 'source').forEach((source) => { + const src = source.getAttribute('src'); + if (src) { + srcs.push(src); + } + const origSrc = source.getAttribute('amp-orig-src'); + if (origSrc) { + srcs.push(origSrc); + } + }); + return srcs; } /** @override */ 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 a9494c55adaa..05a093628682 100644 --- a/extensions/amp-video/0.1/test/test-amp-video.js +++ b/extensions/amp-video/0.1/test/test-amp-video.js @@ -833,7 +833,7 @@ describes.realWin( }); }); - describe('should preconnect to the first cached source', () => { + describe('should preconnect to all sources', () => { let preconnect; beforeEach(() => { @@ -883,11 +883,15 @@ describes.realWin( /* 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 () => { @@ -913,7 +917,7 @@ describes.realWin( null, /* opt_noLayout */ true ); - expect(preconnect.url).to.have.been.calledTwice; + 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' @@ -922,6 +926,10 @@ describes.realWin( 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' + ); }); }); From ea12b440d681f9300516a9603e084c30bb0734c8 Mon Sep 17 00:00:00 2001 From: Malte Ubl Date: Wed, 15 Apr 2020 06:42:48 -0700 Subject: [PATCH 5/7] Fix type, add comment --- extensions/amp-video/0.1/amp-video.js | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/extensions/amp-video/0.1/amp-video.js b/extensions/amp-video/0.1/amp-video.js index 74250feace94..b7f1d7af3a28 100644 --- a/extensions/amp-video/0.1/amp-video.js +++ b/extensions/amp-video/0.1/amp-video.js @@ -165,7 +165,7 @@ class AmpVideo extends AMP.BaseElement { /** * @private - * @return {?string} + * @return {!Array} */ getVideoSourcesForPreconnect_() { const videoSrc = this.element.getAttribute('src'); @@ -178,6 +178,7 @@ class AmpVideo extends AMP.BaseElement { if (src) { srcs.push(src); } + // 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); From 59a56168a3ae52f34ddfbba8252632bb03d8f187 Mon Sep 17 00:00:00 2001 From: Malte Ubl Date: Wed, 15 Apr 2020 13:01:08 -0700 Subject: [PATCH 6/7] Address review comment --- extensions/amp-video/0.1/amp-video.js | 11 +++++++---- 1 file changed, 7 insertions(+), 4 deletions(-) diff --git a/extensions/amp-video/0.1/amp-video.js b/extensions/amp-video/0.1/amp-video.js index b7f1d7af3a28..99b1d89ec492 100644 --- a/extensions/amp-video/0.1/amp-video.js +++ b/extensions/amp-video/0.1/amp-video.js @@ -321,6 +321,10 @@ class AmpVideo extends AMP.BaseElement { this.propagateCachedSources_(); + const videoLoaded = () => { + this.element.dispatchCustomEvent(VideoEvents.LOAD); + }; + // 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. @@ -338,7 +342,8 @@ class AmpVideo extends AMP.BaseElement { // load. return Services.timerFor(this.win) .promise(1) - .then(() => this.loadPromise(this.video_)); + .then(() => this.loadPromise(this.video_)) + .then(videoLoaded); }); } else { this.propagateLayoutChildren_(); @@ -346,9 +351,7 @@ class AmpVideo extends AMP.BaseElement { // loadPromise for media elements listens to `loadedmetadata`. const promise = this.loadPromise(this.video_).then( - () => { - this.element.dispatchCustomEvent(VideoEvents.LOAD); - }, + videoLoaded, (reason) => { if (pendingOriginPromise) { return pendingOriginPromise; From 9d76adc31a9b7590174cc1c5e6387c8e44dfc4da Mon Sep 17 00:00:00 2001 From: Malte Ubl Date: Wed, 15 Apr 2020 13:20:48 -0700 Subject: [PATCH 7/7] Simplify --- extensions/amp-video/0.1/amp-video.js | 18 +++++++----------- 1 file changed, 7 insertions(+), 11 deletions(-) diff --git a/extensions/amp-video/0.1/amp-video.js b/extensions/amp-video/0.1/amp-video.js index 99b1d89ec492..6644443d1ab6 100644 --- a/extensions/amp-video/0.1/amp-video.js +++ b/extensions/amp-video/0.1/amp-video.js @@ -321,10 +321,6 @@ class AmpVideo extends AMP.BaseElement { this.propagateCachedSources_(); - const videoLoaded = () => { - this.element.dispatchCustomEvent(VideoEvents.LOAD); - }; - // 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. @@ -342,23 +338,23 @@ class AmpVideo extends AMP.BaseElement { // load. return Services.timerFor(this.win) .promise(1) - .then(() => this.loadPromise(this.video_)) - .then(videoLoaded); + .then(() => this.loadPromise(this.video_)); }); } else { this.propagateLayoutChildren_(); } // loadPromise for media elements listens to `loadedmetadata`. - const promise = this.loadPromise(this.video_).then( - videoLoaded, - (reason) => { + 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') {