Skip to content

Commit

Permalink
Changed cid-impl to handle a racy condition.
Browse files Browse the repository at this point in the history
When the cid is requested twice in a row and no cookie is present, the
cid in the first call is different from the seconed one.
  • Loading branch information
avimehta committed Apr 15, 2016
1 parent 65c4b02 commit 8edb63a
Show file tree
Hide file tree
Showing 2 changed files with 42 additions and 0 deletions.
14 changes: 14 additions & 0 deletions src/service/cid-impl.js
Expand Up @@ -73,6 +73,13 @@ class Cid {
* @private {?string}
*/
this.baseCid_ = null;

/**
* Cache to store external cids. Scope is used as the key and cookie value
* is the value.
* @private {!Object.<string, string>}
*/
this.externalCidCache_ = Object.create(null);
}

/**
Expand Down Expand Up @@ -169,6 +176,10 @@ function getOrCreateCookie(cid, getCidStruct, persistenceConsent) {
return Promise.resolve(null);
}

if (cid.externalCidCache_[scope]) {
return Promise.resolve(cid.externalCidCache_[scope]);
}

if (existingCookie) {
// If we created the cookie, update it's expiration time.
if (/^amp-/.test(existingCookie)) {
Expand All @@ -181,6 +192,7 @@ function getOrCreateCookie(cid, getCidStruct, persistenceConsent) {
// the value whether we created it.
const newCookie = 'amp-' + cid.sha384Base64_(getEntropy(win));

cid.externalCidCache_[scope] = newCookie;
// Store it as a cookie based on the persistence consent.
persistenceConsent.then(() => {
// The initial CID generation is inherently racy. First one that gets
Expand Down Expand Up @@ -247,6 +259,8 @@ function getBaseCid(cid, persistenceConsent) {
// We need to make a new one.
const seed = getEntropy(win);
const newVal = cid.sha384Base64_(seed);

cid.baseCid_ = newVal;
// Storing the value may require consent. We wait for the respective
// promise.
persistenceConsent.then(() => {
Expand Down
28 changes: 28 additions & 0 deletions test/functional/test-cid.js
Expand Up @@ -382,6 +382,34 @@ describe('cid', () => {
});
});

it('should return same value for multiple calls on non-proxied urls', () => {
fakeWin.location.href = 'https://abc.org/foo/?f=0';
fakeWin.location.hostname = 'foo.abc.org';
const cid1 = cid.get({scope: 'cookie', createCookieIfNotPresent: true},
hasConsent);
const cid2 = cid.get({scope: 'cookie', createCookieIfNotPresent: true},
hasConsent);
return cid1.then(c1 => {
return cid2.then(c2 => {
expect(c1).to.equal(c2);
});
});
});

it('should return same value for multiple calls on proxied urls', () => {
fakeWin.location.href = 'https://cdn.ampproject.org/v/abc.org/foo/?f=0';
fakeWin.location.hostname = 'cdn.ampproject.org';
const cid1 = cid.get({scope: 'cookie', createCookieIfNotPresent: true},
hasConsent);
const cid2 = cid.get({scope: 'cookie', createCookieIfNotPresent: true},
hasConsent);
return cid1.then(c1 => {
return cid2.then(c2 => {
expect(c1).to.equal(c2);
});
});
});

function compare(externalCidScope, compareValue) {
return cid.get(externalCidScope, hasConsent).then(c => {
expect(c).to.equal(compareValue);
Expand Down

0 comments on commit 8edb63a

Please sign in to comment.