Skip to content

Commit 0c8eaba

Browse files
authored
Bugfix: Assume that if AMP_CONTEXT_DATA is an object that it is an instance of context (ampproject#40267)
The setupMetadata_ Object is currently passed in a context object from ads safeframe code which causes a parsing error that is silently caught by the wrapping top level try/catch in https://github.com/ampproject/amphtml/blob/6a610ba7734d8bfced751644876e9f72e9108725/3p/ampcontext-lib.js#L19-L27
1 parent 6a610ba commit 0c8eaba

File tree

2 files changed

+32
-6
lines changed

2 files changed

+32
-6
lines changed

3p/ampcontext.js

Lines changed: 16 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -326,7 +326,7 @@ export class AbstractAmpContext {
326326
/**
327327
* Parse the metadata attributes from the name and add them to
328328
* the class instance.
329-
* @param {!Object|string} data
329+
* @param {string} data
330330
* @private
331331
*/
332332
setupMetadata_(data) {
@@ -346,6 +346,17 @@ export class AbstractAmpContext {
346346
delete this.data['_context'];
347347
}
348348

349+
this.setupMetadataFromContext_(context);
350+
351+
this.embedType_ = dataObject.type || null;
352+
}
353+
354+
/**
355+
* Set the metadata attributes from the "context" instance directly.
356+
* @param {!Object} context
357+
* @private
358+
*/
359+
setupMetadataFromContext_(context) {
349360
this.canary = context.canary;
350361
this.canonicalUrl = context.canonicalUrl;
351362
this.clientId = context.clientId;
@@ -367,8 +378,6 @@ export class AbstractAmpContext {
367378
this.sourceUrl = context.sourceUrl;
368379
this.startTime = context.startTime;
369380
this.tagName = context.tagName;
370-
371-
this.embedType_ = dataObject.type || null;
372381
}
373382

374383
/**
@@ -403,8 +412,11 @@ export class AbstractAmpContext {
403412
} else if (this.win_.AMP_CONTEXT_DATA) {
404413
if (typeof this.win_.AMP_CONTEXT_DATA == 'string') {
405414
this.sentinel = this.win_.AMP_CONTEXT_DATA;
415+
// If AMP_CONTEXT_DATA is an Object, Assume that it is the Context Object
416+
// and not inside the attributes._context which is the structure
417+
// parsed from "name" on other instances.
406418
} else if (isObject(this.win_.AMP_CONTEXT_DATA)) {
407-
this.setupMetadata_(this.win_.AMP_CONTEXT_DATA);
419+
this.setupMetadataFromContext_(this.win_.AMP_CONTEXT_DATA);
408420
}
409421
} else {
410422
this.setupMetadata_(this.win_.name);

test/unit/test-amp-context.js

Lines changed: 16 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -103,8 +103,8 @@ describes.sandboxed('3p ampcontext.js', {}, (env) => {
103103
expect(context.referrer).to.equal('baz.net');
104104
});
105105

106-
it('should add metadata to window.context using window var.', () => {
107-
win.AMP_CONTEXT_DATA = generateAttributes();
106+
it('should add metadata to window.context using window global.', () => {
107+
win.AMP_CONTEXT_DATA = generateAMPConfigObject();
108108
const context = new AmpContext(win);
109109
expect(context.location).to.deep.equal({
110110
'hash': '',
@@ -393,6 +393,20 @@ describes.sandboxed('3p ampcontext.js', {}, (env) => {
393393
});
394394
});
395395

396+
function generateAMPConfigObject() {
397+
return {
398+
location: {
399+
href: 'https://foo.com/a?b=c',
400+
},
401+
canonicalUrl: 'https://bar.com',
402+
pageViewId: '1',
403+
pageViewId64: 'abcdef',
404+
sentinel: '1-291921',
405+
startTime: 0,
406+
referrer: 'baz.net',
407+
};
408+
}
409+
396410
function generateSerializedAttributes(opt_sentinel) {
397411
return JSON.stringify(generateAttributes(opt_sentinel));
398412
}

0 commit comments

Comments
 (0)