Closed Bug 1279494 Opened 4 years ago Closed 4 years ago

Crash in mozilla::net::HttpBaseChannel::SetReferrerWithPolicy

Categories

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

47 Branch
x86
All
defect
Not set
critical

Tracking

()

RESOLVED FIXED
mozilla50
Tracking Status
firefox47 --- affected
firefox48 --- fixed
firefox49 --- fixed
firefox-esr45 --- affected
firefox50 --- fixed

People

(Reporter: philipp, Assigned: tnguyen)

References

Details

(Keywords: crash)

Crash Data

Attachments

(1 file, 1 obsolete file)

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.
Franziskus, do you have any thoughts here?
Flags: needinfo?(franziskuskiefer)
Whiteboard: btpp-followup-2016-07-05
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?
Flags: needinfo?(franziskuskiefer) → needinfo?(tnguyen)
Assignee: nobody → tnguyen
Flags: needinfo?(tnguyen)
Whiteboard: btpp-followup-2016-07-05
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
Ni Tanvi, Christoph,
Do you have any thoughts?
Flags: needinfo?(tanvi)
Flags: needinfo?(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.
Flags: needinfo?(ckerschb)
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
Flags: needinfo?(tanvi) → needinfo?(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.
Flags: needinfo?(ckerschb)
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.
Attached patch Null check principal (obsolete) — Splinter Review
Nice. Thanks Tanvi and Christoph for your idea.
Attachment #8767596 - Flags: review?(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->()
}
Attachment #8767596 - Flags: review?(ckerschb)
Agree, I am a bit overdoing it :). Thanks for your review. Updated the patch.
Attachment #8767596 - Attachment is obsolete: true
Attachment #8767619 - Flags: review?(ckerschb)
Comment on attachment 8767619 [details] [diff] [review]
Null check triggeringPrincipal

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

Awesome, thanks, r=me
Attachment #8767619 - Flags: review?(ckerschb) → review+
Keywords: checkin-needed
Pushed by cbook@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/4069c4e3a0da
Fix crash in SetReferrerWithPolicy, add null check the triggeringPrincipal. r=ckerschb
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/4069c4e3a0da
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla50
Thomas, this patch needs uplifting, right? Please fill out the forms and request uplift.
Flags: needinfo?(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
Flags: needinfo?(tnguyen)
Attachment #8767619 - Flags: approval-mozilla-beta?
Attachment #8767619 - Flags: approval-mozilla-aurora?
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.
Attachment #8767619 - Flags: approval-mozilla-beta?
Attachment #8767619 - Flags: approval-mozilla-beta+
Attachment #8767619 - Flags: approval-mozilla-aurora?
Attachment #8767619 - Flags: approval-mozilla-aurora+
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
Duplicate of this bug: 1275247
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.