1066718 - crash in nsScriptSecurityManager::AddSitesToFileURIWhitelist(nsCString const&)

Status: VERIFIED FIXED

(Core :: Security: CAPS, defect, critical)

Description

[Robert Kaiser]

This bug was filed from the Socorro interface and is 
report bp-dbe0ca2c-53cc-4f07-82a9-a8def2140910.
=============================================================

This is a startup crash of the main process with e10s enabled that is rising significantly now that we nudge people to try e10s.

Find stats and more reports at https://crash-stats.mozilla.com/report/list?product=Firefox&signature=nsScriptSecurityManager%3A%3AAddSitesToFileURIWhitelist%28nsCString+const%26%29

Comment 1

[Robert Kaiser]

Sorry, this is actually mostly happening in the *content* process, but some of those are also in the main "browser" process.

Comment 2

[:dmajor]

We're calling ExtractScheme on a null sIOService.

Comment 3

[Bobby Holley (:bholley)]

Yeah, this basically crashes for everyone who has the caps fileURI prefs set up. The problem is that the test sets the prefs dynamically long after startup, and so we don't hit this crash.

We'll need to take this on all the branches where we uplifted bug 1061136.

Comment 4

[Bobby Holley (:bholley)]

https://tbpl.mozilla.org/?tree=Try&rev=b9597630db49

Comment 5

[Bobby Holley (:bholley)]

Attachment: Get sIOService before invoking ReadPrefs. v1

Comment 6

[Bobby Holley (:bholley)]

Comment on attachment 8489992 [details] [diff] [review]
Get sIOService before invoking ReadPrefs. v1

This is regression from bug 1061136 that will cause it to crash for pretty much everyone who uses this machinery. This fix needs to follow  that one.

Comment 7

[Lawrence Mandel [:lmandel] (use needinfo)]

Comment 0 says that this crash only occurs when e10s is enabled. e10s is currently disabled by default everywhere. This is a trivial fix but still should only be uplifted where necessary. I'm happy to uplift to aurora and will accept on beta if we're asking people to enable e10s there. I don't think we need to uplift to esr31, on which we will never enable e10s and on which the user base has no expectation of bleeding edge features.

Comment 8

[Bobby Holley (:bholley)]

(In reply to Lawrence Mandel [:lmandel] from comment #7)
> Comment 0 says that this crash only occurs when e10s is enabled. e10s is
> currently disabled by default everywhere. This is a trivial fix but still
> should only be uplifted where necessary. I'm happy to uplift to aurora and
> will accept on beta if we're asking people to enable e10s there. I don't
> think we need to uplift to esr31, on which we will never enable e10s and on
> which the user base has no expectation of bleeding edge features.

I don't understand why this would be e10s-only. By my reading of the code, it should be happening to anyone that has the CAPS prefs set. I can investigate this if you like, but the fix is super-safe (just grabbing a pointer to the global IOService one line earlier), so I think we could also just uplift it and move on.

Comment 9

[Robert Kaiser]

(In reply to Lawrence Mandel [:lmandel] from comment #7)
> Comment 0 says that this crash only occurs when e10s is enabled.

So, I thought this would be related due to when it started spiking but I took the wrong conclusions to the data I was seeing. This also happens where e10s is disabled, the way our queries work did confuse me.

As Bobby says that it's caused by using what bug 1061136 introduces, we should see to land this everywhere we land(ed) that other patch as well.

Comment 10

[Boris Zbarsky [:bzbarsky, bz on IRC]]

Comment on attachment 8489992 [details] [diff] [review]
Get sIOService before invoking ReadPrefs. v1

r=me

Comment 11

[Bobby Holley (:bholley)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/82824b25afee

Comment 12

[Wes Kocher (:KWierso)]

https://hg.mozilla.org/mozilla-central/rev/82824b25afee

Comment 13

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/d7ad9b5167d8
https://hg.mozilla.org/releases/mozilla-beta/rev/262de5944a01
https://hg.mozilla.org/releases/mozilla-esr31/rev/6f935c70e194

Comment 14

[Florin Mezei, QA (:FlorinMezei)]

Closing this bug since Socorro [1] shows no more crashes on 33 (after Beta 4), 34 and 35 for the past 4 weeks.

[1] - https://crash-stats.mozilla.com/report/list?product=Firefox&range_unit=days&range_value=28&signature=nsScriptSecurityManager%3A%3AAddSitesToFileURIWhitelist%28nsCString+const%26%29