Closed Bug 1461056 Opened 3 years ago Closed 3 years ago

Remove the "remoteBreakpad" symbol rule

Categories

(WebExtensions :: General, enhancement)

enhancement
Not set
normal

Tracking

(firefox62 fixed)

RESOLVED FIXED
mozilla62
Tracking Status
firefox62 --- fixed

People

(Reporter: mstange, Assigned: mstange)

Details

Attachments

(4 files)

The geckoProfiler WebExtensions API has a getSymbolTable method which uses "remoteBreakpad" as one of the available symbol rules. We no longer need it.

perf.html requests symbols from the Mozilla symbol server on its own now, so there's no need for the WebExtension API to check the server another time. These requests will always fail, and spinning up the worker threads that execute the request actually takes an appreciable amount of time.
This actually calls for some further cleanup because now all rules require haveAbsolutePath. I might do that in a follow-up.
Comment on attachment 8975221 [details]
Bug 1461056 - Remove the "remoteBreakpad" symbol rule, because it's no longer needed.

https://reviewboard.mozilla.org/r/243568/#review249490

Looks fine, but as you mention there's further cleanup that should be done. Looks like we can remove urlForSymFile, in addition to the haveAbsolutePath cleanup you mentioned.
Attachment #8975221 - Flags: review?(dothayer) → review+
Comment on attachment 8975615 [details]
Bug 1461056 - Remove unneeded function urlForSymFile.

https://reviewboard.mozilla.org/r/243876/#review249774

::: browser/components/extensions/parent/ext-geckoProfiler.js
(Diff revision 1)
>  
>  ChromeUtils.defineModuleGetter(this, "OS", "resource://gre/modules/osfile.jsm");
>  ChromeUtils.defineModuleGetter(this, "Subprocess", "resource://gre/modules/Subprocess.jsm");
>  
>  const PREF_ASYNC_STACK = "javascript.options.asyncstack";
> -const PREF_SYMBOLS_URL = "extensions.geckoProfiler.symbols.url";

Nit: Drop the pref definition from firefox.js as well?
Attachment #8975615 - Flags: review?(dothayer) → review+
Comment on attachment 8975616 [details]
Bug 1461056 - Remove haveAbsolutePath and tweak some error messages.

https://reviewboard.mozilla.org/r/243878/#review249776
Attachment #8975616 - Flags: review?(dothayer) → review+
Comment on attachment 8975615 [details]
Bug 1461056 - Remove unneeded function urlForSymFile.

https://reviewboard.mozilla.org/r/243876/#review249774

> Nit: Drop the pref definition from firefox.js as well?

Good catch, fixed. When I searched for the pref name, I encountered the test, so I added another patch to remove that one as well.
Comment on attachment 8975721 [details]
Bug 1461056 - Remove browser_ext_geckoProfiler_symbolicate test.

https://reviewboard.mozilla.org/r/243946/#review249882
Attachment #8975721 - Flags: review?(dothayer) → review+
Pushed by mstange@themasta.com:
https://hg.mozilla.org/integration/autoland/rev/75228c5c58f5
Remove the "remoteBreakpad" symbol rule, because it's no longer needed. r=dthayer
https://hg.mozilla.org/integration/autoland/rev/0dc1c6185eae
Remove unneeded function urlForSymFile. r=dthayer
https://hg.mozilla.org/integration/autoland/rev/80163b051c12
Remove haveAbsolutePath and tweak some error messages. r=dthayer
https://hg.mozilla.org/integration/autoland/rev/ac4a82051ab7
Remove browser_ext_geckoProfiler_symbolicate test. r=dthayer
Is manual testing required on this bug? If Yes, please provide some STR and the proper webextension(if required), if No set the “qe-verify-“ flag.
Flags: needinfo?(mstange)
No manual testing is required.
Flags: needinfo?(mstange) → qe-verify-
Product: Toolkit → WebExtensions
You need to log in before you can comment on or make changes to this bug.