Doubleclick/AdSense FF: Append error parameter to ad url if XHR CORS …

…fails (#10654) * Doubleclick/AdSense FF: Append aet=n to ad url if frame GET due to XHR CORS network failure * fix lint/type errors * fix trunc test to use buildUrl * fix presubmit failure
ampproject · Aug 1, 2017 · fe1b1bd · fe1b1bd
1 parent 3a06c99
commit fe1b1bd
Show file tree

Hide file tree

Showing 8 changed files with 187 additions and 9 deletions.
diff --git a/ads/google/a4a/test/test-utils.js b/ads/google/a4a/test/test-utils.js
@@ -21,7 +21,10 @@ import {
   EXPERIMENT_ATTRIBUTE,
   googleAdUrl,
   mergeExperimentIds,
+  maybeAppendErrorParameter,
+  TRUNCATION_PARAM,
 } from '../utils';
+import {buildUrl} from '../url-builder';
 import {createElementWithAttributes} from '../../../../src/dom';
 import {
   installExtensionsService,
@@ -343,4 +346,20 @@ describe('Google A4A utils', () => {
       expect(mergeExperimentIds(['frob'])).to.equal('');
     });
   });
+
+  describe('#maybeAppendErrorParameter', () => {
+    const url = 'https://foo.com/bar?hello=world&one=true';
+    it('should append parameter', () => {
+      expect(maybeAppendErrorParameter(url, 'n')).to.equal(url + '&aet=n');
+    });
+    it('should not append parameter if already present', () => {
+      expect(maybeAppendErrorParameter(url + '&aet=already', 'n')).to.not.be.ok;
+    });
+    it('should not append parameter if truncated', () => {
+      const truncUrl = buildUrl(
+          'https://foo.com/bar', {hello: 'world'}, 15, TRUNCATION_PARAM);
+      expect(truncUrl.indexOf(TRUNCATION_PARAM.name) != -1);
+      expect(maybeAppendErrorParameter(truncUrl, 'n')).to.not.be.ok;
+    });
+  });
 });
diff --git a/ads/google/a4a/utils.js b/ads/google/a4a/utils.js
@@ -69,6 +69,12 @@ export const EXPERIMENT_ATTRIBUTE = 'data-experiment-id';
  */
 export let AmpAnalyticsConfigDef;
 
+/**
+ * @const {!./url-builder.QueryParameterDef}
+ * @visibleForTesting
+ */
+export const TRUNCATION_PARAM = {name: 'trunc', value: '1'};
+
 /**
  * Check whether Google Ads supports the A4A rendering pathway is valid for the
  * environment by ensuring native crypto support and page originated in the
@@ -248,7 +254,7 @@ export function googleAdUrl(
  */
 export function truncAndTimeUrl(baseUrl, parameters, startTime) {
   return buildUrl(
-      baseUrl, parameters, MAX_URL_LENGTH - 10, {name: 'trunc', value: '1'})
+      baseUrl, parameters, MAX_URL_LENGTH - 10, TRUNCATION_PARAM)
     + '&dtd=' + elapsedTimeWithCeiling(Date.now(), startTime);
 }
 
@@ -541,3 +547,26 @@ export function getEnclosingContainerTypes(adElement) {
   }
   return Object.keys(containerTypeSet);
 }
+
+/**
+ * Appends parameter to ad request indicating error state so long as error
+ * parameter is not already present or url has been truncated.
+ * @param {string} adUrl used for network request
+ * @param {string} parameterValue to be appended
+ * @return {string|undefined} potentially modified url, undefined
+ */
+export function maybeAppendErrorParameter(adUrl, parameterValue) {
+  dev().assert(!!adUrl && !!parameterValue);
+  // Add parameter indicating error so long as the url has not already been
+  // truncated and error parameter is not already present.  Note that we assume
+  // that added, error parameter length will be less than truncation parameter
+  // so adding will not cause length to exceed maximum.
+  if (new RegExp(`[?|&](${encodeURIComponent(TRUNCATION_PARAM.name)}=` +
+      `${encodeURIComponent(String(TRUNCATION_PARAM.value))}|aet=[^&]*)$`)
+      .test(adUrl)) {
+    return;
+  }
+  const modifiedAdUrl = adUrl + `&aet=${parameterValue}`;
+  dev().assert(modifiedAdUrl.length <= MAX_URL_LENGTH);
+  return modifiedAdUrl;
+}
diff --git a/extensions/amp-a4a/0.1/amp-a4a.js b/extensions/amp-a4a/0.1/amp-a4a.js
@@ -56,8 +56,8 @@ import {getTimingDataAsync} from '../../../src/service/variable-source';
 import {getContextMetadata} from '../../../src/iframe-attributes';
 
 // Uncomment the next two lines when testing locally.
-// import '../../amp-ad/0.1/amp-ad-ui';
-// import '../../amp-ad/0.1/amp-ad-xorigin-iframe-handler';
+import '../../amp-ad/0.1/amp-ad-ui';
+import '../../amp-ad/0.1/amp-ad-xorigin-iframe-handler';
 
 /** @type {Array<string>} */
 const METADATA_STRINGS = [
@@ -1092,16 +1092,41 @@ export class AmpA4A extends AMP.BaseElement {
     };
     return Services.xhrFor(this.win)
         .fetch(adUrl, xhrInit)
-        .catch(unusedReason => {
+        .catch(error => {
           // If an error occurs, let the ad be rendered via iframe after delay.
           // TODO(taymonbeal): Figure out a more sophisticated test for deciding
           // whether to retry with an iframe after an ad request failure or just
           // give up and render the fallback content (or collapse the ad slot).
           this.protectedEmitLifecycleEvent_('networkError');
+          const networkFailureHandlerResult =
+              this.onNetworkFailure(error, this.adUrl_);
+          dev().assert(!!networkFailureHandlerResult);
+          if (networkFailureHandlerResult.frameGetDisabled) {
+            // Reset adUrl to null which will cause layoutCallback to not
+            // fetch via frame GET.
+            dev().info(
+                TAG, 'frame get disabled as part of network failure handler');
+            this.resetAdUrl();
+          } else {
+            this.adUrl_ = networkFailureHandlerResult.adUrl || this.adUrl_;
+          }
           return null;
         });
   }
 
+  /**
+   * Called on network failure sending XHR CORS ad request allowing for
+   * modification of ad url and prevent frame GET request on layoutCallback.
+   * By default, GET frame request will be executed with same ad URL as used
+   * for XHR CORS request.
+   * @param {*} unusedError from network failure
+   * @param {string} unusedAdUrl used for network request
+   * @return {!{adUrl: (string|undefined), frameGetDisabled: (boolean|undefined)}}
+   */
+  onNetworkFailure(unusedError, unusedAdUrl) {
+    return {};
+  }
+
   /**
    * To be overridden by network specific implementation indicating which
    * signing service(s) is to be used.

diff --git a/extensions/amp-a4a/0.1/test/test-amp-a4a.js b/extensions/amp-a4a/0.1/test/test-amp-a4a.js
@@ -977,24 +977,83 @@ describe('amp-a4a', () => {
         const a4aElement = createA4aElement(doc);
         const a4a = new MockA4AImpl(a4aElement);
         const getAdUrlSpy = sandbox.spy(a4a, 'getAdUrl');
+        const onNetworkFailureSpy = sandbox.spy(a4a, 'onNetworkFailure');
         a4a.buildCallback();
         const lifecycleEventStub = sandbox.stub(
             a4a, 'protectedEmitLifecycleEvent_');
         a4a.onLayoutMeasure();
         expect(a4a.adPromise_).to.be.instanceof(Promise);
         return a4a.layoutCallback().then(() => {
-          expect(getAdUrlSpy.calledOnce, 'getAdUrl called exactly once')
-              .to.be.true;
+          expect(getAdUrlSpy, 'getAdUrl called exactly once').to.be.calledOnce;
+          expect(onNetworkFailureSpy,
+              'onNetworkFailureSpy called exactly once').to.be.calledOnce;
+          // Verify iframe presence and lack of visibility hidden
+          const iframe = a4aElement.querySelector('iframe[src]');
+          expect(iframe).to.be.ok;
+          expect(iframe.src.indexOf(TEST_URL)).to.equal(0);
+          expect(iframe).to.be.visible;
+          expect(onCreativeRenderSpy.withArgs(false)).to.be.called;
+          expect(lifecycleEventStub).to.be.calledWith('networkError');
+        });
+      });
+    });
+    it('should use adUrl from onNetworkFailure', () => {
+      return createIframePromise().then(fixture => {
+        setupForAdTesting(fixture);
+        fetchMock.getOnce(
+            TEST_URL + '&__amp_source_origin=about%3Asrcdoc',
+            Promise.reject(networkFailure()), {name: 'ad'});
+        const doc = fixture.doc;
+        const a4aElement = createA4aElement(doc);
+        const a4a = new MockA4AImpl(a4aElement);
+        const getAdUrlSpy = sandbox.spy(a4a, 'getAdUrl');
+        sandbox.stub(a4a, 'onNetworkFailure')
+            .withArgs(sinon.match(val =>
+              val.message && val.message.indexOf('XHR Failed fetching') == 0),
+            TEST_URL)
+            .returns({adUrl: TEST_URL + '&err=true'});
+        a4a.buildCallback();
+        const lifecycleEventStub = sandbox.stub(
+            a4a, 'protectedEmitLifecycleEvent_');
+        a4a.onLayoutMeasure();
+        expect(a4a.adPromise_).to.be.instanceof(Promise);
+        return a4a.layoutCallback().then(() => {
+          expect(getAdUrlSpy, 'getAdUrl called exactly once').to.be.calledOnce;
           // Verify iframe presence and lack of visibility hidden
           const iframe = a4aElement.querySelector('iframe[src]');
           expect(iframe).to.be.ok;
           expect(iframe.src.indexOf(TEST_URL)).to.equal(0);
+          expect(/&err=true/.test(iframe.src), iframe.src).to.be.true;
           expect(iframe).to.be.visible;
           expect(onCreativeRenderSpy.withArgs(false)).to.be.called;
           expect(lifecycleEventStub).to.be.calledWith('networkError');
         });
       });
     });
+    it('should not execute frame GET if disabled via onNetworkFailure', () => {
+      return createIframePromise().then(fixture => {
+        setupForAdTesting(fixture);
+        fetchMock.getOnce(
+            TEST_URL + '&__amp_source_origin=about%3Asrcdoc',
+            Promise.reject(networkFailure()), {name: 'ad'});
+        const doc = fixture.doc;
+        const a4aElement = createA4aElement(doc);
+        const a4a = new MockA4AImpl(a4aElement);
+        const getAdUrlSpy = sandbox.spy(a4a, 'getAdUrl');
+        sandbox.stub(a4a, 'onNetworkFailure')
+            .withArgs(sinon.match(val =>
+              val.message && val.message.indexOf('XHR Failed fetching') == 0),
+            TEST_URL)
+            .returns({frameGetDisabled: true});
+        a4a.buildCallback();
+        a4a.onLayoutMeasure();
+        return a4a.layoutCallback().then(() => {
+          expect(getAdUrlSpy, 'getAdUrl called exactly once').to.be.calledOnce;
+          const iframe = a4aElement.querySelector('iframe');
+          expect(iframe).to.not.be.ok;
+        });
+      });
+    });
     it('should handle XHR error when resolves before layoutCallback', () => {
       return createIframePromise().then(fixture => {
         setupForAdTesting(fixture);

diff --git a/extensions/amp-ad-network-adsense-impl/0.1/amp-ad-network-adsense-impl.js b/extensions/amp-ad-network-adsense-impl/0.1/amp-ad-network-adsense-impl.js
@@ -37,6 +37,7 @@ import {
   extractAmpAnalyticsConfig,
   addCsiSignalsToAmpAnalyticsConfig,
   QQID_HEADER,
+  maybeAppendErrorParameter,
 } from '../../../ads/google/a4a/utils';
 import {
   googleLifecycleReporterFactory,
@@ -61,6 +62,9 @@ import {
 /** @const {string} */
 const ADSENSE_BASE_URL = 'https://googleads.g.doubleclick.net/pagead/ads';
 
+/** @const {string} */
+const TAG = 'amp-ad-network-adsense-impl';
+
 /**
  * See `VisibilityState` enum.
  * @const {!Object<string, string>}
@@ -208,6 +212,12 @@ export class AmpAdNetworkAdsenseImpl extends AmpA4A {
         this, ADSENSE_BASE_URL, startTime, parameters, experimentIds);
   }
 
+  /** @override */
+  onNetworkFailure(error, adUrl) {
+    dev().info(TAG, 'network error, attempt adding of error parameter', error);
+    return {adUrl: maybeAppendErrorParameter(adUrl, 'n')};
+  }
+
   /** @override */
   extractSize(responseHeaders) {
     setGoogleLifecycleVarsFromHeaders(responseHeaders, this.lifecycleReporter_);

diff --git a/extensions/amp-ad-network-adsense-impl/0.1/test/test-amp-ad-network-adsense-impl.js b/extensions/amp-ad-network-adsense-impl/0.1/test/test-amp-ad-network-adsense-impl.js
@@ -93,9 +93,6 @@ describes.realWin('amp-ad-network-adsense-impl', {amp: true}, env => {
     impl = new AmpAdNetworkAdsenseImpl(element);
   });
 
-  afterEach(() => {
-  });
-
   describe('#isValidElement', () => {
     it('should be valid', () => {
       expect(impl.isValidElement()).to.be.true;
@@ -171,6 +168,15 @@ describes.realWin('amp-ad-network-adsense-impl', {amp: true}, env => {
     });
   });
 
+  describe('#onNetworkFailure', () => {
+
+    it('should append error parameter', () => {
+      const TEST_URL = 'https://somenetwork.com/foo?hello=world&a=b';
+      expect(impl.onNetworkFailure(new Error('xhr failure'), TEST_URL))
+          .to.jsonEqual({adUrl: TEST_URL + '&aet=n'});
+    });
+  });
+
   describe('#onCreativeRender', () => {
     beforeEach(() => {
       const doc = env.win.document;

diff --git a/extensions/amp-ad-network-doubleclick-impl/0.1/amp-ad-network-doubleclick-impl.js b/extensions/amp-ad-network-doubleclick-impl/0.1/amp-ad-network-doubleclick-impl.js
@@ -48,6 +48,7 @@ import {
   QQID_HEADER,
   getEnclosingContainerTypes,
   ValidAdContainerTypes,
+  maybeAppendErrorParameter,
 } from '../../../ads/google/a4a/utils';
 import {getMultiSizeDimensions} from '../../../ads/google/utils';
 import {
@@ -473,6 +474,12 @@ export class AmpAdNetworkDoubleclickImpl extends AmpA4A {
       });
   }
 
+  /** @override */
+  onNetworkFailure(error, adUrl) {
+    dev().info(TAG, 'network error, attempt adding of error parameter', error);
+    return {adUrl: maybeAppendErrorParameter(adUrl, 'n')};
+  }
+
   /** @override */
   extractSize(responseHeaders) {
     setGoogleLifecycleVarsFromHeaders(responseHeaders, this.lifecycleReporter_);

diff --git a/extensions/amp-ad-network-doubleclick-impl/0.1/test/test-amp-ad-network-doubleclick-impl.js b/extensions/amp-ad-network-doubleclick-impl/0.1/test/test-amp-ad-network-doubleclick-impl.js
@@ -200,6 +200,29 @@ describes.sandboxed('amp-ad-network-doubleclick-impl', {}, () => {
     });
   });
 
+  describe('#onNetworkFailure', () => {
+
+    beforeEach(() => {
+      return createIframePromise().then(fixture => {
+        setupForAdTesting(fixture);
+        const doc = fixture.doc;
+        doc.win = window;
+        element = createElementWithAttributes(doc, 'amp-ad', {
+          'width': '200',
+          'height': '50',
+          'type': 'doubleclick',
+        });
+        impl = new AmpAdNetworkDoubleclickImpl(element);
+      });
+    });
+
+    it('should append error parameter', () => {
+      const TEST_URL = 'https://somenetwork.com/foo?hello=world&a=b';
+      expect(impl.onNetworkFailure(new Error('xhr failure'), TEST_URL))
+          .to.jsonEqual({adUrl: TEST_URL + '&aet=n'});
+    });
+  });
+
   describe('#onCreativeRender', () => {
     beforeEach(() => {
       return createIframePromise().then(fixture => {