1430317 - Add memory reporter to show basic information about active WebExtensions

Status: RESOLVED FIXED

(WebExtensions :: General, enhancement)

Description

[Kris Maglione [:kmag]]

We might want to display more detailed information about the extensions in the future, but for now, just basic information should be enough to help decipher memory reports we see in the wild.

Comment 1

[Kris Maglione [:kmag]]

Attachment: Bug 1430317: Add memory reporter to show basic information about active WebExtensions.

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

Comment 2

[Code Review Bot [:reviewbot]]

Comment on attachment 8942389 [details]
Bug 1430317: Add memory reporter to show basic information about active WebExtensions.

https://reviewboard.mozilla.org/r/212680/#review218370


C/C++ static analysis found 1 defect in this patch.

You can run this analysis locally with: `./mach static-analysis check path/to/file.cpp`

If you see a problem in this automated review, please report it here: http://bit.ly/2y9N9Vx


::: toolkit/components/extensions/ExtensionPolicyService.cpp:87
(Diff revision 1)
>    Preferences::AddBoolVarCache(&sRemoteExtensions, "extensions.webextensions.remote", false);
>  
>    RegisterObservers();
>  }
>  
> +ExtensionPolicyService::~ExtensionPolicyService()

Warning: Use '= default' to define a trivial destructor [clang-tidy: modernize-use-equals-default]

Comment 3

[Kris Maglione [:kmag]]

(In reply to Static Analysis Bot [:clangbot] from comment #2)
> > +ExtensionPolicyService::~ExtensionPolicyService()
> 
> Warning: Use '= default' to define a trivial destructor [clang-tidy:
> modernize-use-equals-default]

"Go home clangbot you're drunk"?

Comment 4

[Eric Rahm [:erahm]]

Comment on attachment 8942389 [details]
Bug 1430317: Add memory reporter to show basic information about active WebExtensions.

https://reviewboard.mozilla.org/r/212680/#review218390

Thanks Kris, this will be super helpful for triaging!

::: toolkit/components/extensions/ExtensionPolicyService.cpp:204
(Diff revision 1)
> +    name.ReplaceSubstring("\\", "");
> +
> +    nsString url;
> +    MOZ_TRY_VAR(url, ext->GetURL(NS_LITERAL_STRING("")));
> +
> +    nsPrintfCString desc("Extension(id=%s, name=\"%s\", baseURL=%s)",

If I understand correctly the ID changes from run to run, right?

If so can you either add a patch here or as a followup that normalizes this path in [about:memory diffing](https://searchfox.org/mozilla-central/rev/7fb999d1d39418fd331284fab909df076b967ac6/toolkit/components/extensions/WebExtensionPolicy.cpp#211)? That should also include an upate to the tests. See bug 1142174 for an example.

::: toolkit/components/extensions/ExtensionPolicyService.cpp:206
(Diff revision 1)
> +    nsString url;
> +    MOZ_TRY_VAR(url, ext->GetURL(NS_LITERAL_STRING("")));
> +
> +    nsPrintfCString desc("Extension(id=%s, name=\"%s\", baseURL=%s)",
> +                         id.get(), name.get(),
> +                         NS_ConvertUTF16toUTF8(url).get());

Just thought I'd share the wonderfulness of this:
`ext->GetURL` takes a `nsString`, converts it to a `nsCString`, gets the url as a `nsCString`, converts it to a `nsString` and then we convert it back here.

Comment 5

[Kris Maglione [:kmag]]

Comment on attachment 8942389 [details]
Bug 1430317: Add memory reporter to show basic information about active WebExtensions.

https://reviewboard.mozilla.org/r/212680/#review218390

> If I understand correctly the ID changes from run to run, right?
> 
> If so can you either add a patch here or as a followup that normalizes this path in [about:memory diffing](https://searchfox.org/mozilla-central/rev/7fb999d1d39418fd331284fab909df076b967ac6/toolkit/components/extensions/WebExtensionPolicy.cpp#211)? That should also include an upate to the tests. See bug 1142174 for an example.

No, the internal ID is always the same, unless you're using a temporarily-installed add-on that doesn't specify an ID.

The ID used in the URL changes every time you install the add-on in a new profile, but it's the same in every session.

> Just thought I'd share the wonderfulness of this:
> `ext->GetURL` takes a `nsString`, converts it to a `nsCString`, gets the url as a `nsCString`, converts it to a `nsString` and then we convert it back here.

Yeah, I don't like it either. The problem is that this method is mainly used from JS, via WebIDL, and while the JS engine does sometimes use 8-bit strings internally, the public API that our binding code uses only deals with UTF-16 strings. The upshot of that is that it's usually cheaper to have bindings deal with UTF-16 strings, though in this case, maybe not.

Comment 6

[Kris Maglione [:kmag]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/9114177c760378f740cf1d986f12a560fc124240
Bug 1430317: Add memory reporter to show basic information about active WebExtensions. r=erahm

Comment 7

[Kris Maglione [:kmag]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/cd6b2b32b23fd003c95605d7c29ea306f766a16a
Bug 1430317: Follow-up: Support QI to nsIMemoryReporter. r=me

Comment 8

[Cristina Coroiu [:ccoroiu]]

https://hg.mozilla.org/mozilla-central/rev/9114177c7603
https://hg.mozilla.org/mozilla-central/rev/cd6b2b32b23f

Comment 9

[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 10

[Kris Maglione [:kmag]]

Comment on attachment 8942389 [details]
Bug 1430317: Add memory reporter to show basic information about active WebExtensions.

Approval Request Comment
[Feature/Bug causing the regression]: N/A
[User impact if declined]: This change makes it much easier for us to triage reported memory leaks that involve WebExtensions. Having it on release a cycle sooner likely means that we'll be able to deal with major leaks (caused by either add-ons or by the platform) a cycle sooner.
[Is this code covered by automated tests?]: No
[Has the fix been verified in Nightly?]: I've verified it on the latest nightly.
[Needs manual test from QE? If yes, steps to reproduce]: No
[List of other uplifts needed for the feature/fix]: None
[Is the change risky?]: No
[Why is the change risky/not risky?]: It only adds a new, relatively simple memory reporter. It doesn't interact with any other code.
[String changes made/needed]: None

Comment 11

[Gerry Chang [:gchang]]

Comment on attachment 8942389 [details]
Bug 1430317: Add memory reporter to show basic information about active WebExtensions.

Too late for 58. Let's let it ride the train on 59.