1306300 - Crash in nsILoadContext::GetOriginAttributes

Status: RESOLVED FIXED

(Core :: DOM: Navigation, defect)

Description

[Honza Bambas (:mayhemer)]

This bug was filed from the Socorro interface and is 
report bp-fdf5e255-a415-4186-8cdd-aa4042160929.
=============================================================

STR, not reliable:
- running Nightly (old heavy profile) as a default browser to open web links
- click an https (not sure about http may reproduce too) link in an external app (for me happens with Thunderbird, clicking a (trustworthy) emailed link)
=> instant crash

STR2, even less reliable:
- DXR page
- click an identifier
- right e.g. the "find callers" link
=> instant crash

nsILoadContext::GetOriginAttributes(mozilla::DocShellOriginAttributes&)
nsScriptSecurityManager::GetLoadContextCodebasePrincipal(nsIURI*, nsILoadContext*, nsIPrincipal**)
NS_InvokeByIndex

Not sure about addon influence, I didn't try safe more (and would like to avoid it unless really necessary.)

Comment 1

[Olli Pettay [:smaug][[e-mail]]]

Isn't the issue that someone in JS is passing null nsILoadContext to GetLoadContextCodebasePrincipal.
http://searchfox.org/mozilla-central/rev/572e74ee991bbfd812766b4524237eb77577a4b1/docshell/base/nsILoadContext.idl#146 is non-virtual.
So we end up crashing in https://hg.mozilla.org/mozilla-central/annotate/66a77b9bfe5d/docshell/base/LoadContext.cpp#l22

Comment 2

[Olli Pettay [:smaug][[e-mail]]]

Attachment: guess fix

This will hopefully fix the crash, and then propagate the exception to JS side so that we can find who is passing the null context.

Comment 3

[Olli Pettay [:smaug][[e-mail]]]

Attachment: better looking

Comment 4

[Honza Bambas (:mayhemer)]

Thanks Olli!

Comment 5

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/14d41b959f3a
null check nsILoadContext in GetLoadContextCodebasePrincipal, r=baku

Comment 6

[Ryan VanderMeulen [:RyanVM]]

Not sure if the frequency is high enough to warrant ESR45 consideration (though AFAICT, the issue goes back at least that far), but I'm thinking we could probably stand to at least take this on Aurora/Beta.

Comment 7

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/14d41b959f3a

Comment 8

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Hi Olli, given the one-line fix, should we uplift to Aurora51/Beta50? I see one instance of this signature on 50.0b1 and 50.0b5.

Comment 9

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8796227 [details] [diff] [review]
better looking

Approval Request Comment
[Feature/regressing bug #]: I think bug 1011024
[User impact if declined]: Crashes if some addon or browser chrome js passes null loadcontext
[Describe test coverage new/current, TreeHerder]: NA
[Risks and why]: Just a null check
[String/UUID change made/needed]: NA

Comment 10

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Comment on attachment 8796227 [details] [diff] [review]
better looking

Crash fix, Aurora51+, Beta50+

Comment 11

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/fc553ef41ab8

Comment 12

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-beta/rev/3162dbd8214f