Closed
Bug 1150045
Opened 9 years ago
Closed 9 years ago
[PerformanceStats] De-anonymize "[Expanded Principal]" compartments
Categories
(Toolkit :: Performance Monitoring, defect)
Tracking
()
RESOLVED
FIXED
mozilla40
Tracking | Status | |
---|---|---|
firefox40 | --- | fixed |
People
(Reporter: Yoric, Assigned: Yoric)
References
Details
Attachments
(1 file, 4 obsolete files)
4.30 KB,
patch
|
Yoric
:
review+
|
Details | Diff | Splinter Review |
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.
Assignee | ||
Comment 1•9 years ago
|
||
A side-effect of this is that these compartments confuse the PerformanceStatsService.
Assignee | ||
Updated•9 years ago
|
Component: General → Performance Monitoring
Assignee | ||
Comment 2•9 years ago
|
||
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)
Comment 3•9 years ago
|
||
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)
Comment 5•9 years ago
|
||
(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)
Assignee | ||
Comment 6•9 years ago
|
||
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.
Assignee | ||
Comment 7•9 years ago
|
||
Bobby, would changing the result of `nsExpandedPrincipal::GetScriptLocation` have security implications?
Assignee | ||
Updated•9 years ago
|
Flags: needinfo?(bobbyholley)
Assignee | ||
Comment 8•9 years ago
|
||
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
Assignee | ||
Comment 9•9 years ago
|
||
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.
Assignee | ||
Comment 10•9 years ago
|
||
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)
Comment 11•9 years ago
|
||
(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 12•9 years ago
|
||
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+
Assignee | ||
Comment 13•9 years ago
|
||
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)
Assignee | ||
Comment 14•9 years ago
|
||
Try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=818e89b71e2c
Comment 15•9 years ago
|
||
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-
Assignee | ||
Comment 16•9 years ago
|
||
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 17•9 years ago
|
||
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+
Assignee | ||
Comment 18•9 years ago
|
||
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 19•9 years ago
|
||
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+
Assignee | ||
Comment 20•9 years ago
|
||
(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.
Comment 21•9 years ago
|
||
Right on both counts - proceed. :-)
Assignee | ||
Comment 22•9 years ago
|
||
As a side-note, I suspect that some of the anonymous expanded principals come from here: https://dxr.mozilla.org/mozilla-central/source/toolkit/components/addoncompat/RemoteAddonsParent.jsm?from=RemoteAddonsParent.jsm#682-696
Assignee | ||
Comment 23•9 years ago
|
||
Fixed typo. Try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=ab89f1a14caf
Assignee | ||
Comment 24•9 years ago
|
||
Attachment #8590160 -
Attachment is obsolete: true
Attachment #8592151 -
Flags: review+
Assignee | ||
Updated•9 years ago
|
Keywords: checkin-needed
Comment 26•9 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/9af492c191a9
Status: NEW → RESOLVED
Closed: 9 years ago
status-firefox40:
--- → fixed
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → mozilla40
Comment 27•9 years ago
|
||
Commit pushed to master at https://github.com/mozilla/addon-sdk https://github.com/mozilla/addon-sdk/commit/134e1fa4d4df6372fb0a6baabc93daab98eb7f74 Bug 1150045 - De-anonymize Expanded Principals. r=bholley
You need to log in
before you can comment on or make changes to this bug.
Description
•