XHR fetch when view first becomes visible #14837

alabiaga · 2018-04-24T17:40:01Z

kristoferbaxter · 2018-04-24T17:41:58Z

src/service/xhr-impl.js

@@ -170,30 +167,30 @@ export class Xhr {
   * XHRs are intercepted if all of the following are true:
   * - The AMP doc is in single doc mode
   * - The viewer has the `xhrInterceptor` capability
-   * - The Viewer is a trusted viewer or AMP is currently in developement mode
+   * - The Viewer is a trusted viewer or ApMP is currently in developement mode


Nit: AMP, looks like a typo!

kristoferbaxter · 2018-04-24T17:43:45Z

test/functional/test-xhr.js

@@ -14,7 +14,7 @@
 * limitations under the License.
 */

-import * as sinon from 'sinon';
+import * as sinon from 'sinon';;


Nit: Double semi-colon?

kristoferbaxter · 2018-04-24T17:46:43Z

test/functional/test-xhr.js

+    });
+    ampdocServiceForStub.returns({
+      isSingleDoc: () => false,
+      getAmpDoc: () => { return ampdocViewerStub; },


Not a huge deal since this is a test file however I believe you could reduce this down a bit.

ampdocServiceForStub.returns({ isSingleDoc: () => false, getAmpDoc: () => ampdocViewerStub, });

kristoferbaxter · 2018-04-24T17:47:45Z

test/functional/test-xhr.js

@@ -73,7 +75,17 @@ describe.configure().skipSafari().run('XHR', function() {
  beforeEach(() => {
    sandbox = sinon.sandbox.create();
    ampdocServiceForStub = sandbox.stub(Services, 'ampdocServiceFor');
-    ampdocServiceForStub.returns({isSingleDoc: () => false});
+    ampdocViewerStub = sandbox.stub(Services, 'viewerForDoc');
+    ampdocViewerStub.returns({


Also here:

ampdocViewerStub.returns({ whenFirstVisible: () => Promise.resolve(), });

alabiaga · 2018-04-24T19:19:50Z

@jridgewell sending out for early review. i just modified the tests so that existing tests now pass but I need to create a new test as well.

jridgewell · 2018-04-24T19:38:40Z

src/service/xhr-impl.js

-    /** @private {?./ampdoc-impl.AmpDoc} */
-    this.ampdocSingle_ =
-        ampdocService.isSingleDoc() ? ampdocService.getAmpDoc() : null;
+    this.ampdocService = Services.ampdocServiceFor(win);


The isSingleDoc is still necessary (context).

Also, leave this as a private field, they get minified better.

hey justin, i know we discussed this a bit yesterday but it might have been under a different context. Do we really need this private field. in my changes you can see that i check if amp is single doc in the fetch call and pass along that info to the methods that need that info. I dont think this private field is needed at all.

Leaving this as a private is still valuable.

jridgewell · 2018-04-24T19:39:14Z

src/service/xhr-impl.js

-        !Services.viewerForDoc(this.ampdocSingle_).hasBeenVisible()) {
-      dev().error('XHR', 'attempted to fetch %s before viewer was visible',
+        parseUrl(input).origin != this.win.location.origin) {
+      dev().error('XHR', 'attempted to fetch URL from different origin',
          input);
    }


This entire if statement can be removed, since we're now waiting for viewer visible regardless.

jridgewell · 2018-04-24T19:40:48Z

src/service/xhr-impl.js

-    });
+    return Services.viewerForDoc(ampdocSingle).whenFirstVisible()
+        .then(() => {
+          return this.maybeIntercept_(input, init, ampdocSingle);


@zhangsu: What are the privacy guarantees of the Gmail interceptor? Does it need us to wait for the doc to be shown?

(Basically, should this be the first call, or should whenFirstVisible be first)

Thanks for checking with us. Waiting for the email to be shown before firing XHRs sounds good to me since we also want the same level of privacy.

Just curious: what's an example today where XHRs can be fired before the viewer is visible (e.g. in prerender mode)? amp-list shouldn't do that because prerenderAllowed is false, right?

👍, that makes it safer on our side as well.

what's an example today where XHRs can be fired before the viewer is visible (e.g. in prerender mode)?

Generally, these are bugs. There are exceptions for connecting to the CDN itself (or whatever origin is hosting the page), since they've already received the document request.

amp-list shouldn't do that because prerenderAllowed is false, right?

Correct. The prerenderAllowed is one way to catch them, but we're increasingly getting extensions that need to make requests before layout (ads 🙄, and one other). These make requests in #buildCallback or other earlier lifecycle callbacks, and they sometimes forget to wait for visible.

alabiaga · 2018-05-02T16:57:47Z

@jridgewell Hey Justin, pls take another look. I made changes to make the check for viewer visibility to encapsulate the logic where other viewer related checks are made as well and ensured that the fetch happens at all times which wasn't the case in my initial change.

jridgewell · 2018-05-02T18:57:43Z

src/service/xhr-impl.js

-    /** @private {?./ampdoc-impl.AmpDoc} */
-    this.ampdocSingle_ =
-        ampdocService.isSingleDoc() ? ampdocService.getAmpDoc() : null;
+    this.ampdocService = Services.ampdocServiceFor(win);


Leaving this as a private is still valuable.

jridgewell · 2018-05-02T18:58:48Z

src/service/xhr-impl.js

+    // getAmpDoc.
+    // TODO(alabiaga): This should be investigated and fixed.
+    const ampdoc = this.ampdocService.isSingleDoc() ?
+      this.ampdocService.getAmpDoc(this.win.document) : null;


The this.win.document is unnecessary for single docs.

Done. The reason I added it was because I was trying to do away with the isSingleDoc() check..and just make the call getAmpDoc..which was then causing test-bind-impl.js to fail and I was trying to get it to work by passing in the node, though it was not sufficient as the AmpDoc.shellShadowDoc_ was not defined. I wrote a TODO on that line of code as it seems like buggy code to me..or the test is not set up properly. So i believe this is where your initial comment of why this is still required now makes sense...i had to actually debug through the code for it to actually make sense.

jridgewell · 2018-05-02T19:00:50Z

src/service/xhr-impl.js

-      }
-      return (this.win.fetch || fetchPolyfill).apply(null, arguments);
-    });
+    return this.maybeIntercept_(input, init, ampdoc)


No need to pass in ampdoc since it's a property on this. Just access it in the method, and remove its uses in this method.

fixed to use the assigned property ampdocSingle_ as it was originally.

jridgewell · 2018-05-02T19:01:56Z

src/service/xhr-impl.js

-  maybeIntercept_(input, init) {
-    if (!this.ampdocSingle_) {
+  maybeIntercept_(input, init, ampdoc) {
+    if (!this.ampdocService.isSingleDoc()) {


Look at that, we're already accessing it as the property. 😉

fine you win. reverted to a field property 😄

jridgewell · 2018-05-02T19:02:43Z

src/service/xhr-impl.js

      return Promise.resolve();
    }
-
-    const htmlElement = this.ampdocSingle_.getRootNode().documentElement;
+    const htmlElement = ampdoc.getRootNode().documentElement;
    const docOptedIn = htmlElement.hasAttribute('allow-xhr-interception');
    if (!docOptedIn) {
      return Promise.resolve();


Bug: We still need to wait for viewer visible in this case.

jridgewell · 2018-05-02T19:04:10Z

src/service/xhr-impl.js

+          if (!viewer.hasCapability('xhrInterceptor')) {
+            return Promise.resolve();
+          }
+          return viewer.isTrustedViewer().then(viewerTrusted => {


Nit: we can unnest this if we take extract out the "hasCapability" into a separate path.

unnested hasCapability and docOptedIn checked

…fetching the amp doc. for test test-bind-impl

jridgewell · 2018-05-02T21:28:21Z

src/service/xhr-impl.js

@@ -109,7 +109,15 @@ export class Xhr {
    /** @const {!Window} */
    this.win = win;

+    /** @private */


This isn't a property, so can't be @private.

jridgewell · 2018-05-02T21:35:36Z

src/service/xhr-impl.js

+          }
+          return viewer.isTrustedViewer();
+        }).then(viewerTrusted => {
+          if (!viewerTrusted && !getMode(this.win).development) {


Sorry, this lead to confusion when I recommended unnesting. What I meant is that these should be two separate promise chains:

// Shorthand, don't actually write like this maybeIntercept_(input, init) { if (!single) return Promise.resolve(); const vis = viewer.whenFirstVisible(); if (!hasCapability) return vis; return vis.then(() => { if (!optedIn) { return false; } return viewer.isTrusted() }).then(trusted => { if (!trusted) return; return sendMessage(...); }); }

@jridgewell no worries. hopefully i got it right this time. otherwise we can discuss in person. thanks

jridgewell · 2018-05-03T17:08:59Z

src/service/xhr-impl.js

+    const docOptedIn = htmlElement.hasAttribute('allow-xhr-interception');
+    const isDevMode = getMode(this.win).development;
+    if (!docOptedIn) {
+      return Promise.resolve();


Return whenFirstVisible.

right. Done. Thanks

* fetch when viewer is visible * fetch by default when viewer first becomes visible * fix typo * fixes from kris' comments * more fixes from kris' comments * add test * address jridgewell comments * fix usage of spy * fix xhr, to resolve earlier if not a single doc * fix per justin comment. * minor refactor * get ampdoc fetch with isSingleDoc check to fix dev error thrown when fetching the amp doc. for test test-bind-impl * address jridgewell comments * address jridgewell comments * address comments * return promise fix

googlebot added the cla: yes label Apr 24, 2018

kristoferbaxter reviewed Apr 24, 2018

View reviewed changes

alabiaga requested a review from jridgewell April 24, 2018 19:19

jridgewell requested changes Apr 24, 2018

View reviewed changes

jridgewell requested changes May 2, 2018

View reviewed changes

alabiaga added 14 commits May 2, 2018 15:26

fetch when viewer is visible

91f47a5

fetch by default when viewer first becomes visible

723e537

fix typo

7dd56d5

fixes from kris' comments

56287de

more fixes from kris' comments

54a8abc

add test

c692216

address jridgewell comments

ff496c2

fix usage of spy

c80e0d5

fix xhr, to resolve earlier if not a single doc

a11d7a2

fix per justin comment.

37d2075

minor refactor

79b4bd2

get ampdoc fetch with isSingleDoc check to fix dev error thrown when …

a61e06e

…fetching the amp doc. for test test-bind-impl

address jridgewell comments

45f8791

address jridgewell comments

b022091

jridgewell requested changes May 2, 2018

View reviewed changes

address comments

349a212

jridgewell reviewed May 3, 2018

View reviewed changes

return promise fix

28b6f4d

jridgewell approved these changes May 3, 2018

View reviewed changes

alabiaga merged commit bdbb745 into ampproject:master May 3, 2018

jridgewell added this to To do in Make Malte, Dima, Justin Happy via automation Sep 6, 2018

jridgewell moved this from To do to XSS Injections Caught in Make Malte, Dima, Justin Happy Sep 6, 2018

jridgewell moved this from XSS Injections Caught to Reviewed in Make Malte, Dima, Justin Happy Sep 6, 2018

dreamofabear mentioned this pull request Jun 6, 2019

Bypass xhr interceptor for amp-worker invocation #22600

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

XHR fetch when view first becomes visible #14837

XHR fetch when view first becomes visible #14837

alabiaga commented Apr 24, 2018

kristoferbaxter Apr 24, 2018

kristoferbaxter Apr 24, 2018

kristoferbaxter Apr 24, 2018

kristoferbaxter Apr 24, 2018

alabiaga commented Apr 24, 2018

jridgewell Apr 24, 2018

alabiaga Apr 26, 2018

jridgewell May 2, 2018

jridgewell Apr 24, 2018

jridgewell Apr 24, 2018

zhangsu Apr 26, 2018

jridgewell Apr 26, 2018

alabiaga commented May 2, 2018 •

edited

jridgewell May 2, 2018

jridgewell May 2, 2018

alabiaga May 2, 2018

jridgewell May 2, 2018

alabiaga May 2, 2018

jridgewell May 2, 2018

alabiaga May 2, 2018

jridgewell May 2, 2018

alabiaga May 2, 2018

jridgewell May 2, 2018

alabiaga May 2, 2018

jridgewell May 2, 2018

jridgewell May 2, 2018

alabiaga May 3, 2018

jridgewell May 3, 2018

alabiaga May 3, 2018

XHR fetch when view first becomes visible #14837

XHR fetch when view first becomes visible #14837

Conversation

alabiaga commented Apr 24, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alabiaga commented Apr 24, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alabiaga commented May 2, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alabiaga commented May 2, 2018 •

edited