1279494 - Crash in mozilla::net::HttpBaseChannel::SetReferrerWithPolicy

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[[:philipp]]

This bug was filed from the Socorro interface and is 
report bp-4844f546-1d0b-48e8-b465-c2fff2160608.
=============================================================
Crashing Thread (0)
Frame 	Module 	Signature 	Source
0 	xul.dll 	mozilla::net::HttpBaseChannel::SetReferrerWithPolicy(nsIURI*, unsigned int) 	netwerk/protocol/http/HttpBaseChannel.cpp:1388
1 	xul.dll 	mozilla::net::HttpBaseChannel::SetupReplacementChannel(nsIURI*, nsIChannel*, bool, unsigned int) 	netwerk/protocol/http/HttpBaseChannel.cpp:2764
2 	xul.dll 	mozilla::net::nsHttpChannel::SetupReplacementChannel(nsIURI*, nsIChannel*, bool, unsigned int) 	netwerk/protocol/http/nsHttpChannel.cpp:4603
3 	xul.dll 	mozilla::net::nsHttpChannel::StartRedirectChannelToURI(nsIURI*, unsigned int) 	netwerk/protocol/http/nsHttpChannel.cpp:2025
4 	xul.dll 	mozilla::net::nsHttpChannel::StartRedirectChannelToHttps() 	netwerk/protocol/http/nsHttpChannel.cpp:1981
5 	xul.dll 	mozilla::net::nsHttpChannel::HandleAsyncRedirectChannelToHttps() 	netwerk/protocol/http/nsHttpChannel.cpp:1965
6 	xul.dll 	nsRunnableMethodImpl<void ( mozilla::net::HttpChannelChild::*)(void), 1>::Run() 	xpcom/glue/nsThreadUtils.h:870
7 	xul.dll 	nsThread::ProcessNextEvent(bool, bool*) 	xpcom/threads/nsThread.cpp:994
8 	xul.dll 	mozilla::ipc::MessagePump::Run(base::MessagePump::Delegate*) 	ipc/glue/MessagePump.cpp:95
9 	xul.dll 	MessageLoop::RunHandler() 	ipc/chromium/src/base/message_loop.cc:227
10 	xul.dll 	nsThreadManager::GetCurrentThread() 	xpcom/threads/nsThreadManager.cpp:220
11 	xul.dll 	nsAppShell::Run() 	widget/windows/nsAppShell.cpp:262
12 	xul.dll 	nsAppStartup::Run() 	toolkit/components/startup/nsAppStartup.cpp:281
13 	xul.dll 	XREMain::XRE_mainRun() 	toolkit/xre/nsAppRunner.cpp:4337
14 	xul.dll 	XREMain::XRE_main(int, char** const, nsXREAppData const*) 	toolkit/xre/nsAppRunner.cpp:4434
15 	xul.dll 	XRE_main 	toolkit/xre/nsAppRunner.cpp:4540

this cross-platform but low level crash has been around in firefox for a while and looks like it is related to the changes from bug 704320.

Comment 1

[Andrew Overholt [:overholt]]

Franziskus, do you have any thoughts here?

Comment 2

[Franziskus Kiefer [:franziskus]]

A quick fix would probably be to check that mLoadInfo->TriggeringPrincipal() != null (which I guess is the case here and we should do anyway). But if that fixes the problem we have to find out why there is no triggering principle.
We need a test case first. Looks like STS redirects should be able to trigger this.
Thomas, could you give this a try?

Comment 3

[Thomas Nguyen (:tnguyen)]

Hmm, imho, the case that may have a null mLoadInfo->TriggeringPrincipal() is some addons created a channel using the deprecated API. 

I see the crash is in Firefox 47, and I could't reproduce the case in the current implementation.(I am using mac and linux platforms). In current version, assertions in the null case are added within nsIOService which all channels go through, and, triggeringPrincipal is set to systemPrincipal in the fallback. TriggeringPrincipal should not be null, never.
Perhaps, we should add the condition mLoadInfo->TriggeringPrincipal() != null, but I am still not very sure how we handle the if the case happens. Assuming cross-origin loading probably is a right thing as [1].
[1] https://dxr.mozilla.org/mozilla-central/source/netwerk/protocol/http/HttpBaseChannel.cpp#1435

Comment 4

[Thomas Nguyen (:tnguyen)]

Ni Tanvi, Christoph,
Do you have any thoughts?

Comment 5

[Christoph Kerschbaumer [:ckerschb]]

(In reply to Thomas Nguyen[:tnguyen] from comment #3)
> Hmm, imho, the case that may have a null mLoadInfo->TriggeringPrincipal() is
> some addons created a channel using the deprecated API. 

Well, not the deprecated API, because then we wouldn't have a loadInfo object at all, but rather use the API incorrectly. Within debug builds we have assertions everywhere, but in release builds we have nothing to bail out early in case the triggeringPrincipal (or also LoadingPrincipal) are set to null.

> Perhaps, we should add the condition mLoadInfo->TriggeringPrincipal() !=
> null, but I am still not very sure how we handle the if the case happens.

Sure, we could do that, but I see we are using loadinfo->TriggeringPrincipal() in other places within our codebase as well, probably we should null check those as well.

For now, if this is a top crash then just surround the code in question with a |if (triggeringPrincipal) |. It's an easy fix and easy to uplift. Given that we can't reproduce that's probably the best way forward with this bug. I am happy to r+ such a patch.

Comment 6

[Tanvi Vyas[:tanvi]]

Christoph - we assert that triggeringPrincipal exists for all loads.  If triggeringPrincipal doesn't exist when loadinfo is created, we set it to loadingPrincipal.  Hence, I'm not sure why we are getting a null triggeringPrincipal.  Won't this behavior exist even if an addon creates a loadInfo?  If the triggeringPrincipal is not there, we will automatically set it to the loadingPrincipal in the loadInfo constructor, right? http://searchfox.org/mozilla-central/source/netwerk/base/LoadInfo.cpp#45

Comment 7

[Christoph Kerschbaumer [:ckerschb]]

(In reply to Tanvi Vyas - behind on reviews [:tanvi] from comment #6)
> Christoph - we assert that triggeringPrincipal exists for all loads.  If
> triggeringPrincipal doesn't exist when loadinfo is created, we set it to
> loadingPrincipal.  Hence, I'm not sure why we are getting a null
> triggeringPrincipal.  Won't this behavior exist even if an addon creates a
> loadInfo?  If the triggeringPrincipal is not there, we will automatically
> set it to the loadingPrincipal in the loadInfo constructor, right?
> http://searchfox.org/mozilla-central/source/netwerk/base/LoadInfo.cpp#45

Tanvi, assertions disappear in production code, so we have no guarantee that the triggeringPrincipal or even the loadingPrincipal is non null. A crazy addon could even create a channel using a correct loadInfo and then call channel.setLoadInfo() during runtime. No guarantees there.

I think moving forward, let's just null-check the triggeringPrincipal for now. I suppose there is nothing else we can do given that we don't have  a testcase at hand.

Comment 8

[Tanvi Vyas[:tanvi]]

We could null check the triggeringPrincipal.  If it is null, we could null check the loadingPrincipal.  If the loadingPrincipal is not null, we can use that.  If both are null, then we try and continue.

Comment 9

[Thomas Nguyen (:tnguyen)]

Attachment: Null check principal

Nice. Thanks Tanvi and Christoph for your idea.

Comment 10

[Christoph Kerschbaumer [:ckerschb]]

Comment on attachment 8767596 [details] [diff] [review]
Null check principal

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

::: netwerk/protocol/http/HttpBaseChannel.cpp
@@ +1420,5 @@
>    if (mLoadInfo) {
> +    nsCOMPtr<nsIPrincipal> principal = mLoadInfo->TriggeringPrincipal();
> +    if (principal) {
> +      principal->GetURI(getter_AddRefs(triggeringURI));
> +    }

I think we should not overdue it in this bug and just fix the crash. In other words, only update HttpBaseChannel and discard all the other changes. We have to investigate what's going on, but for now only fix the topcrash.

nit: please use the name triggeringPrincipal instead of principal.

nsCOMPtr<nsIPrincipal> triggeringPrincipal = mLoadInfo->TriggeringPrincipal();
if (triggeringPrincipal) {
  triggeringPrincipal->()
}

Comment 11

[Thomas Nguyen (:tnguyen)]

Attachment: Null check triggeringPrincipal

Agree, I am a bit overdoing it :). Thanks for your review. Updated the patch.

Comment 12

[Christoph Kerschbaumer [:ckerschb]]

Comment on attachment 8767619 [details] [diff] [review]
Null check triggeringPrincipal

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

Awesome, thanks, r=me

Comment 13

[Thomas Nguyen (:tnguyen)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=a5ae5e05ed2f

Comment 14

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/4069c4e3a0da
Fix crash in SetReferrerWithPolicy, add null check the triggeringPrincipal. r=ckerschb

Comment 15

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/4069c4e3a0da

Comment 16

[Christoph Kerschbaumer [:ckerschb]]

Thomas, this patch needs uplifting, right? Please fill out the forms and request uplift.

Comment 17

[Thomas Nguyen (:tnguyen)]

Comment on attachment 8767619 [details] [diff] [review]
Null check triggeringPrincipal

Approval Request Comment
Comment 0, this is a top crash
[Feature/regressing bug #]:
Bug 704320 
[User impact if declined]:
Crash may occur when users installed some certain addons and access certain sites support HSTS
[Describe test coverage new/current, TreeHerder]:
Manually test.
[Risks and why]: 
Low risk. Reason: only add a null check
[String/UUID change made/needed]:
No

Comment 18

[Gerry Chang [:gchang]]

Comment on attachment 8767619 [details] [diff] [review]
Null check triggeringPrincipal

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

This patch fixes a crash. Take it in 48 beta 6 and aurora.

Comment 19

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/4f9464b013a5

Comment 20

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-beta/rev/48a5986567e5

Comment 21

[BugBot [:suhaib / :marco/ :calixte]]

Crash volume for signature 'mozilla::net::HttpBaseChannel::SetReferrerWithPolicy':
 - nightly (version 50): 31 crashes from 2016-06-06.
 - aurora  (version 49): 27 crashes from 2016-06-07.
 - beta    (version 48): 193 crashes from 2016-06-06.
 - release (version 47): 1419 crashes from 2016-05-31.
 - esr     (version 45): 3 crashes from 2016-04-07.

Crash volume on the last weeks:
             Week N-1   Week N-2   Week N-3   Week N-4   Week N-5   Week N-6   Week N-7
 - nightly          0          0          1         25          0          2          3
 - aurora           0          0          3          7          9          8          0
 - beta             3          4         18         43         63         53          9
 - release        191        162        193        218        252        251         75
 - esr              0          0          1          2          0          0          0

Affected platforms: Windows, Mac OS X, Linux