-
Notifications
You must be signed in to change notification settings - Fork 3.9k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Exclude errors generated from A4A iframe #10562
Changes from 21 commits
e3fdd09
4bff96a
cee943e
53868f7
5b7c36f
58da36e
9125d24
2b160f9
1bdbcbd
9c0cdf5
9d1d093
45a21e3
dd2bfe8
9626057
2ac30b5
9004f6f
73eb77e
293db2f
a47f121
5c9880d
4e32c74
000f6ba
fa293b1
135a54d
9aa0819
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -33,13 +33,28 @@ const start = Date.now(); | |
export const USER_ERROR_SENTINEL = '\u200B\u200B\u200B'; | ||
|
||
|
||
/** | ||
* Four zero width space. | ||
* | ||
* @const {string} | ||
*/ | ||
export const USER_ERROR_EMBED_SENTINEL = '\u200B\u200B\u200B\u200B'; | ||
|
||
|
||
/** | ||
* @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 a user error from an iframe embed. | ||
*/ | ||
export function isUserErrorEmbed(message) { | ||
return message.indexOf(USER_ERROR_EMBED_SENTINEL) >= 0; | ||
} | ||
|
||
|
||
/** | ||
* @enum {number} | ||
|
@@ -64,6 +79,10 @@ export function setReportError(fn) { | |
|
||
/** | ||
* Logging class. | ||
* Use of sentinel string instead of a boolean to check user/dev errors | ||
* because errors could be rethrown by some native code as a new error, and only a message would survive. | ||
* Also, some browser don’t support a 5th error object argument in window.onerror. List of supporting browser can be found | ||
* here: https://blog.sentry.io/2016/01/04/client-javascript-reporting-window-onerror.html | ||
* @final | ||
* @private Visible for testing only. | ||
*/ | ||
|
@@ -494,11 +513,12 @@ export function rethrowAsync(var_args) { | |
/** | ||
* Cache for logs. We do not use a Service since the service module depends | ||
* on Log and closure literally can't even. | ||
* @type {{user: ?Log, dev: ?Log}} | ||
* @type {{user: ?Log, dev: ?Log, userForEmbed: ?Log}} | ||
*/ | ||
self.log = (self.log || { | ||
user: null, | ||
dev: null, | ||
userForEmbed: null, | ||
}); | ||
|
||
const logs = self.log; | ||
|
@@ -531,7 +551,6 @@ export function resetLogConstructorForTesting() { | |
logConstructor = null; | ||
} | ||
|
||
|
||
/** | ||
* Publisher level log. | ||
* | ||
|
@@ -540,25 +559,45 @@ export function resetLogConstructorForTesting() { | |
* 2. Development mode is enabled via `#development=1` or logging is explicitly | ||
* enabled via `#log=D` where D >= 1. | ||
* | ||
* @param {!Element=} opt_element | ||
* @return {!Log} | ||
*/ | ||
export function user() { | ||
export function user(opt_element) { | ||
//let logger; | ||
if (logs.user) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. we should now cache 2 instances of user logger.
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;
} |
||
logs.user = logs.user; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. why this? |
||
} else { | ||
logs.user = getUserLogger(USER_ERROR_SENTINEL); | ||
} | ||
if (!!opt_element && | ||
isFromEmbed(logs.user.win, /** @type {!Element} */ (opt_element))) { | ||
if (logs.userForEmbed) { | ||
return logs.userForEmbed; | ||
} | ||
return logs.userForEmbed = getUserLogger(USER_ERROR_EMBED_SENTINEL); | ||
} else { | ||
return logs.user; | ||
} | ||
} | ||
|
||
/** | ||
* Getter for user logger | ||
* @param {string=} suffix | ||
* @returns {!Log} | ||
*/ | ||
function getUserLogger(suffix) { | ||
if (!logConstructor) { | ||
throw new Error('failed to call initLogConstructor'); | ||
} | ||
return logs.user = new logConstructor(self, mode => { | ||
return new logConstructor(self, mode => { | ||
const logNum = parseInt(mode.log, 10); | ||
if (mode.development || logNum >= 1) { | ||
return LogLevel.FINE; | ||
} | ||
return LogLevel.OFF; | ||
}, USER_ERROR_SENTINEL); | ||
}, suffix); | ||
} | ||
|
||
|
||
/** | ||
* AMP development log. Calls to `devLog().assert` and `dev.fine` are stripped in | ||
* the PROD binary. However, `devLog().assert` result is preserved in either case. | ||
|
@@ -587,3 +626,12 @@ export function dev() { | |
return LogLevel.OFF; | ||
}); | ||
} | ||
|
||
/** | ||
* @param {!Window} win | ||
* @param {!Element} element | ||
* @returns {boolean} isEmbed | ||
*/ | ||
export function isFromEmbed(win, element) { | ||
return element.ownerDocument.defaultView != win; | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -24,6 +24,7 @@ import { | |
setReportError, | ||
user, | ||
duplicateErrorIfNecessary, | ||
isUserErrorEmbed, | ||
} from '../../src/log'; | ||
import * as sinon from 'sinon'; | ||
|
||
|
@@ -673,5 +674,47 @@ describe('Logging', () => { | |
expect(duplicate.associatedElement).to.equal(error.associatedElement); | ||
}); | ||
}); | ||
|
||
describe('embed error', () => { | ||
let sandbox; | ||
let iframe; | ||
let element; | ||
let element1; | ||
let element2; | ||
|
||
beforeEach(() => { | ||
sandbox = sinon.sandbox.create(); | ||
iframe = document.createElement('iframe'); | ||
document.body.appendChild(iframe); | ||
}); | ||
|
||
afterEach(() => { | ||
sandbox.restore(); | ||
document.body.removeChild(iframe); | ||
}); | ||
|
||
it('should return logger for user-error', () => { | ||
const error = user().createError(); | ||
expect(isUserErrorEmbed(error.message)).to.be.false; | ||
expect(isUserErrorMessage(error.message)).to.be.true; | ||
}); | ||
|
||
it('should return logger for embed-error', () => { | ||
element = document.createElement('embed'); | ||
iframe.contentWindow.document.body.appendChild(element); | ||
const error = user(element).createError(); | ||
expect(isUserErrorEmbed(error.message)).to.be.true; | ||
}); | ||
|
||
it('should not create extra identical loggers', () => { | ||
element1 = document.createElement('embed_1'); | ||
element2 = document.createElement('embed_2'); | ||
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)); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. add one more check:
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @lannka i made all requested change. Thanks! |
||
expect(user()).to.not.equal(user(element1)); | ||
}); | ||
}); | ||
}); | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Question: Do we plan to migrate to the new
user(opt_element)
in this PR? If so why we only make the change toamp-img.js
. I would suggest migrate to new format in separate PR.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Talked offline. Please revert. and I'll approve and merge the PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@zhouyx reverted!