-
Notifications
You must be signed in to change notification settings - Fork 4.1k
Description
On this line:
Line 212 in 7731c43
| onError['call'](undefined, undefined, undefined, undefined, |
The first argument to call sets the this for the onError function, and in this case it is undefined. However, the function actually assumes that this is a !Window (for various checks such as whether to report the error to the viewer). Currently it always fail on this line:
Line 361 in 7731c43
| const ampdocService = Services.ampdocServiceFor(win); |
Because win is undefined. This is a bug that affects error reporting in regular AMP documents on the web, because the maybeReportErrorToViewer code path is triggered for regular AMP documents too. However, it only affects approximately half of the errors that are reported explicitly using reportError (and transitively, errors that are explicitly logged using dev().error(), for example). This doesn't affect errors that are thrown to the top level and caught by the window.onerror handler, since the this reference in that case would not be undefined:
Line 278 in 7731c43
| win.onerror = /** @type {!Function} */ (onError); |
I believe this bug has been in the AMP runtime since the following commit:
Where maybeReportErrorToViewer was first introduced.
It's hard to detect this bug because the error in the error reporting logic actually gets swallowed by the very same error reporting logic: what ended up happening is that Services.ampdocServiceFor(win) throws a Script error., and when that happens the error gets caught by the onError handler again. However, only 0.1% of Script error. are reported:
Line 468 in 7731c43
| if (throttleBase > NON_ACTIONABLE_ERROR_THROTTLE_THRESHOLD) { |
This is probably why this bug never triggered any alert and was unnoticed for months... The observed behavior is just that no error happened and no error reported, given the low 0.1% probability of reporting such error, which would only happen if some other error happened in the first place.
To fix the error, we need the win reference inside maybeReportErrorToViewer to be !Window and not !Window|undefined. Do you think it is safe to just use Services.ampdocServiceFor(self) or should we plumb the win reference all the way from reportErrorForWin to reportError to onError and then to maybeReportErrorToViewer? There are many callers of reportError outside of the error.js file if we have to update its signature.