Report user-error from 3p iframe #11002

tiendt · 2017-08-19T05:45:21Z

lannka · 2017-08-20T03:03:02Z

examples/ads.amp.html

@@ -208,7 +209,8 @@
      type="a9"
      data-aax_size="300x250"
      data-aax_pubname="test123"
-      data-aax_src="302">
+      data-aax_src="302"


pls have a separate example page

lannka · 2017-08-20T03:03:19Z

examples/ads.amp.html

+    <script type="application/json">
+      {
+        "requests": {
+          "user-error": "http://localhost:8000/ampadtest"


include error type and message in the request

jridgewell · 2017-08-22T18:45:56Z

3p/ampcontext.js

+   */
+  errorReport() {
+    this.win_.addEventListener('error', event => {
+      if (!!event.error) {


Unnecessary !!

lannka · 2017-08-24T17:43:40Z

examples/3p.error.amp.html

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


why is this file?
for a testing example, you can put in analytics-user-error.amp.html once that PR is merged

lannka · 2017-08-24T17:45:30Z

@zhouyx I will leave the rest to you

@zhouyx

@zhouyx I will leave the rest to you

zhouyx · 2017-08-28T18:36:10Z

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

@@ -206,6 +207,11 @@ export class AmpAdXOriginIframeHandler {
      this.sendEmbedInfo_(this.baseInstance_.isInViewport());
    }));

+    this.unlisteners_.push(listenFor(this.iframe, 'user-error',


nit: Use MessageType.USER_ERROR.

zhouyx · 2017-08-28T18:37:38Z

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

+    this.unlisteners_.push(listenFor(this.iframe, 'user-error',
+        data => {
+          this.userErrorForAnalytics(data['error'], data['message']);
+        }, true, true));


opt_includingNestedWindows set to true if a messages from nested frames should also be accepted.

I don't think you want to include messages from nested frames?

What do you mean and why so?

please see: https://github.com/ampproject/amphtml/blob/master/src/iframe-helper.js#L244

zhouyx · 2017-08-28T18:41:45Z

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

+   * @param {!Error} error
+   * @param {string} message
+   */
+  userErrorForAnalytics(error, message) {


who is calling this function?

in the listenFor('user-error), line 212

should be private function

zhouyx · 2017-08-28T19:23:32Z

3p/ampcontext.js

+      if (!!event.error) {
+        this.client_.sendMessage(MessageType.USER_ERROR, dict({
+          'error': event.error,
+          'message': event.error.message,


is event the ErrorEvent? if so should it be event.message here?

I was thinking event.error returns error so I can get message from that error

I honestly don't know. Can you make sure?

zhouyx · 2017-08-28T19:26:45Z

3p/ampcontext.js

@@ -104,6 +104,7 @@ export class AbstractAmpContext {
    this.client_ = new IframeMessagingClient(win);
    this.client_.setHostWindow(this.getHostWindow_());
    this.client_.setSentinel(dev().assertString(this.sentinel));
+    this.errorReport();


nit: reportError()?

zhouyx · 2017-08-30T18:30:27Z

3p/ampcontext.js

+  report3pError_() {
+    this.win_.onerror = function(message) {
+      if (message) {
+        this.client_.sendMessage(MessageType.USER_ERROR, dict({


the code here won't work. this changes in function. You can either use () => {} or bind this to the function.

zhouyx

Looks good in general. Few more comments : )

zhouyx · 2017-08-30T18:35:15Z

examples/analytics-error-reporting.amp.html

 <amp-youtube width="480" height="270"
             data-param-autoplay
             data-videoid="dQw4w9WgXcQ">
 </amp-youtube>

-<amp-analytics type="googleanalytics">
+<h1>These create 3P error</h1>
+<h2>A9 fake 3p error</h2>


I would prefer throw fake error in our fake ping ad. But I am ok with using these two, if they return use error.

yeah these return user error. I can add more errors if needed?

I am ok with this

zhouyx · 2017-08-30T18:37:35Z

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

+   * @private
+   */
+  userErrorForAnalytics_(message) {
+    const e = new Error();


Can we check message is a string here. Just want to be safe.
Then do e = new Error(message);

zhouyx · 2017-08-30T18:40:13Z

test/functional/test-amp-context.js

+    };
+    windowMessageHandler(message);
+
+    window.onerror = function(message, source, lineno, colno, error) {


I see we use describe here, so please do not rewrite window function. This can potentially break other unrelated tests that depend on window.onerror.
You can mock it in win, like what we did for addEventListener.
Also this test here simply fake everything that we want to test. Can we use the try/catch we have before?

Because for testing onerror, it will need to manually call the handler instead of how we dealt with addEventListener(). cc @cramforce

I see. I am however still opposing overwriting window.onerror here, it would super hard to debug later. We can 1) rewrite the test to use describes or 2) make the onerror function a method and test that method instead.

zhouyx · 2017-08-30T18:41:13Z

test/functional/test-amp-context.js

+      expect(message).to.equal('message');
+      context.report3pError_();
+      expect(windowPostMessageSpy).to.be.called;
+      expect(windowPostMessageSpy).to.be.calledWith(


please use .jsonObject as we agreed.

I just took a look at the code, postMessage would call to serializeMessage which will JSON.stringify(message). Therefore I think checking the value as string here is expected.

you can use deserializeMessage before testing. I know the test works fine, but it would be really hard for people to fix, if it fails for any reason.

zhouyx · 2017-08-30T18:42:21Z

src/3p-frame-messaging.js

@@ -51,6 +51,9 @@ export const MessageType = {
  // For amp-analytics' iframe-transport
  SEND_IFRAME_TRANSPORT_EVENTS: 'send-iframe-transport-events',
  IFRAME_TRANSPORT_EVENTS: 'iframe-transport-events',
+
+  // For user-error
+  USER_ERROR: 'user-error',


Can we rename this to USER_ERROR_IN_IFRAME? I find that to be more clear. Thanks

zhouyx · 2017-08-30T18:44:43Z

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

+    this.unlisteners_.push(listenFor(this.iframe, MessageType.USER_ERROR,
+        data => {
+          this.userErrorForAnalytics_(data['message']);
+        }, true, true));


please see my previous comments. I really don't think we should listen to message from nested iframe here. Also i don't think nested iframe will send us error message.

I thought when we talked offline you said you'd prefer to send message around instead of error. What's your suggestion to improve this?

Please see. #11002 (comment)
Let me know if there's any question

This reverts commit 5191442.

cramforce · 2017-08-30T21:18:26Z

3p/ampcontext.js

+    this.win_.onerror = message => {
+      if (message) {
+        this.client_.sendMessage(MessageType.USER_ERROR_IN_IRAME, dict({
+          'message': message,


Don't you want the stack?

I think I only need the message to be passed through

Curious how can we get the stack here?

The 4th argument (I think) of onerror is the Error object or undefined.

So you can do 'stack': error && error.stack

cramforce · 2017-08-30T21:19:35Z

test/functional/test-amp-context.js

+      throw e;
+    } catch (err) {
+      win.onerror = function(message, source, lineno, colno, error) {
+        expect(error).to.equal(err);


Does this actually run? How?

zhouyx · 2017-08-30T22:50:12Z

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

@@ -523,6 +529,18 @@ export class AmpAdXOriginIframeHandler {
      this.intersectionObserver_.fire();
    }
  }
+
+  /**
+   * @param {!string} message


nit: {string}

zhouyx · 2017-08-30T22:51:22Z

src/3p-frame-messaging.js

@@ -51,6 +51,9 @@ export const MessageType = {
  // For amp-analytics' iframe-transport
  SEND_IFRAME_TRANSPORT_EVENTS: 'send-iframe-transport-events',
  IFRAME_TRANSPORT_EVENTS: 'iframe-transport-events',
+
+  // For user-error
+  USER_ERROR_IN_IRAME: 'user-error',


nit : USER_ERROR_IN_IFRAME: 'user-error-in-iframe',

zhouyx · 2017-08-30T22:53:49Z

3p/ampcontext.js

+          'message': message,
+        }));
+      }
+      return false;


just curious, do we have to return false here?

if we don't return false then it would be undefined, which is false. But i think we should explicitly return false to make it readable

sounds good

zhouyx · 2017-08-31T01:58:08Z

3p/ampcontext.js

+    this.win_.onerror = message => {
+      if (message) {
+        this.client_.sendMessage(MessageType.USER_ERROR_IN_IRAME, dict({
+          'message': message,


Curious how can we get the stack here?

zhouyx · 2017-08-31T01:58:19Z

3p/ampcontext.js

+          'message': message,
+        }));
+      }
+      return false;


sounds good

zhouyx · 2017-08-31T01:58:44Z

examples/analytics-error-reporting.amp.html

 <amp-youtube width="480" height="270"
             data-param-autoplay
             data-videoid="dQw4w9WgXcQ">
 </amp-youtube>

-<amp-analytics type="googleanalytics">
+<h1>These create 3P error</h1>
+<h2>A9 fake 3p error</h2>


I am ok with this

zhouyx · 2017-08-31T02:06:37Z

src/3p-frame-messaging.js

@@ -51,6 +51,9 @@ export const MessageType = {
  // For amp-analytics' iframe-transport
  SEND_IFRAME_TRANSPORT_EVENTS: 'send-iframe-transport-events',
  IFRAME_TRANSPORT_EVENTS: 'iframe-transport-events',
+
+  // For user-error-in-iframe
+  USER_ERROR_IN_IRAME: 'user-error-in-iframe',


nit: USER_ERROR_IN_IFRAME

zhouyx

LGTM

zhouyx · 2017-08-31T16:56:51Z

test/functional/test-amp-context.js

@@ -32,8 +32,6 @@ describe('3p ampcontext.js', () => {
    windowPostMessageSpy = sandbox.spy();
    win = {
      addEventListener: (eventType, handlerFn) => {
-        expect(eventType).to.equal('message');


don't think you should remove Alan's test here though.

zhouyx · 2017-08-31T16:56:53Z

test/functional/test-amp-context.js

+    win.name = generateSerializedAttributes();
+    const context = new AmpContext(win);
+    expect(context).to.be.ok;
+    context.report3pError_();


Same question again, the constructor already called report3pError_() why we need to call it here?

i needed it to test addEventListener call before, but forgot to remove. Will change now.

zhouyx · 2017-08-31T16:57:47Z

test/functional/test-amp-context.js

@@ -61,6 +59,24 @@ describe('3p ampcontext.js', () => {
    windowMessageHandler = undefined;
  });

+  it('should send message when report3pError_()', () => {


nit: 'should send error message with report3pError_'

cramforce

LGTM
See comments about stack traces (if appliacable).

googlebot added the cla: yes label Aug 19, 2017

tiendt requested review from lannka and zhouyx August 19, 2017 05:45

tiendt self-assigned this Aug 19, 2017

lannka previously requested changes Aug 20, 2017

View reviewed changes

tiendt requested review from camelburrito, cramforce, dvoytenko and jridgewell as code owners August 21, 2017 23:25

jridgewell reviewed Aug 22, 2017

View reviewed changes

tiendt added 11 commits August 23, 2017 04:19

errorReport, introduce error

e5e4f9a

3p ampcontext, postMessage, report to analytics

e353f97

make errorReport public

927f102

call errorReport in ampcontext constructor

f3fc38b

add seperate html test file

b6e3554

rm commented code

6d07552

unit test user-error API

6c7bd91

test

ff1dd4b

test for errorReport() and addEventListener in ampcontext

c0e9aed

fix test ampcontext

0f7fd6f

resolve conflict

c199879

lannka reviewed Aug 24, 2017

View reviewed changes

Merge branch 'master' of github.com:tiendt/amphtml into inside-3p

8f32f29

tiendt force-pushed the inside-3p branch from a3b145b to 8f32f29 Compare August 25, 2017 20:08

tiendt added 3 commits August 28, 2017 10:50

fix test-ampcontext, windowMessageHandler

d34fc80

Merge branch 'master' of github.com:tiendt/amphtml into inside-3p

2856ed0

uncomment

e72b42c

zhouyx suggested changes Aug 28, 2017

View reviewed changes

tiendt added 4 commits August 29, 2017 17:56

win.onerror

9d692bd

fix test-ampcontext

890d600

fix test ad-xorigin

e0b3888

revert ads.amp.html

aea38a1

zhouyx reviewed Aug 30, 2017

View reviewed changes

fix this.client_

25be230

zhouyx suggested changes Aug 30, 2017

View reviewed changes

tiendt added 7 commits August 30, 2017 11:51

user-error-in-iframe

9fa3be0

check message string

6e9da24

check message string

5191442

Revert "check message string"

6c7a6ca

This reverts commit 5191442.

window->win

284b510

try catch

758d92c

fix lint

9631184

cramforce reviewed Aug 30, 2017

View reviewed changes

zhouyx reviewed Aug 30, 2017

View reviewed changes

tiendt added 4 commits August 30, 2017 17:09

fix test

bf508da

user-error-in-iframe

dff4be9

string

a02bd93

json

56ce0c8

zhouyx reviewed Aug 31, 2017

View reviewed changes

tiendt added 3 commits August 30, 2017 19:40

use serializeMessage

c7554e7

rename to IFRAME

5d556dc

npm install

5560faa

tiendt mentioned this pull request Aug 31, 2017

Enable different sampling strategies for 1P and 3P errors #11151

Closed

zhouyx approved these changes Aug 31, 2017

View reviewed changes

cramforce approved these changes Aug 31, 2017

View reviewed changes

rm report3perror call in tetest

c00ac84

cramforce merged commit dbd6399 into ampproject:master Aug 31, 2017

tiendt mentioned this pull request Sep 1, 2017

Add todos #11164

Closed

Report user-error from 3p iframe #11002

Report user-error from 3p iframe #11002

Conversation

tiendt commented Aug 19, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lannka commented Aug 24, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zhouyx left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zhouyx left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cramforce left a comment

Choose a reason for hiding this comment