🚨 Error: The string did not match the expected pattern.

[ampprojectbot]

Details

Error report: link
First seen: Apr 8, 2020
Frequency: ~ 5,572/day

Stacktrace

Error: The string did not match the expected pattern.
    at res (extensions/amp-consent/0.1/amp-consent.js:613:20)

Notes

@micajuine-ho modified extensions/amp-consent/0.1/amp-consent.js:610-622 in #29696 (Sep 4, 2020)

Seen in:

• 09-11 Control (2034+2)
• 09-11 Nightly-Control (2034+2)
• 09-11 Stable (2034+2)
• 09-19 Beta (0410)
• 09-25 Nightly (2320)
• +6 more

/cc @ampproject/release-on-duty

[rcebulko]

Sample URL: https://www.express.co.uk/showbiz/tv-radio/1340919/Doctor-Who-Jodie-Whittaker-danger-Master-alive-revenge-Missy-mistake-BBC-video/amp

Appears to occur only on Safari. This instance only seems to register for express.co.uk, but there are many similar duplicate error reports on other domains. A cursory investigation suggests this may be CORS-related, but I'm not certain. It's also possible this is a publisher configuration issue, in which case we should try to catch it and report it via the user().error() helper.

I see amp-consent is owned by @ampproject/wg-ads-reviewers . Think you could take a look?

[calebcordry]

I think @micajuine-ho has been working on this recently

[micajuine-ho]

The error is due to this: ampproject/amphtml#29655

The vendor passing back an invalid JSON, which is unable to be parsed. I added the try/catch block with the userAssert.

@rcebulko Is userAssert() the same as user().error()?

[rcebulko]

They should be roughly equivalent; in fact, instead of userAssert(false, msg) it's probably preferable to call user().error(msg) directly, but that's not the issue here.

// Within getConsentRemote in amp-consent.js
return ampdoc.whenFirstVisible().then(() => {
  return expandConsentEndpointUrl(this.element, resolvedHref).then(
    (expandedHref) => {
      return xhrService.fetchJson(expandedHref, init).then((res) => {
        try {
          return xhrService.xssiJson(
            res,
            this.consentConfig_['xssiPrefix']
          );
        } catch (e) {
          userAssert(
            false,
            'Could not parse the `checkConsentHref` response.'
          );
        }
      });
    }
  );
});

I suspect what is happening is that the try/catch added in the above fix isn't actually catching an error (which explains why the error message you added hasn't shown up in any error logs). When the line return xhrService.xxiJson runs, it ends up returning res.json(), which itself returns a Promise. It's that Promise which is rejected, but it's still successfully returned through the promise chain, never actually triggering an error within the try/catch block that was added. Some of this confusion might be alleviated if we migrate some of this to async/await, which I believe we support now.

The above code could be collapsed to the following, provided the function it lives in is declared async

await ampdoc.whenFirstVisible();
const expandedHref = await expandConsentEndpointUrl(this.element, resolvedHref);
const res = await xhrService.fetchJson(expandedHref, init);
try {
  return await xhrService.xssiJson(res, this.consentConfig_['xssiPrefix']);
} catch (e) {
  user().error('Could not parse the `checkConsentHref` response.');
}

Promises, especially nested ones and cases involving error handling, are unfortunately fairly easy to miss in cases like this. The majority of error reports we see logged in production come, at some level, from race conditions or error handling around Promises. The best we can do is progressively shift to the more modern async/await where appropriate.

[micajuine-ho]

Ahh I see. Thanks for the thorough explanation Ryan. I agree that moving to async/await is the preferred approach here. I will look into making this change.

[rcebulko]

Update: apparently we aren't able to use async/await in AMP code yet, so .catch will have to do =/

[micajuine-ho]

@rcebulko Hmm, should we surround the res.json() (in xhr-impl#xssiJson()) in a try catch block instead?

[rcebulko]

Doing that would reproduce the same issue a level lower: res.json() would successfully return a Promise, and that promise would reject with the error. At some point in the chain (probably where the current try/catch is), we need to .catch and handle the error there

[samouri]

I agree with @rcebulko's analysis. try/catch cant be used to catch rejecting promises unless we use async/await.

If you don't need to distinguish between the fetch and json parsing error, you can throw the catch block at the end.

// Within getConsentRemote in amp-consent.js
return ampdoc.whenFirstVisible().then(() => {
  return expandConsentEndpointUrl(this.element, resolvedHref).then(
    (expandedHref) => {
      return xhrService
        .fetchJson(expandedHref, init)
        .then((res) =>
          xhrService.xssiJson(res, this.consentConfig_["xssiPrefix"])
        )
        .catch((e) => {
          userAssert(false, "Could not parse the `checkConsentHref` response.");
        });
    }
  );
});

Nit: userAssert(false, ... seems strange

[rcebulko]

We can at least clean up some of the confusion from nesting here if we take advantage of promise chaining:

// Within getConsentRemote in amp-consent.js
return ampdoc.whenFirstVisible()
  .then(() => expandConsentEndpointUrl(this.element, resolvedHref))
  .then((expandedHref) => xhrService.fetchJson(expandedHref, init))
  .then((res) =>
    xhrService.xssiJson(res, this.consentConfig_["xssiPrefix"])
      .catch((e) => {
        user().error("Could not parse the `checkConsentHref` response.");
      })
  );

[rcebulko]

Thanks for fixing this up!