Closed Bug 1430317 Opened 2 years ago Closed 2 years ago

Add memory reporter to show basic information about active WebExtensions

Categories

(WebExtensions :: General, enhancement)

enhancement
Not set

Tracking

(firefox59 fixed)

RESOLVED FIXED
mozilla59
Tracking Status
firefox59 --- fixed

People

(Reporter: kmag, Assigned: kmag)

References

Details

Attachments

(1 file)

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 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]
(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 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.
Attachment #8942389 - Flags: review?(erahm) → review+
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.
https://hg.mozilla.org/integration/mozilla-inbound/rev/9114177c760378f740cf1d986f12a560fc124240
Bug 1430317: Add memory reporter to show basic information about active WebExtensions. r=erahm
https://hg.mozilla.org/mozilla-central/rev/9114177c7603
https://hg.mozilla.org/mozilla-central/rev/cd6b2b32b23f
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla59
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?(kmaglione+bmo)
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
Attachment #8942389 - Flags: approval-mozilla-beta?
Flags: needinfo?(kmaglione+bmo) → qe-verify-
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.
Attachment #8942389 - Flags: approval-mozilla-beta? → approval-mozilla-beta-
Product: Toolkit → WebExtensions
Duplicate of this bug: 787466
You need to log in before you can comment on or make changes to this bug.