Closed Bug 1150045 Opened 9 years ago Closed 9 years ago

[PerformanceStats] De-anonymize "[Expanded Principal]" compartments

Categories

(Toolkit :: Performance Monitoring, defect)

x86
macOS
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla40
Tracking Status
firefox40 --- fixed

People

(Reporter: Yoric, Assigned: Yoric)

References

Details

Attachments

(1 file, 4 obsolete files)

Jetpack seems to create lots of compartments whose single name is "[Expanded Principal]". We should make sure that they are counted and displayed as part of their parent add-on.
A side-effect of this is that these compartments confuse the PerformanceStatsService.
Component: General → Performance Monitoring
For some reason, it looks like `addonId` is not set in these compartments. Apparently, the code should have been set here: https://hg.mozilla.org/mozilla-central/annotate/18a8ea7c2c62/js/xpconnect/src/Sandbox.cpp#l896.

Jandem, Bill, do you know what could cause that?
Flags: needinfo?(wmccloskey)
Flags: needinfo?(jdemooij)
Leaving this to Bill, I'm not very familiar with the addonId.
Flags: needinfo?(jdemooij)
Jetpack is supposed to set options.addonId. That happens here:
http://mxr.mozilla.org/mozilla-central/source/addon-sdk/source/lib/toolkit/loader.js#247

Do you have a testcase or something? Ni Mossop in case there's something wrong with the code there.
Flags: needinfo?(wmccloskey) → needinfo?(dtownsend)
(In reply to Bill McCloskey (:billm) from comment #4)
> Jetpack is supposed to set options.addonId. That happens here:
> http://mxr.mozilla.org/mozilla-central/source/addon-sdk/source/lib/toolkit/
> loader.js#247
> 
> Do you have a testcase or something? Ni Mossop in case there's something
> wrong with the code there.

This is probably a page-mod or worker sandbox which looks like it isn't passing the addonId optiuons correctly (I think we switched from passing it in the metadata to the options at one point): http://mxr.mozilla.org/mozilla-central/source/addon-sdk/source/lib/sdk/loader/sandbox.js#26

I can review a patch there.
Flags: needinfo?(dtownsend)
I have noticed this only under Windows. Launching mochitest-bc try on Windows seems to be leaving ~7 such compartments by the time we hit test browser_compartments.js. I have also witnessed such compartments in regular browsing with a few add-ons. I will try and hunt them down tomorrow.
Bobby, would changing the result of `nsExpandedPrincipal::GetScriptLocation` have security implications?
Flags: needinfo?(bobbyholley)
For the moment, I haven't been able to find out what creates these Expanded Principals. Many seem to be created during startup by XBL.

Try run with an experimental patch attempting to make this easier to debug by giving them a more complete name than "Expanded Principals": https://treeherder.mozilla.org/#/jobs?repo=try&revision=f09dbd7494c4
According to my breakpoints, the Expanded Principals that are not attached to an addon and that I have witnessed under Windows have not been created by `Cu.Sandbox`. I have no clue where they come from.
While this will not solve the issue, this might at least help a bit with debugging (and probably fix bug 1151240).
Flags: needinfo?(bobbyholley)
Attachment #8589721 - Flags: feedback?(bobbyholley)
(In reply to David Rajchenbach-Teller [:Yoric] (use "needinfo") from comment #9)
> According to my breakpoints, the Expanded Principals that are not attached
> to an addon and that I have witnessed under Windows have not been created by
> `Cu.Sandbox`. I have no clue where they come from.

Presumably they're created by XPCWrappedNativeScope::EnsureContentXBLScope? We create one extra compartment for each content global that loads and XBL binding.
Comment on attachment 8589721 [details] [diff] [review]
Making Expanded Principals less anonymous

Review of attachment 8589721 [details] [diff] [review]:
-----------------------------------------------------------------

::: caps/nsPrincipal.cpp
@@ +990,5 @@
> +
> +    nsCOMPtr<nsIPrincipal> principal = mPrincipals.ElementAt(i);
> +    nsCOMPtr<nsIURI> uri;
> +    DebugOnly<nsresult> rv = principal->GetURI(getter_AddRefs(uri));
> +    MOZ_ASSERT(NS_SUCCEEDED(rv));

What if the principal doesn't have a URI? Sees like we should recursively invoke GetScriptLocation on the sub-principals.

::: js/xpconnect/src/Sandbox.cpp
@@ -907,5 @@
>      }
> -    if (!addonId) {
> -        printf_stderr("xpc::CreateSandboxObject: no addonId\n");
> -        DumpJSStack();
> -    }

I don't think you meant to have this in the patch.
Attachment #8589721 - Flags: feedback?(bobbyholley) → feedback+
Applied your feedback.
I make the assumption that the only instances of `nsIPrincpal` are subclasses of `nsBasePrincipal`. The alternative would be to somehow add `GetScriptLocation` to the idl.
Assignee: nobody → dteller
Attachment #8589721 - Attachment is obsolete: true
Attachment #8589896 - Flags: review?(bobbyholley)
Comment on attachment 8589896 [details] [diff] [review]
Making expanded principals less anonymous, v2.

Review of attachment 8589896 [details] [diff] [review]:
-----------------------------------------------------------------

::: caps/nsPrincipal.cpp
@@ +987,5 @@
> +    if (i != 0) {
> +      aStr.AppendLiteral(", ");
> +    }
> +
> +    nsRefPtr<nsBasePrincipal> principal = dynamic_cast<nsBasePrincipal*>(mPrincipals.ElementAt(i).get());

We don't build with RTTI, which is why we forbid dynamic_cast. Does this actually work? I would be very surprised.

Anyway, not all principals inherit from nsBasePrincipal (i.e. nsSystemPrincipal and nsNullPrincipal). What they _do_ all inherit from is nsJSPrincipals. You can do nsJSPrincipals::get(nsIPrincipal*)->GetScriptLocation. Alternatively, we could move GetScriptLocation from nsJSPrincipals to a [noscript,notxpcom] method on nsIPrincipal.
Attachment #8589896 - Flags: review?(bobbyholley) → review-
You are, of course, right. This is much nicer.
(I blame the dynamic_cast on sleep deprivation)

Try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=5276fd50368c
Attachment #8589896 - Attachment is obsolete: true
Attachment #8589967 - Flags: review?(bobbyholley)
Comment on attachment 8589967 [details] [diff] [review]
Making Expanded Principals less anonymous, v3

Review of attachment 8589967 [details] [diff] [review]:
-----------------------------------------------------------------

Can you add a test for this in js/xpconnect/tests/unit? r=me with all that.

::: caps/nsPrincipal.h
@@ +31,5 @@
>    NS_IMETHOD_(MozExternalRefCountType) Release(void);
>    NS_IMETHOD GetCsp(nsIContentSecurityPolicy** aCsp);
>    NS_IMETHOD SetCsp(nsIContentSecurityPolicy* aCsp);
> +
> +  virtual void GetScriptLocation(nsACString& aStr) = 0;

This is extraneous, right? Please remove it.
Attachment #8589967 - Flags: review?(bobbyholley) → review+
I had to make a small change to Sandbox.cpp, for testing purposes, although it could prove useful, tell me if that's ok with you.

Alternatively, I could have made a similar change in nsXPCComponents_Utils::EvalInSandbox.
Attachment #8589967 - Attachment is obsolete: true
Attachment #8590160 - Flags: review?(bobbyholley)
Comment on attachment 8590160 [details] [diff] [review]
Making Expanded Principals less anonymous, v4

Review of attachment 8590160 [details] [diff] [review]:
-----------------------------------------------------------------

::: caps/nsPrincipal.cpp
@@ +983,5 @@
>    aStr.Assign(EXPANDED_PRINCIPAL_SPEC);
> +  aStr.AppendLiteral(" (");
> +
> +  for (size_t i = 0; i < mPrincipals.Length(); ++i) {
> +    if (i != 0) {

Shouldn't we also check |i != mPrincipals.Length() - 1| to avoid a trailing comma?

::: js/xpconnect/tests/unit/test_sandbox_name.js
@@ +14,5 @@
> +  let fileName = Cu.evalInSandbox(
> +    "(new Error()).fileName",
> +    sandbox,
> +    "latest" /*js version*/,
> +    ""/*file name*/

The last two arguments are optional, you can just omit them. That should also mean that you don't have to change Sandbox.cpp, though I don't care much one way or the other there.
Attachment #8590160 - Flags: review?(bobbyholley) → review+
(In reply to Bobby Holley (:bholley) from comment #19)
> Shouldn't we also check |i != mPrincipals.Length() - 1| to avoid a trailing
> comma?

Unless my sleep deprivation is worse than I thought, I don't think so.

> ::: js/xpconnect/tests/unit/test_sandbox_name.js
> The last two arguments are optional, you can just omit them. That should
> also mean that you don't have to change Sandbox.cpp, though I don't care
> much one way or the other there.

Actually, `nsXPCComponents_Utils::EvalInSandbox` intercepts the case in which the file name is omitted and replaces with the filename of the caller. That's where I needed to patch Sandbox.cpp.
Right on both counts - proceed. :-)
https://hg.mozilla.org/integration/fx-team/rev/9af492c191a9
Flags: in-testsuite+
Keywords: checkin-needed
Whiteboard: [fixed-in-fx-team]
https://hg.mozilla.org/mozilla-central/rev/9af492c191a9
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → mozilla40
You need to log in before you can comment on or make changes to this bug.