Exclude errors generated from A4A iframe #10562

tiendt · 2017-07-20T22:15:20Z

User error Reporting:
Filter out errors generated from A4A iframe embed

tiendt · 2017-07-20T22:57:57Z

This is just initial skeleton, I'm hoping to see whether the if else logic in user() is good. The next thing to do is to get ampdoc here (right now there's no win object) @lannka

lannka · 2017-07-20T23:11:53Z

src/log.js

 * @return {!Log}
 */
-export function user() {
+export function user(opt_element) {
 if (logs.user) {


we should now cache 2 instances of user logger.

logs.user logs.userForEmbed

logic will be like:

if (isFromEmbed(opt_element) { if (!logs.userForEmbed) { logs.userForEmbed = ... } return logs.userForEmbed; } else { if (!logs.user) { logs.user = ... } return logs.user; }

lannka · 2017-08-04T02:58:32Z

src/log.js

+/**
+ * @return {boolean} Whether this message was a iframe embed error.
+ */
+function isErrorEmbed(message) {


isUserErrorEmbed

lannka · 2017-08-04T02:59:00Z

src/log.js

+ *
+ * @const {string}
+ */
+export const EMBED = '\u200B\u200B\u200B\u200B';


USER_ERROR_EMBED_SENTINEL

lannka · 2017-08-04T03:02:41Z

src/log.js

+ if (logs.userForEmbed) {
+ return logs.userForEmbed;
+ }
+ logs.userForEmbed = new logConstructor(self, mode => {


seems duplicated logic, can we consolidate with getUserLogger?

lannka · 2017-08-04T03:04:09Z

src/error.js

- if (error && isUserErrorMessage(error.message) && !!win) {
- reportErrorToAnalytics(/** @type {!Error} */(error), win);
+ if (error && !!win) {
+ if (isUserErrorMessage(error.message) && !error.embed) {


seems no nested if needed

lannka · 2017-08-04T03:11:01Z

test/functional/test-log.js

+ });
+
+ it('should return logger for user-error', () => {
+ expect(user().suffix_).to.equal(USER_ERROR_SENTINEL);


you can use

const error = user().createError(); expect(isUserErrorEmbed(error.message)).to.be.false;

also add test to make sure you not not creating extra identical logger

expect(user()).to.equal(user(element1)); expect(user()).to.equal(user(element2));

zhouyx · 2017-08-04T17:53:27Z

extensions/amp-ad/validator-amp-ad.protoascii

@@ -18,8 +18,8 @@
 # Historically, amp-ad/amp-embed javascript were embedded into the main
 # runtime (v0.js), so there are many pages that do not include the amp-ad
 # extension. We want to warn for these.
-# AMP-EMBED requires the amp-ad extension, not amp-embed.
-# Including the amp-ad extension, but not an AMP-AD/AMP-EMBED tag should only
+# AMP-USER_ERROR_EMBED_SENTINEL requires the amp-ad extension, not amp-embed.


<amp-embed> stays <amp-embed>， please revert

zhouyx · 2017-08-04T17:59:00Z

src/error.js

@@ -97,7 +97,7 @@ let detectedJsEngine;
 */
 export function reportErrorForWin(win, error, opt_associatedElement) {
 reportError(error, opt_associatedElement);
- if (error && isUserErrorMessage(error.message) && !!win) {
+ if (error && !!win && isUserErrorMessage(error.message) && !error.embed) {


should we use isUserErrorEmbed(error.message) here? Same reason to why we don't have error.isUserError.

zhouyx · 2017-08-04T17:59:55Z

src/log.js

@@ -210,6 +229,7 @@ export class Log {
 const error = this.error_.apply(this, arguments);
 if (error) {
 error.name = tag || error.name;
+ error.embed = isUserErrorEmbed(error.message);


I thought only error.message will survive, should we still use error.embed?

yeah i just realized, seems like error.embed is not needed anymore

zhouyx · 2017-08-04T18:05:15Z

build-system/tasks/presubmit-checks.js

@@ -341,6 +341,7 @@ var forbiddenTerms = {
 'extensions/amp-access/0.1/amp-access.js',
 'extensions/amp-experiment/0.1/variant.js',
 'extensions/amp-user-notification/0.1/amp-user-notification.js',
+ 'dist.3p/current/integration.js',


what's this for?

@zhouyx When i run presubmit it shows that i'm using cid, so Hongfei suggested to whitelist it.

sorry if there's mis-understanding, but I don't see it cid used in your change. did you try make a new build and run presubmit again?

This reverts commit 0cac14c.

This reverts commit 21402f1.

lannka

LGTM, some minor comments

lannka · 2017-08-06T18:38:00Z

src/log.js

 * @return {boolean} Whether this message was a user error.
 */
 export function isUserErrorMessage(message) {
 return message.indexOf(USER_ERROR_SENTINEL) >= 0;
 }

+/**
+ * @return {boolean} Whether this message was a iframe embed error.


Whether this message was a a user error from an iframe embed

lannka · 2017-08-06T18:40:54Z

src/log.js

 * @return {!Log}
 */
-export function user() {
+export function user(opt_element) {
+ let logger;


seems this local var is not needed

@lannka i think it is because I'd need to assign different value to this var and also use it at the last return call. Lmk what you think!

why don't you just assign to logs.user ?

lannka · 2017-08-06T18:41:54Z

test/functional/test-log.js

+ it('should return logger for user-error', () => {
+ const error = user().createError();
+ expect(isUserErrorEmbed(error.message)).to.be.false;
+ expect(user(this.element).suffix_).to.equal(USER_ERROR_SENTINEL);


avoid checking member field

expect(isUserError(error.message)).to.be.true;

lannka · 2017-08-06T18:42:03Z

test/functional/test-log.js

+ it('should return logger for embed-error', () => {
+ element = document.createElement('embed');
+ iframe.contentWindow.document.body.appendChild(element);
+ expect(user(element).suffix_).to.equal(USER_ERROR_EMBED_SENTINEL);


lannka · 2017-08-06T18:42:13Z

test/functional/test-log.js

+ expect(user(element).suffix_).to.equal(USER_ERROR_EMBED_SENTINEL);
+ });
+
+ it('should not create extra identical logs', () => {


lannka · 2017-08-06T18:43:11Z

test/functional/test-log.js

+ iframe.contentWindow.document.body.appendChild(element1);
+ iframe.contentWindow.document.body.appendChild(element2);
+ expect(user()).to.equal(user(this.element));
+ expect(user(element1)).to.equal(user(element2));


add one more check:

expect(user()).to.not.equal(user(element1));

@lannka i made all requested change. Thanks!

lannka · 2017-08-07T03:33:02Z

src/log.js

 if (logs.user) {
+ logs.user = logs.user;


zhouyx

LGTM with one question.

zhouyx · 2017-08-07T17:08:20Z

builtins/amp-img.js

- user().error('AMP-IMG', 'Setting role=img on amp-img elements breaks ' +
- 'screen readers please just set alt or ARIA attributes, they will ' +
- 'be correctly propagated for the underlying <img> element.');
+ user(this.element).error('AMP-IMG',


Question: Do we plan to migrate to the new user(opt_element) in this PR? If so why we only make the change to amp-img.js. I would suggest migrate to new format in separate PR.

@zhouyx I need that change to amp-img.js to do the test, but the whole migration to use(opt_element) would be in a separate PR. Is that ok?

Talked offline. Please revert. and I'll approve and merge the PR.

@zhouyx reverted!

lannka · 2017-08-07T17:55:32Z

src/log.js

+ */
+export function isFromEmbed(win, opt_element) {
+ if (!!opt_element) {
+ return opt_element.ownerDocument.defaultView != win;


nitpick: logically cleaner to flip the if

lannka · 2017-08-07T18:16:38Z

LGTM @zhouyx pls help merge once you have done the review.

googlebot added the cla: yes label Jul 20, 2017

tiendt requested a review from lannka July 20, 2017 22:15

lannka requested changes Jul 20, 2017

View reviewed changes

tiendt force-pushed the exclude-a4a branch from e8297a4 to c9dd097 Compare July 26, 2017 21:11

tiendt requested a review from zhouyx July 26, 2017 22:03

lannka requested changes Aug 4, 2017

View reviewed changes

zhouyx suggested changes Aug 4, 2017

View reviewed changes

tiendt added 16 commits August 4, 2017 16:12

initial change to user() in log.js

e3fdd09

add userForEmbed, hardcode isfromEmbed

4bff96a

change logic in user()

cee943e

add comments for SENTINEL use

53868f7

use EMBED string instead of global isEmbed boolean

5b7c36f

add test for correct user log

58da36e

add amp-pixel error

9125d24

add integration test

2b160f9

integration test in progress

1bdbcbd

Revert "integration test in progress"

9c0cdf5

This reverts commit 0cac14c.

Revert "add integration test"

9d1d093

This reverts commit 21402f1.

fix names to useError

45a21e3

update test-log

dd2bfe8

consolidate getUserLogger

9626057

revert change validator-amp-ad-protoascii

2ac30b5

add test identical logs

9004f6f

tiendt force-pushed the exclude-a4a branch from 48151ac to 9004f6f Compare August 4, 2017 23:12

tiendt added 2 commits August 4, 2017 16:25

delete error.embed

73eb77e

chang user() logic

293db2f

lannka reviewed Aug 6, 2017

View reviewed changes

lannka approved these changes Aug 6, 2017

View reviewed changes

tiendt added 3 commits August 6, 2017 14:31

change test-log and add test

a47f121

revert whitelist in presubmit-check

5c9880d

remove loggers var in user()

4e32c74

lannka reviewed Aug 7, 2017

View reviewed changes

src/log.js Outdated

if (logs.user) {

logs.user = logs.user;

Copy link

Contributor

lannka Aug 7, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why this?

remove user.logs = user.logs

000f6ba

lannka added in progress and removed in progress labels Aug 7, 2017

zhouyx suggested changes Aug 7, 2017

View reviewed changes

change code arrangement user()

fa293b1

lannka reviewed Aug 7, 2017

View reviewed changes

flip if logic in isFromEmbed

135a54d

zhouyx approved these changes Aug 7, 2017

View reviewed changes

tiendt force-pushed the exclude-a4a branch from 392ac49 to 135a54d Compare August 7, 2017 21:08

revert amp-img user(element)

9aa0819

zhouyx merged commit 5fc668e into ampproject:master Aug 7, 2017

ampprojectbot added the PR use: In Beta / Experimental label Aug 10, 2017

lannka mentioned this pull request Aug 11, 2017

Exclude A4A errors from amp-analytics error trigger #10697

Closed

tiendt removed the PR use: Work in Progress label Aug 31, 2017

This was referenced Nov 6, 2021

🚨 Error: startLayout called but not LAYOUT_SCHEDULED currently: 2 #36824

Closed

🚨 Error: startLayout called but not LAYOUT_SCHEDULED currently: 2 #36834

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Exclude errors generated from A4A iframe #10562

Exclude errors generated from A4A iframe #10562

tiendt commented Jul 20, 2017 •

edited

Loading

tiendt commented Jul 20, 2017

lannka Jul 20, 2017

lannka Aug 4, 2017

lannka Aug 4, 2017

lannka Aug 4, 2017

lannka Aug 4, 2017

lannka Aug 4, 2017

zhouyx Aug 4, 2017

zhouyx Aug 4, 2017

zhouyx Aug 4, 2017

tiendt Aug 4, 2017

zhouyx Aug 4, 2017

tiendt Aug 6, 2017

lannka Aug 6, 2017

lannka left a comment

lannka Aug 6, 2017

lannka Aug 6, 2017

tiendt Aug 6, 2017

lannka Aug 6, 2017

lannka Aug 6, 2017

lannka Aug 6, 2017

lannka Aug 6, 2017

lannka Aug 6, 2017

tiendt Aug 7, 2017

lannka Aug 7, 2017

zhouyx left a comment

zhouyx Aug 7, 2017 •

edited

Loading

tiendt Aug 7, 2017

zhouyx Aug 7, 2017

tiendt Aug 7, 2017

lannka Aug 7, 2017

lannka commented Aug 7, 2017

Exclude errors generated from A4A iframe #10562

Exclude errors generated from A4A iframe #10562

Conversation

tiendt commented Jul 20, 2017 • edited Loading

tiendt commented Jul 20, 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

Choose a reason for hiding this comment

lannka 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

zhouyx left a comment

Choose a reason for hiding this comment

zhouyx Aug 7, 2017 • edited Loading

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 7, 2017

tiendt commented Jul 20, 2017 •

edited

Loading

zhouyx Aug 7, 2017 •

edited

Loading