Fix nasty race condition in AMP video

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<string>}
    */
-  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,38 @@ 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) {
+          return pendingOriginPromise;
+        }
+        throw reason;
+      }
+    );
 
     // Resolve layoutCallback right away if the video won't preload.
     if (this.element.getAttribute('preload') === 'none') {

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'
+          );
         });
       });
 

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