Fix: Dont install SW via amp-install-serviceworker for safari (#19922)

* fixing method call for esm build * ua check for iphone * adressing comments * reverting unwanted changes * fixing tests * independent user error check * adding an example
ampproject · Dec 26, 2018 · fbf177b · fbf177b
1 parent b32fca0
commit fbf177b
Show file tree

Hide file tree

Showing 4 changed files with 45 additions and 7 deletions.
diff --git a/examples/amp-install-serviceworker.amp.html b/examples/amp-install-serviceworker.amp.html
@@ -0,0 +1,19 @@
+<!doctype html>
+<html ⚡>
+<head>
+  <meta charset="utf-8">
+  <title>amp-install-serviceworker example</title>
+  <link rel="canonical" href="amps.html">
+  <meta name="viewport" content="width=device-width,minimum-scale=1,initial-scale=1">
+  <style amp-boilerplate>body{-webkit-animation:-amp-start 8s steps(1,end) 0s 1 normal both;-moz-animation:-amp-start 8s steps(1,end) 0s 1 normal both;-ms-animation:-amp-start 8s steps(1,end) 0s 1 normal both;animation:-amp-start 8s steps(1,end) 0s 1 normal both}@-webkit-keyframes -amp-start{from{visibility:hidden}to{visibility:visible}}@-moz-keyframes -amp-start{from{visibility:hidden}to{visibility:visible}}@-ms-keyframes -amp-start{from{visibility:hidden}to{visibility:visible}}@-o-keyframes -amp-start{from{visibility:hidden}to{visibility:visible}}@keyframes -amp-start{from{visibility:hidden}to{visibility:visible}}</style><noscript><style amp-boilerplate>body{-webkit-animation:none;-moz-animation:none;-ms-animation:none;animation:none}</style></noscript>
+  <script async custom-element="amp-install-serviceworker" src="https://cdn.ampproject.org/v0/amp-install-serviceworker-0.1.js"></script>
+  <script async src="https://cdn.ampproject.org/v0.js"></script>
+</head>
+<body>
+  <amp-install-serviceworker
+    src="/examples/example-sw.js"
+    data-scope="/examples/"
+    layout="nodisplay">
+  </amp-install-serviceworker>
+</body>
+</html>
diff --git a/examples/example-sw.js b/examples/example-sw.js
@@ -0,0 +1,3 @@
+self.addEventListener('fetch', e => {
+  return fetch(e.request);
+});
diff --git a/extensions/amp-install-serviceworker/0.1/amp-install-serviceworker.js b/extensions/amp-install-serviceworker/0.1/amp-install-serviceworker.js
@@ -44,6 +44,9 @@ export class AmpInstallServiceWorker extends AMP.BaseElement {
 
     /** @visibleForTesting {?UrlRewriter_}  */
     this.urlRewriter_ = null;
+
+    /** @private @const {boolean}*/
+    this.isSafari_ = Services.platformFor(this.win).isSafari();
   }
 
   /** @override */
@@ -57,8 +60,8 @@ export class AmpInstallServiceWorker extends AMP.BaseElement {
     const src = this.element.getAttribute('src');
     urlService.assertHttpsUrl(src, this.element);
 
-    if (urlService.isProxyOrigin(src) ||
-        urlService.isProxyOrigin(win.location.href)) {
+    if ((urlService.isProxyOrigin(src) ||
+        urlService.isProxyOrigin(win.location.href)) && !this.isSafari_) {
       const iframeSrc = this.element.getAttribute('data-iframe-src');
       if (iframeSrc) {
         urlService.assertHttpsUrl(iframeSrc, this.element);
@@ -87,6 +90,14 @@ export class AmpInstallServiceWorker extends AMP.BaseElement {
           'Did not install ServiceWorker because it does not ' +
           'match the current origin: ' + src);
     }
+
+    if ((urlService.isProxyOrigin(src) ||
+    urlService.isProxyOrigin(win.location.href)) && this.isSafari_) {
+      // https://webkit.org/blog/8090/workers-at-your-service/
+      this.user().error(TAG,
+          'Did not install ServiceWorker because of safari double keyring ' +
+          'caching as it will not have any effect');
+    }
   }
 
   /**
@@ -288,7 +299,6 @@ class UrlRewriter_ {
   }
 }
 
-
 /**
  * Installs the service worker at src via direct service worker installation.
  * @param {!Window} win

diff --git a/extensions/amp-install-serviceworker/0.1/test/test-amp-install-serviceworker.js b/extensions/amp-install-serviceworker/0.1/test/test-amp-install-serviceworker.js
@@ -410,7 +410,7 @@ describes.realWin('amp-install-serviceworker', {
       });
     });
 
-    function testIframe() {
+    function testIframe(callCount = 1) {
       const iframeSrc = 'https://www.example.com/install-sw.html';
       install.setAttribute('data-iframe-src', iframeSrc);
       let iframe;
@@ -437,12 +437,13 @@ describes.realWin('amp-install-serviceworker', {
       implementation.buildCallback();
       return Promise.all([whenVisible, loadPromise(implementation.win)]).then(
           () => {
-            expect(mutateElement).to.have.been.calledOnce;
+            expect(mutateElement).to.have.been.callCount(callCount);
           });
     }
 
-    it('should inject iframe on proxy if provided (valid canonical)',
-        testIframe);
+    it('should inject iframe on proxy if provided (valid canonical)', () => {
+      return testIframe();
+    });
 
     it('should inject iframe on proxy if provided (valid source)', () => {
       docInfo = {
@@ -467,6 +468,11 @@ describes.realWin('amp-install-serviceworker', {
         implementation.buildCallback();
       }).to.throw(/https/); });
     });
+
+    it('should not inject iframe on proxy if safari', () => {
+      implementation.isSafari_ = true;
+      return allowConsoleError(() => testIframe(0));
+    });
   });
 });