1445009 - Tag browser error reports caused by extensions

Status: RESOLVED FIXED

(Firefox :: General, enhancement)

Description

[Osmose [:osmose, :mkelly]]

Many of the errors being reported to Sentry are caused by extensions and have a moz-extension:// URL as the first part of the stack trace. We should include a tag to mark these errors in the payload sent to Sentry so that we can filter these errors out or search only these errors in the Sentry UI.

Comment 1

[Osmose [:osmose, :mkelly]]

I think this is actually a bug with the error reporter rather than webextensions; we should be inspecting the source file errors are coming from and seeing if they start with moz-extension://.

Comment 2

[Kris Maglione [:kmag]]

Technically it's possible for extension errors to come from non-moz-extension: URIs. You should ideally check the principal of the top frame.

Comment 3

[Osmose [:osmose, :mkelly]]

Attachment: Bug 1445009: Refactor BrowserErrorReporter.jsm and tests.

The transforms for turning an nsIScriptError into a payload that Sentry
understands were getting a bit complex for a single function, so they're
refactored into a list of transform functions that are applied in sequence to
produce the payload. This will make it easier to manage adding new transforms to
the list.

Refactoring this revaled a problem with the test code: it assumed that listeners
for console messages were notified in order of registration (since it used a
temporary listener to determine when the rest of the listeners had been notified
of a message). Changing the async evaluation of the code broke the tests, so
they had to be refactored as well.

Without a way to know when all console listeners have been notified, we can't
assert that a message will not be received by BrowserErrorReporter. We do two
things to get around this:

- Where possible, call `observe` directly on the reporter instance.
- Add constructor params for registering and unregistering listeners so we can
  test that logic without relying on messages being received or not.

Review commit: https://reviewboard.mozilla.org/r/229104/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/229104/

Comment 4

[Osmose [:osmose, :mkelly]]

Attachment: Bug 1445009: Tag errors from extensions with isExtensionError.

Review commit: https://reviewboard.mozilla.org/r/229106/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/229106/

Comment 5

[:Gijs (he/him)]

Comment on attachment 8960349 [details]
Bug 1445009: Tag errors from extensions with isExtensionError.

https://reviewboard.mozilla.org/r/229106/#review235134

Comment 6

[:Gijs (he/him)]

Comment on attachment 8960348 [details]
Bug 1445009: Refactor BrowserErrorReporter.jsm and tests.

https://reviewboard.mozilla.org/r/229104/#review235210

Generally LGTM, but some suggestions below.

::: browser/modules/BrowserErrorReporter.jsm:104
(Diff revision 1)
>      return this.logger;
>    }
>  
>    init() {
>      if (this.collectionEnabled) {
> -      Services.console.registerListener(this);
> +      this.registerListener(this);

Tempted to suggest this (and the unregister version) should just not pass anything, as we always pass `this`, so the lambda could just take care of that?

::: browser/modules/test/browser/browser_BrowserErrorReporter.js:118
(Diff revision 1)
> +  Services.console.logMessage(createScriptError({message: "Logged before init"}));
>    reporter.init();

Sorry, unrelated to this change but, should we ensure that we wait for a separate console listener to have received the script error before calling `reporter.init()`, to properly test what's going on here, given the async-ness within the console service?

::: browser/modules/test/browser/browser_BrowserErrorReporter.js
(Diff revision 1)
>      "Stack frame filenames use the proper add-on ID instead of internal UUIDs.",
>    );
>  
>    await extension.unload();
>    reporter.uninit();
> -  resetConsole();

Why can we omit this last reset? I realize it's been moved, but from the test it looks like the error is logged after the `resetConsole` call, so then the console will still have errors at the end of the test...

Comment 7

[Osmose [:osmose, :mkelly]]

Comment on attachment 8960348 [details]
Bug 1445009: Refactor BrowserErrorReporter.jsm and tests.

https://reviewboard.mozilla.org/r/229104/#review235220

::: browser/modules/test/browser/browser_BrowserErrorReporter.js:118
(Diff revision 1)
> +  Services.console.logMessage(createScriptError({message: "Logged before init"}));
>    reporter.init();

We already removed one instance of doing that to check that something reached the console service (which turned out to be an unreliable method for checking that), so I'm a bit hesitant to add it back.

I think it's reasonable to assume that the listeners are notified async, but the list of previously-logged messages is updated synchronously. Is that ok?

::: browser/modules/test/browser/browser_BrowserErrorReporter.js
(Diff revision 1)
>      "Stack frame filenames use the proper add-on ID instead of internal UUIDs.",
>    );
>  
>    await extension.unload();
>    reporter.uninit();
> -  resetConsole();

The resets were at the end of each test as a sort've guard, but with the switch to using reporter.observe in most places, it makes less sense to obsessively clean up the console logging. So instead I switched to calling the reset before each test that might be affected by it, which seems a bit more reliable to me than just cleaning up after every test and hoping nothing gets logged in the meantime.

Comment 8

[Osmose [:osmose, :mkelly]]

Comment on attachment 8960348 [details]
Bug 1445009: Refactor BrowserErrorReporter.jsm and tests.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/229104/diff/1-2/

Comment 9

[Osmose [:osmose, :mkelly]]

Comment on attachment 8960349 [details]
Bug 1445009: Tag errors from extensions with isExtensionError.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/229106/diff/1-2/

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/a7c5889ae81f
Refactor BrowserErrorReporter.jsm and tests. r=Gijs
https://hg.mozilla.org/integration/autoland/rev/e62d73ef396c
Tag errors from extensions with isExtensionError. r=Gijs

Comment 11

[Xidorn Quan [:xidorn] UTC+10]

This bug has been backed out in https://hg.mozilla.org/integration/autoland/rev/6e5043e04b88f201eeceb59165d213bc4b8b15e8 for merge conflicts with bug 1444554.

Since it seems the tree is a bit messed up after some unsuccessful backout, I directly reverted the files to the previous revision before both changes.

Comment 12

[Kris Maglione [:kmag]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/df318dd1a27e2dc63852224264af3bbf635bc63d
Bug 1445009: Refactor BrowserErrorReporter.jsm and tests. r=Gijs

https://hg.mozilla.org/integration/mozilla-inbound/rev/5c612345552cdf2ffa4b0c127b412b1083e61fb7
Bug 1445009: Tag errors from extensions with isExtensionError. r=Gijs

Comment 13

[Tiberius Oros[:tiberius_oros]]

https://hg.mozilla.org/mozilla-central/rev/df318dd1a27e
https://hg.mozilla.org/mozilla-central/rev/5c612345552c