Crash in nsNodeInfoManager::SetDocumentPrincipal

RESOLVED FIXED in Firefox 58

Status

()

defect
--
critical
RESOLVED FIXED
2 years ago
5 months ago

People

(Reporter: calixte, Assigned: kmag)

Tracking

({crash, regression})

58 Branch
mozilla58
x86
Windows 7
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox-esr52 unaffected, firefox56 unaffected, firefox57 unaffected, firefox58 fixed)

Details

(crash signature)

Attachments

(1 attachment)

This bug was filed from the Socorro interface and is 
report bp-b5fa0ee1-ae01-4924-9143-be4110171010.
=============================================================

There are 50 crashes in nightly 58 with buildid 20171010100200.
They're probably related to fix for bug 1391693.
:bkelly, could you investigate please ?
Flags: needinfo?(bkelly)
I don't think this is related to bug 1391693.  That bug changed code in the parent process, but this crash is clearly happening in the child process.  While e10s does also use the parent process channel, we don't use it to provide data like this stack shows.  I think this is a different problem.

Also, this stack is using nsBaseChannel which HttpChannelChild and InterceptedHttpChannel don't use.
Flags: needinfo?(bkelly)
A bunch of principal changes in bug 1406278.
Kris, can you take a look at this?  It seems we're getting an expanded principal someplace we should not.  Could this be fallout from bug 1406278?
Flags: needinfo?(kmaglione+bmo)
Component: DOM: Service Workers → DOM
STR over in bug 1407275.
See Also: → 1407275
I guess we're loading a URL that inherits principal from a content script? I thought we already refused to inherit expanded principals that way...
Assignee: nobody → kmaglione+bmo
Flags: needinfo?(kmaglione+bmo)
Since calixte filed this there are now over 1129 crashes/239 installs.
Duplicate of this bug: 1407275
Comment on attachment 8917152 [details]
Bug 1407258: Dowgrade document loads with expanded principals to their last sub-principal.

https://reviewboard.mozilla.org/r/188166/#review193320

Please make sure a followup for this is filed and referenced in the code comments or commit message.

::: dom/base/nsDocument.cpp:2497
(Diff revision 1)
> +  // extension principal, in the case of extension content scripts).
> +  auto* basePrin = BasePrincipal::Cast(aPrincipal);
> +  if (basePrin->Is<ExpandedPrincipal>()) {
> +    auto* expanded = basePrin->As<ExpandedPrincipal>();
> +
> +    nsTArray<nsCOMPtr<nsIPrincipal>>* whitelist;

Ugh.  Why does GetWhiteList not return a pointer-to-const-array???  I guess we have the one consumer that wants to do edit it; we should have a GetMutableWhiteList() for that or something.  Because I was going to suggest using .forget(), and it would have totally "worked"...

Could you file a followup on this?
Attachment #8917152 - Flags: review?(bzbarsky) → review+
Comment on attachment 8917152 [details]
Bug 1407258: Dowgrade document loads with expanded principals to their last sub-principal.

https://reviewboard.mozilla.org/r/188166/#review193320

> Ugh.  Why does GetWhiteList not return a pointer-to-const-array???  I guess we have the one consumer that wants to do edit it; we should have a GetMutableWhiteList() for that or something.  Because I was going to suggest using .forget(), and it would have totally "worked"...
> 
> Could you file a followup on this?

I think it's just because it's an XPIDL-defined noscript attribute, and that's the signature XPIDL generates. Part of the documented contract is that it's not allowed to be changed, so it's not mutable on purpose.

I actually seriously considered changing the signature to `const nsTArray<nsCOMPtr<nsIPrincipal>>& WhiteList()`, but this didn't seem to be the place to do it. I'll file a follow-up, though.
See Also: → 1407428
https://hg.mozilla.org/integration/mozilla-inbound/rev/3aad26ddfedf4f2765b121d0aa9032b55c01076c
Bug 1407258: Dowgrade document loads with expanded principals to their last sub-principal. r=bz
> I think it's just because it's an XPIDL-defined noscript attribute

I'm talking about the actual argument type, not the function itself being const.

> Part of the documented contract is that it's not allowed to be changed

Ah, I misread PrincipalToPrincipalInfo.  In that case, we should definitely only hand out const refs or ptrs to it.  And yes, followup bug.
https://hg.mozilla.org/mozilla-central/rev/3aad26ddfedf
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla58
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.