Tag browser error reports caused by extensions

RESOLVED FIXED in Firefox 61

Status

()

enhancement
RESOLVED FIXED
Last year
Last year

People

(Reporter: osmose, Assigned: osmose)

Tracking

Trunk
Firefox 61
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox61 fixed)

Details

Attachments

(2 attachments)

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.
Component: General → WebExtensions: Untriaged
Product: Firefox → Toolkit
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://.
Component: WebExtensions: Untriaged → General
Product: Toolkit → Firefox
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 on attachment 8960349 [details]
Bug 1445009: Tag errors from extensions with isExtensionError.

https://reviewboard.mozilla.org/r/229106/#review235134
Attachment #8960349 - Flags: review?(gijskruitbosch+bugs) → review+
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...
Attachment #8960348 - Flags: review?(gijskruitbosch+bugs) → review+
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.
Pushed by mkelly@mozilla.com:
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
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.
Flags: needinfo?(mkelly)
https://hg.mozilla.org/mozilla-central/rev/df318dd1a27e
https://hg.mozilla.org/mozilla-central/rev/5c612345552c
Status: NEW → RESOLVED
Closed: Last year
Resolution: --- → FIXED
Target Milestone: --- → Firefox 61
Flags: needinfo?(mkelly)
See Also: → 1449312
You need to log in before you can comment on or make changes to this bug.