1461056 - Remove the "remoteBreakpad" symbol rule

Status: RESOLVED FIXED

(WebExtensions :: General, enhancement)

Description

[Markus Stange [:mstange]]

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.

Comment 1

[Markus Stange [:mstange]]

Attachment: Bug 1461056 - Remove the "remoteBreakpad" symbol rule, because it's no longer needed.

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

Comment 2

[Markus Stange [:mstange]]

This actually calls for some further cleanup because now all rules require haveAbsolutePath. I might do that in a follow-up.

Comment 3

[Alex Thayer [:alexical] (she/her)]

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.

Comment 4

[Markus Stange [:mstange]]

Attachment: Bug 1461056 - Remove unneeded function urlForSymFile.

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

Comment 5

[Markus Stange [:mstange]]

Attachment: Bug 1461056 - Remove haveAbsolutePath and tweak some error messages.

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

Comment 6

[Markus Stange [:mstange]]

Comment on attachment 8975221 [details]
Bug 1461056 - Remove the "remoteBreakpad" symbol rule, because it's no longer needed.

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

Comment 7

[Alex Thayer [:alexical] (she/her)]

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?

Comment 8

[Alex Thayer [:alexical] (she/her)]

Comment on attachment 8975616 [details]
Bug 1461056 - Remove haveAbsolutePath and tweak some error messages.

https://reviewboard.mozilla.org/r/243878/#review249776

Comment 9

[Markus Stange [:mstange]]

Attachment: Bug 1461056 - Remove browser_ext_geckoProfiler_symbolicate test.

The thing that it's testing is being removed.
I couldn't think of something that I could replace the test with. Maybe once we have
bug 1457481 we can check in an actual ELF binary and check that we're dumping its
symbol table correctly.

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

Comment 10

[Markus Stange [:mstange]]

Comment on attachment 8975221 [details]
Bug 1461056 - Remove the "remoteBreakpad" symbol rule, because it's no longer needed.

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

Comment 11

[Markus Stange [:mstange]]

Comment on attachment 8975615 [details]
Bug 1461056 - Remove unneeded function urlForSymFile.

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

Comment 12

[Markus Stange [:mstange]]

Comment on attachment 8975616 [details]
Bug 1461056 - Remove haveAbsolutePath and tweak some error messages.

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

Comment 13

[Markus Stange [:mstange]]

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 14

[Alex Thayer [:alexical] (she/her)]

Comment on attachment 8975721 [details]
Bug 1461056 - Remove browser_ext_geckoProfiler_symbolicate test.

https://reviewboard.mozilla.org/r/243946/#review249882

Comment 15

[Pulsebot]

Pushed by [e-mail]:
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

Comment 16

[Natalia Csoregi [:nataliaCs]]

https://hg.mozilla.org/mozilla-central/rev/75228c5c58f5
https://hg.mozilla.org/mozilla-central/rev/0dc1c6185eae
https://hg.mozilla.org/mozilla-central/rev/80163b051c12
https://hg.mozilla.org/mozilla-central/rev/ac4a82051ab7

Comment 17

[marius.santa]

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.

Comment 18

[Markus Stange [:mstange]]

No manual testing is required.