Pass context down for a4a ad rendering to allow window.context to function by bradfrizzell · Pull Request #7132 · ampproject/amphtml

bradfrizzell · 2017-01-20T22:00:02Z

No description provided.

taymonbeal · 2017-01-20T22:13:06Z

examples/amp-static-image-dfp.html

@@ -0,0 +1,25 @@
+<!doctype html>


Is this file supposed to be part of this PR?

no this shouldn't be. removed

taymonbeal · 2017-01-20T22:13:51Z

extensions/amp-a4a/0.1/amp-a4a.js

        }, SHARED_IFRAME_PROPERTIES));
+    const sentinel = generateSentinel(window);
+    const metadata = getContextMetadata(window, iframe, sentinel);
+    iframe.setAttribute('name', encodeURIComponent(JSON.stringify(metadata)));


Why encodeURIComponent?

because this is getting written to the name attribute in the html, this was to protect against if anything in the metadata has a closing quote

Why would it matter if something had a closing quote?

My concern is an XSS injection, because we're writing this data into the page that could contain in the JSON a bunch of stuff that we're not controlling. So I meant that they could close the quote on the name attribute, and then like open a script tag or something. Like if the json object was:

{"foo":"<script>alert("foo");</script>}

that would be written right in the page.

DOM manipulation doesn't work that way. iframe.setAttribute('name', someString) will never change anything other than the value of iframe.name no matter what's in someString.

ok. if there's no risk then I guess we don't need encodeURIComponent

taymonbeal · 2017-01-20T22:14:10Z

extensions/amp-a4a/0.1/amp-a4a.js

            'src': srcPath,
            'name': nameData,
          }, SHARED_IFRAME_PROPERTIES));
+      if (method == XORIGIN_MODE.NAMEFRAME){


Add a space before the brace.

taymonbeal · 2017-01-20T22:18:54Z

extensions/amp-a4a/0.1/amp-a4a.js

+      if (method == XORIGIN_MODE.NAMEFRAME){
+        const sentinel = generateSentinel(window);
+        const metadata = getContextMetadata(window, iframe, sentinel);
+        let name = JSON.stringify({creative, _context: metadata._context});


const, since it doesn't change. Also, I'd prefer for these string keys to be quoted; even if the actual Closure rules (which I don't fully understand) don't allow renaming of fields under these circumstances, it's better for no one reading the code to have to wonder about that.

done, changed it so both creative and _context are the name of the local variable, so they variables are the keys and we don't need the strings anyways

taymonbeal · 2017-01-20T22:20:01Z

src/attributes.js

+ * @param {!Window} parentWindow
+ * @param {!Element} element
+ * @return {!Object} Contains
+ *       precedence over the data- attributes.


Did something get incorrectly copy-pasted here?

yes that's screwed up, but this file actually also should not be in this PR (although it is needed for this pr). #7129 is where I am creating it, and this PR should block on that one's merging

bradfrizzell · 2017-01-24T17:38:47Z

Upon talking to keith, we are going to ignore the safeframe case for now and just focus on getting this working for nameframe and direct frame

bradfrizzell · 2017-01-24T17:54:49Z

blocked by #7129

jridgewell · 2017-01-24T18:13:29Z

extensions/amp-ad/0.1/amp-ad-xorigin-iframe-handler.js

          .then(() => this.noContent_());
    }

+    this.element_.appendChild(this.iframe);


Appending this before setting all the styles could affect painting.

would moving the setStyle line above the append be sufficient?

jridgewell · 2017-01-24T18:13:36Z

extensions/amp-ad/0.1/amp-ad-xorigin-iframe-handler.js

    }

+    this.element_.appendChild(this.iframe);
+    if (opt_isA4A) {


Why was this moved down?

This was moved down because we want to listen for the no content message in the a4a case.

jridgewell · 2017-01-24T18:31:34Z

extensions/amp-a4a/0.1/amp-a4a.js

          }, SHARED_IFRAME_PROPERTIES));
+      if (method == XORIGIN_MODE.NAMEFRAME) {
+        const sentinel = generateSentinel(window);
+        const _context = getContextMetadata(window, iframe, sentinel)._context;


Can we expose this as a method?

sure. I put it in amp-a4a, but maybe this should go in iframe-attributes.js?

lannka

I'm wondering what the status of those various ways of delayed rendering: nameframe, safeframe, cachedcontent.

Do we plan to eventually eliminate them and keep one? Maintaining them is a bit costly.

@keithwrightbos

lannka · 2017-02-01T17:30:57Z

extensions/amp-a4a/0.1/amp-a4a.js

+   */
+  generateSentinelAndContext(iframe) {
+    const sentinel = generateSentinel(window);
+    const context = getContextMetadata(window, iframe, sentinel)._context;


return it directly?

lannka · 2017-02-01T17:34:18Z

extensions/amp-a4a/0.1/amp-a4a.js

+   */
+  generateSentinelAndContext(iframe) {
+    const sentinel = generateSentinel(window);
+    const context = getContextMetadata(window, iframe, sentinel)._context;


looking at the impl of getContextMetadata, it seems weird that we're building an attributes object but end up using on one field of it. can't we just extract that part out as a separate method?

this is actually a mistake, shouldn't be returning ._context, just the main object

lannka · 2017-02-01T17:35:09Z

extensions/amp-ad/0.1/amp-ad-xorigin-iframe-handler.js

+    if (opt_isA4A) {
+      this.element_.appendChild(this.iframe);
+      // A4A writes creative frame directly to page therefore does not expect
+      // post message to unset visibility hidden


not sure what the "post message" is here.

I'm also not sure, this was a holdover comment. I don't think that this postmessage ever happens. I'm going to delete this comment

ah, i think it is referring to render-start and no-content events. Please update the doc.

lannka · 2017-02-01T17:45:07Z

extensions/amp-ad/0.1/amp-ad-xorigin-iframe-handler.js

-            setStyle(this.iframe, 'visibility', '');
-            if (this.baseInstance_.emitLifecycleEvent) {
-              this.baseInstance_.emitLifecycleEvent('adSlotUnhidden');
+    if (opt_isA4A) {


any reason to move this code? A4A creative in friendly iframe will not send 'render-start' event, so there's no need to listen to it.

because we want to register listening for 'no-content'

keithwrightbos · 2017-02-01T17:56:09Z

Regarding xdomain write methods (cached, safeframe, nameframe): once nameframe shows same metrics as safeframe, we will no longer use sameframe methodology. We may keep the cached way as currently it shows better metrics than safeframe (this is expected given it requires less browser resources) however my SRA design will necessitate the use of nameframe. If we find that majority of requests use nameframe, we can also remove cached method.

…ied tests

lannka · 2017-02-01T19:21:19Z

3p/ampcontext.js

+   *  @private
+   */
+  getWindowSentinel_() {
+    this.sentinel = this.win_.AMP_SENTINEL || null;


I'm wondering who will be setting window.AMP_SENTINEL

this is to allow a bypass with safeframe if needed, in which case we don't want to write on the sentinel or hash. instead we can write a script into the ad itself that sets this variable

Can we make that more explicit by changing AMP_SENTINEL to another name?

changed to SAFEFRAME_SENTINEL

lannka · 2017-02-01T19:24:15Z

3p/ampcontext.js

-    this.setupMetadata_();
+    this.win_ = win;
+    if (!this.getWindowSentinel_()) {
+      this.setupMetadata_();


can you add some doc to explain in what scenario we don't need to parse the metadata?

jridgewell · 2017-02-01T19:44:53Z

3p/ampcontext.js

+   *  @private
+   */
+  getWindowSentinel_() {
+    this.sentinel = this.win_.AMP_SENTINEL || null;


Can we make that more explicit by changing AMP_SENTINEL to another name?

jridgewell · 2017-02-01T19:46:24Z

extensions/amp-ad/0.1/amp-ad-xorigin-iframe-handler.js

+    if (opt_isA4A) {
+      this.element_.appendChild(this.iframe);
+      return Promise.resolve();
+    } else {


Unnecessary else.

jridgewell · 2017-02-01T19:46:54Z

extensions/amp-a4a/0.1/amp-a4a.js

            'name': nameData,
          }, SHARED_IFRAME_PROPERTIES));
+      if (method == XORIGIN_MODE.NAMEFRAME) {
+        const attributes = this.generateSentinelAndContext(iframe);


Does this return a context or attributes?

I've been using it somewhat interchangeably, but sentinel and context together are the attributes assigned on the frame. 'context' is referring to the old context that did not include sentinel

what about renaming attributes to metadata, generateSentinelAndContext to generateMetadata?

I could do that, but there are a lot of other places that would have to change. could I do a cleanup refactoring PR for that? really need to finish this asap

A TODO works for me :-)

This reverts commit a61ddf8.

lannka · 2017-02-02T00:03:40Z

src/iframe-helper.js

    const contentWindow = we.frame.contentWindow;
    if (!contentWindow) {
      setTimeout(dropListenSentinel, 0, listenSentinel);
-    } else if (sentinel === 'amp') {


so, we end up removing this whole block for "sentinel==='amp''.
why was it affecting the A4A iframes? I don't think they have sentinel of 'amp', do they?

will this break our non-3p iframes which has sentinel = 'amp'?

bradfrizzell · 2017-02-02T16:27:01Z

In addition to the gulp tests, I also did manual testing on live pages and my example page to verify that direct frame and nameframe both set up properly and can send/receive postMessages to/from the runtime.

lannka · 2017-02-02T16:27:19Z

3p/ampcontext.js

+    if (typeof this.win_.AMP_CONTEXT_DATA == 'string') {
+      this.sentinel = this.win_.AMP_CONTEXT_DATA;
+    } else {
+      this.setupMetadata_(this.win_.AMP_CONTEXT_DATA);


now this method mutates states, so we should rename it.
And probably you will just want to move line 33 into this method as well.

lannka · 2017-02-02T16:29:53Z

3p/ampcontext.js

  /**
   *  Parse the metadata attributes from the name and add them to
   *  the class instance.
+   *  @param {!Object|!String} contextData


{!Object|string} data

lannka · 2017-02-02T16:31:54Z

3p/ampcontext.js

-      this.referrer = context.referrer;
-    } catch (err) {
+      data = JSON.parse(decodeURI(data));
+    } catch (err) {}


you can use tryParseJson() in json.js

lannka · 2017-02-02T16:32:52Z

3p/ampcontext.js

+    } catch (err) {}
+
+    if (!data || typeof data != 'object') {
      user().error('AMPCONTEXT', '- Could not parse metadata.');


this is more likely a dev error? we actually can just skip it, since we are throwing in the next line.

lannka · 2017-02-02T16:35:07Z

3p/ampcontext.js

+      data = JSON.parse(decodeURI(data));
+    } catch (err) {}
+
+    if (!data || typeof data != 'object') {


no need to check typeof data

lannka · 2017-02-02T16:36:12Z

examples/loads-windowcontext-creative.html

+    <br>
+    <br>
+    <br>
+    <br>


why so many br? can you use a blank div instead?

tdrl

Mostly minor stuff and nits. The only one I'm really concerned about is the comment about overwriting name that's just been set. Otherwise, LGTM.

tdrl · 2017-02-02T14:45:20Z

3p/ampcontext.js

-    this.setupMetadata_();
+    this.win_ = win;
+
+    // if the window sentinel is set directly, that means we are running


Nit: Start with capital letter and end with a period.

tdrl · 2017-02-02T14:48:48Z

3p/ampcontext.js

+  /**
+   *  Checks to see if there is a window variable assigned with the
+   *  sentinel value, sets it, and returns true if so.
+   *  @private


removed return value

tdrl · 2017-02-02T14:54:32Z

3p/ampcontext.js

+
+    // if the window sentinel is set directly, that means we are running
+    // in a situation where we couldn't attach the name attribute before
+    // creating the iframe, so don't try to parse the metadata


Woo -- I'm not sure I can follow all the negatives here. Can you restate this in terms of positives -- when do you set the metadata?

I'm trying to decide if you're setting the sentinel in hasSafeframeSentinel and then overriding in setupMetadata. I don't think so, but I'm too foggy-headed this morning to work through the negatives and cases. :-)

tdrl · 2017-02-02T16:00:45Z

examples/ampcontext-creative.js

@@ -0,0 +1,88 @@
+if (!window.context){
+  // window.context doesn't exist yet, must perform steps to create it


Nit: end with period. (Here and below.)

file deleted

tdrl · 2017-02-02T16:01:02Z

examples/ampcontext-creative.js

+  // before using it
+  console.log("window.context NOT READY");
+
+  // must add listener for the creation of window.context


Nit: Start with capital letter. (Here and below.)

file deleted

tdrl · 2017-02-02T16:38:07Z

extensions/amp-a4a/0.1/test/test-amp-a4a.js

+    expect(attributes._context).to.be.ok;
+    if (!attributes._context.amp3pSentinel) {
+      expect(attributes._context.sentinel).to.be.ok;
+    }


So here you're saying that at least one of these should be present, right? Do you want to ensure that exactly one of them is present?

Perhaps a better way to structure this is to add an argument to verifyNameFrameRender that specifies which you should expect, given the case being tested. Then you can do something like:

function verifyNameFrameRender(element, expectSentinel) { // stuff expect(attributes._context).not.to.contain.all.keys('sentinel', 'amp3pSentinel'); if (expectSentinel) { expect(attributes._context).to.contain.keys('sentinel'); expect(attributes._context.sentinel).to.match(/some_regex/); } else { expect(attributes._context).to.contain.keys('amp3pSentinel'); expect(attributes._context.amp3pSentinel).to.match(/some_regex/); }

The two cases for sentinel naming will collapse down very soon, so I didn't want to modify the other tests too much. I can add additional logic in here as suggested for making sure it doesn't have both keys and that it matches the regex

tdrl · 2017-02-02T16:39:33Z

extensions/amp-a4a/0.1/test/test-amp-a4a.js

+    const attributes = JSON.parse(nameData);
+    expect(attributes).to.be.ok;
+    expect(attributes._context).to.be.ok;
+    if (!attributes._context.amp3pSentinel) {


Ah. You're doing essentially the same test here as above. I suggest factoring the nameData validation out to a common function that you call from both places.

tdrl · 2017-02-02T16:40:30Z

extensions/amp-a4a/0.1/test/test-amp-a4a.js

+
+      it('should be able to create AmpContext', () => {
+        return a4a.layoutCallback().then(() => {
+          const window_ = a4aElement.childNodes[0].contentWindow;


Probably safer to use a querySelector to get reference to the iframe, so that this test doesn't break if we add stuff to or rearrange the DOM structure under the a4a element.

tdrl · 2017-02-02T16:40:42Z

extensions/amp-a4a/0.1/test/test-amp-a4a.js


+      it('should be able to create AmpContext', () => {
+        return a4a.layoutCallback().then(() => {
+          const window_ = a4aElement.childNodes[0].contentWindow;


tdrl · 2017-02-02T16:43:03Z

extensions/amp-ad/0.1/amp-ad-xorigin-iframe-handler.js

          .then(() => this.noContent_());
    }

+    if (opt_isA4A) {


Why did you move this down here? Does it depend on the listeners having already been set up? If so, should probably add a note about that dependency. Also, seems worth keeping the original comment -- I don't think it's deprecated. (?)

because we want to listen for nocontent message for a4a. I'll add comment back in

lannka

LGTM, some minor comments.
I didn't take close look at the tests, which Terran is right now on. Please address them as well.

lannka · 2017-02-02T16:52:25Z

3p/ampcontext.js

+    } else {
+      this.setupMetadata_(this.win_.AMP_CONTEXT_DATA);
+    }
+    return !!this.sentinel;


is return still needed?

lannka · 2017-02-02T16:52:41Z

examples/loads-windowcontext-creative.html

+            src="http://localhost:8000/examples/ampcontext-creative.html">
+    </amp-ad>
+    <div>
+


if all you want is space, you can do it via CSS. not sure if empty lines here would work

tdrl · 2017-02-02T18:48:55Z

examples/ampcontext-creative.html

+
+      function intersectionCallback(payload){
+        changes = payload.changes;
+        // Step 4: Do something with the intersection updates!


Nit: Still need to do something with "Step 4", b/c there's no 1..3 in this file.

tdrl

LGTM. Thanks!

…ction (ampproject#7132) * Created attributes.js * Added context to direct frame rendering * Added context to rendering via nameframe * Removed unneeded files * Fixed lint and naming * Don't need to uriencode * Modified tests * Created generateSentinelAndContext and moved setStyle * Fixed test breakages, handled sentinel name change experiment * Addressed comments; ampContext now checks for window var; added/modified tests * Added docstring * Addressed comments * Added test * Attach data-amp-3p-sentinel attribute to frame * Fix a4a frames not having messages processed correctly * Added examples for manual testing * Addressed comments' * Revert "Addressed comments'" This reverts commit a61ddf8. * Changed name of function to hasSafeframeSentinel: * Added todo * Updated docstring and added todo * Fixed getListenForEvents * Collapse if block cases * Undid change to getListenForEvents * Fixed nameframe to keep context data on name * Broke out isAmpMessage function to use for parsing * Handle different ways of setting up AmpContext * Make test page take up multiple viewports * Fixed lint mistakes * Inlined ampcontext-creative.js into html * Addressed comments * Addressed comments * Add comments to explain setting of name on nameframe * removed comment

bruscantini · 2018-02-16T00:50:37Z

Hi all. Can creatives now use this window.AMP_CONTEXT_DATA to get the top level url? An issue I keep having, from a creative standpoint, is trying to get the top level page url when inside a x-domain iframe. I've noticed if the x-domain iframe has domain *.ampproject.net I can get it by parsing the window.name JSON and eventually getting the sourceUrl value. Now, I've come across an occasion where I'm inside a safeframe and have an empty window.name property, but this AMP_CONTEXT_DATA is populated. Is there a consistent way for creatives to get the top page url from AMP? I raised this issue before in #12342.
Thanks for the help guys. :)

bradfrizzell · 2018-02-16T14:08:50Z

hi @bruscantini

So as you've seen, in a safeframe setting, the window name is wiped clean before you have access to it, so GPT safeframe saves the amp context data onto that window var to allow for use later. To save yourself some hassle, you can just use the AmpContext library directly https://github.com/ampproject/amphtml/blob/master/3p/ampcontext.js which handles all the parsing for you. The binary is served as https://3p.ampproject.net/1518441587106/ampcontext-v0.js

bradfrizzell assigned jridgewell, taymonbeal and lannka Jan 20, 2017

taymonbeal suggested changes Jan 20, 2017

View reviewed changes

bradfrizzell mentioned this pull request Jan 23, 2017

Created attributes.js #7129

Merged

bradfrizzell changed the title ~~WIP - Pass context down for a4a ad rendering to allow window.context to function~~ Pass context down for a4a ad rendering to allow window.context to function Jan 24, 2017

bradfrizzell added the WIP label Jan 24, 2017

jridgewell reviewed Jan 24, 2017

View reviewed changes

bradfrizzell removed the WIP label Jan 25, 2017

bradfrizzell assigned tdrl Feb 1, 2017

bradfrizzell added 9 commits February 1, 2017 12:26

Created attributes.js

5232c76

Added context to direct frame rendering

b4ff522

Added context to rendering via nameframe

e256479

Removed unneeded files

97787fd

Fixed lint and naming

45bac87

Don't need to uriencode

d1eca06

Modified tests

7185f8c

Created generateSentinelAndContext and moved setStyle

2c1f1a3

Fixed test breakages, handled sentinel name change experiment

908b077

lannka suggested changes Feb 1, 2017

View reviewed changes

Addressed comments; ampContext now checks for window var; added/modif…

65f66cb

…ied tests

lannka reviewed Feb 1, 2017

View reviewed changes

Added docstring

6804009

jridgewell reviewed Feb 1, 2017

View reviewed changes

bradfrizzell added 6 commits February 1, 2017 18:05

Revert "Addressed comments'"

3aa884d

This reverts commit a61ddf8.

Changed name of function to hasSafeframeSentinel:

7c89bd0

Added todo

14f8b2a

Updated docstring and added todo

80865f7

Fixed getListenForEvents

1b22ac7

Collapse if block cases

a31946a

lannka reviewed Feb 2, 2017

View reviewed changes

bradfrizzell added 7 commits February 1, 2017 19:14

Undid change to getListenForEvents

6f221f9

Fixed nameframe to keep context data on name

898778f

Broke out isAmpMessage function to use for parsing

fe6c705

Handle different ways of setting up AmpContext

1235adc

Make test page take up multiple viewports

6496f2c

Fixed lint mistakes

3a71522

Inlined ampcontext-creative.js into html

6e8bec4

lannka suggested changes Feb 2, 2017

View reviewed changes

tdrl suggested changes Feb 2, 2017

View reviewed changes

Addressed comments

a91f8ae

lannka approved these changes Feb 2, 2017

View reviewed changes

bradfrizzell added 2 commits February 2, 2017 12:32

Addressed comments

78491cc

Add comments to explain setting of name on nameframe

4b732ce

tdrl reviewed Feb 2, 2017

View reviewed changes

tdrl approved these changes Feb 2, 2017

View reviewed changes

removed comment

6a8ced2

lannka merged commit af90bed into ampproject:master Feb 2, 2017

bruscantini unassigned tdrl Feb 16, 2018

		@@ -0,0 +1,88 @@
		if (!window.context){
		// window.context doesn't exist yet, must perform steps to create it

Conversation

bradfrizzell commented Jan 20, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bradfrizzell Jan 23, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bradfrizzell Jan 23, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bradfrizzell commented Jan 24, 2017

Uh oh!

bradfrizzell commented Jan 24, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lannka left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

keithwrightbos commented Feb 1, 2017

Uh oh!

Choose a reason for hiding this comment

bradfrizzell commented Jan 20, 2017 •

edited

Loading

bradfrizzell Jan 23, 2017 •

edited

Loading

bradfrizzell Jan 23, 2017 •

edited

Loading