Recursive initialization of nsLayoutStatics when capability.policy.policynames is set

RESOLVED FIXED in Firefox 47

Status

()

Core
Security: CAPS
P1
normal
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: mccr8, Assigned: Gabor Krizsanits (INACTIVE))

Tracking

({crash})

Trunk
mozilla47
crash
Points:
---

Firefox Tracking Flags

(e10s+, firefox47 fixed)

Details

(Whiteboard: dom-triaged btpp-triage-active, crash signature)

Attachments

(1 attachment)

(Reporter)

Description

2 years ago
This is the #14 top crash for Nightly content processes.

During InitXPCOM2, nsFactoryEntry::GetFactory() gets called, which calls into nsScriptSecurityManager::AddSitesToFileURIWhitelist(), which calls into NS_NewURI(), which somehow ends up back in the component manager, and we hit an assertion against reentry.

https://crash-stats.mozilla.com/report/index/4a5386a3-c57d-4685-8cba-7e9a22160210
I'll bet this is crashing because the prefs this codepath follows:

http://hg.mozilla.org/mozilla-central/annotate/0ecd7d72f232/caps/nsScriptSecurityManager.cpp#l1427

aren't set in most normal profiles.

Requesting e10s tracking as this is going to hurt people who have these prefs set for...whatever reason.
tracking-e10s: --- → ?
(Reporter)

Updated

2 years ago
Component: XPCOM → Security: CAPS
Hmm.  Why is getting a protocol handler ending up in the layout module?  Or is this just a general "find the module that has this protocol handler, so load them all" thing?
Flags: needinfo?(benjamin)

Comment 3

2 years ago
It's definitely not loading all of them. It's hard to say more without knowing the actual protocol that's failing, but here are the protocol handlers that are part of layout, as far as I can tell by reading nsLayoutModule.cpp:

blob: mediastream: mediasourceuri: moz-fonttable:
Flags: needinfo?(benjamin)
Who can fix this?
Priority: -- → P1
Whiteboard: dom-triaged btpp-triage-followup-2016-02-19
Hmm.  OK, I guess maybe people have their prefs set to mention those...

I guess the real question is where we can initialize this stuff without causing layout module reentry.
Assignee: nobody → gkrizsanits
tracking-e10s: ? → +

Comment 6

2 years ago
It appears these prefs get set by script security related addons. An addon search turns up a number of hits for 'capability.policy.policynames'.
(Reporter)

Updated

2 years ago
Summary: Recursive initialization of nsLayoutStatics → Recursive initialization of nsLayoutStatics when capability.policy.policynames is set
Whiteboard: dom-triaged btpp-triage-followup-2016-02-19 → dom-triaged btpp-triage-active
(Assignee)

Comment 7

2 years ago
(In reply to Boris Zbarsky [:bz] from comment #5)
> Hmm.  OK, I guess maybe people have their prefs set to mention those...
> 
> I guess the real question is where we can initialize this stuff without
> causing layout module reentry.

Probably before http://hg.mozilla.org/mozilla-central/annotate/0ecd7d72f232/xpcom/build/XPCOMInit.cpp#l724
If I let say call a getProtocolHandler there for mediastream: then using capability.policy.localfilelinks.sites will no longer crash, but we should do it for all the protocols in nsLayoutModule.cpp (and hope that other protocols an URI can have won't cause any similar troubles).

Alternatives:

- break up nsLayoutModule.cpp somehow and put the protocol handlers somewhere else

- nsScriptSecurityManager could store initially mFileURIWhitelist entries as strings and we could turn them into URIs later lazily when we need to use them. I would do this.

- Do we need this at all in a content process? If we do, is it possible to just send the list of URIs with IPC at the right moment of content process initialization (like we do for domain policies already)?
(Assignee)

Comment 8

2 years ago
Any preference which version would you like to review? :)
Flags: needinfo?(bobbyholley)
(In reply to Gabor Krizsanits [:krizsa :gabor] from comment #7)
> (In reply to Boris Zbarsky [:bz] from comment #5)
> > Hmm.  OK, I guess maybe people have their prefs set to mention those...
> > 
> > I guess the real question is where we can initialize this stuff without
> > causing layout module reentry.
> 
> Probably before
> http://hg.mozilla.org/mozilla-central/annotate/0ecd7d72f232/xpcom/build/
> XPCOMInit.cpp#l724
> If I let say call a getProtocolHandler there for mediastream: then using
> capability.policy.localfilelinks.sites will no longer crash, but we should
> do it for all the protocols in nsLayoutModule.cpp (and hope that other
> protocols an URI can have won't cause any similar troubles).
> 
> Alternatives:
> 
> - break up nsLayoutModule.cpp somehow and put the protocol handlers
> somewhere else
> 
> - nsScriptSecurityManager could store initially mFileURIWhitelist entries as
> strings and we could turn them into URIs later lazily when we need to use
> them. I would do this.
> 

This means we need to reify on every call to CheckLoadURIWithPrincipal. That's why I didn't do it that way.

> - Do we need this at all in a content process? If we do, is it possible to
> just send the list of URIs with IPC at the right moment of content process
> initialization (like we do for domain policies already)?

The source of the data (parent process vs prefs) isn't really the issue here. It's a question of which moment we initialize it. We could do some sort of lazy-init, but I've been explicitly moving us away from that because of its tendency to cause bugs like this.

In general, the only way to avoid these kinds of dependency-resolution bugs is to hardcode the startup order, so I'd be for something like that. Split out the protocol handler stuff somehow, and then explicitly initialize that (and then the SSM) from XPCOMInit, rather than letting it all try to sort itself out during layout init.
Flags: needinfo?(bobbyholley)
(Assignee)

Comment 10

2 years ago
(In reply to Bobby Holley (busy) from comment #9)
> > - nsScriptSecurityManager could store initially mFileURIWhitelist entries as
> > strings and we could turn them into URIs later lazily when we need to use
> > them. I would do this.
> > 
> 
> This means we need to reify on every call to CheckLoadURIWithPrincipal.
> That's why I didn't do it that way.
> 

I honestly have no idea what reify might mean in this context, could you elaborate?
You're worry is that every time someone introduces a new CheckLoadURIWithPrincipal
call there is a risk we end up in the same situation where we are now?

> In general, the only way to avoid these kinds of dependency-resolution bugs
> is to hardcode the startup order, so I'd be for something like that. Split
> out the protocol handler stuff somehow, and then explicitly initialize that
> (and then the SSM) from XPCOMInit, rather than letting it all try to sort
> itself out during layout init.

Alright I'll try that.
(In reply to Gabor Krizsanits [:krizsa :gabor] from comment #10)
> (In reply to Bobby Holley (busy) from comment #9)
> > > - nsScriptSecurityManager could store initially mFileURIWhitelist entries as
> > > strings and we could turn them into URIs later lazily when we need to use
> > > them. I would do this.
> > > 
> > 
> > This means we need to reify on every call to CheckLoadURIWithPrincipal.
> > That's why I didn't do it that way.
> > 
> 
> I honestly have no idea what reify might mean in this context, could you
> elaborate?
> You're worry is that every time someone introduces a new
> CheckLoadURIWithPrincipal
> call there is a risk we end up in the same situation where we are now?


No, I mean that the security check we perform in CheckLoadURIWithPrincipal (which gets called often) requires that we have URIs for the values in mFileURIWhitelist. So if mFileURIWhitelist were to store strings, we'd need to convert those strings to URIs each time that CheckLoadURIWithPrincipal was called, rather than just once (when we set it up).

> 
> > In general, the only way to avoid these kinds of dependency-resolution bugs
> > is to hardcode the startup order, so I'd be for something like that. Split
> > out the protocol handler stuff somehow, and then explicitly initialize that
> > (and then the SSM) from XPCOMInit, rather than letting it all try to sort
> > itself out during layout init.
> 
> Alright I'll try that.

Cool. :-)
(Assignee)

Comment 12

2 years ago
(In reply to Bobby Holley (busy) from comment #11)
> No, I mean that the security check we perform in CheckLoadURIWithPrincipal
> (which gets called often) requires that we have URIs for the values in
> mFileURIWhitelist. So if mFileURIWhitelist were to store strings, we'd need
> to convert those strings to URIs each time that CheckLoadURIWithPrincipal
> was called, rather than just once (when we set it up).

Oh, then you got my idea wrong, by lazily I meant that I do the conversion only once,
the first time when it's needed, then keep the list of URIs and throw away the list of strings.

Anyway, I'm trying to stick to the splitting out the protocols from LayoutModule approach, that would be less hacky. My only concern is GetJSStackForBlob but I think we can get away with it and just put an assertion there, since it should be only used from CreateObjectURL and by that time xpconnect should be up and ready already.
(In reply to Gabor Krizsanits [:krizsa :gabor] from comment #12)
> (In reply to Bobby Holley (busy) from comment #11)
> > No, I mean that the security check we perform in CheckLoadURIWithPrincipal
> > (which gets called often) requires that we have URIs for the values in
> > mFileURIWhitelist. So if mFileURIWhitelist were to store strings, we'd need
> > to convert those strings to URIs each time that CheckLoadURIWithPrincipal
> > was called, rather than just once (when we set it up).
> 
> Oh, then you got my idea wrong, by lazily I meant that I do the conversion
> only once,
> the first time when it's needed, then keep the list of URIs and throw away
> the list of strings.

If we do that, we might as well just not do the strings at all, and instead just lazily read the pref values when we need them. But that gets back to the whole business about how lazy-initialization is a short-sighted solution to this sort of problem.

> Anyway, I'm trying to stick to the splitting out the protocols from
> LayoutModule approach, that would be less hacky. My only concern is
> GetJSStackForBlob but I think we can get away with it and just put an
> assertion there, since it should be only used from CreateObjectURL and by
> that time xpconnect should be up and ready already.

Yeah that sounds fine. With startup ordering it's really just "find an explicit order to initialize things that doesn't crash at startup". ;-)
(Assignee)

Comment 15

2 years ago
Created attachment 8725625 [details] [diff] [review]
Splitting out protocol handlers from nsLayoutModule. v1

Looks green.
Attachment #8725625 - Flags: review?(bobbyholley)
Comment on attachment 8725625 [details] [diff] [review]
Splitting out protocol handlers from nsLayoutModule. v1

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

This isn't quite what I had in mind, but happy to go with it if it solves the problem.
Attachment #8725625 - Flags: review?(bobbyholley) → review+
(Assignee)

Comment 18

2 years ago
(In reply to Bobby Holley (busy) from comment #16)
> This isn't quite what I had in mind, but happy to go with it if it solves
> the problem.

Sorry about that, but I figured that there is nothing to init for these protocols really. Their first instantiation just triggered LayoutModule init (for no good reason), which triggered xpconnect init. So the only way to "init" protocols were to instantiate them, but then if I start with protocol handler A and then from xpconnect we need protocol handler B we end up in the same situation, so I had to split out the protocol handlers in their separate module anyway. And after I did that there was nothing else to be done really, so I think this is the cleanest way to do it.
(In reply to Gabor Krizsanits [:krizsa :gabor] from comment #18)
> (In reply to Bobby Holley (busy) from comment #16)
> > This isn't quite what I had in mind, but happy to go with it if it solves
> > the problem.
> 
> Sorry about that, but I figured that there is nothing to init for these
> protocols really. Their first instantiation just triggered LayoutModule init
> (for no good reason), which triggered xpconnect init. So the only way to
> "init" protocols were to instantiate them, but then if I start with protocol
> handler A and then from xpconnect we need protocol handler B we end up in
> the same situation, so I had to split out the protocol handlers in their
> separate module anyway. And after I did that there was nothing else to be
> done really, so I think this is the cleanest way to do it.

I just meant that I was hoping that we would split out SSM initialization into an explicit call in XPCOMInit (along with explicit calls for anything it depends on). But given that this is now unrelated to what you're doing, let's forget about that for now.

Comment 20

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/76fa7dac8282
Status: NEW → RESOLVED
Last Resolved: 2 years ago
status-firefox47: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla47
You need to log in before you can comment on or make changes to this bug.