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

RESOLVED FIXED in Firefox 48

Status

()

defect
--
critical
RESOLVED FIXED
3 years ago
3 months ago

People

(Reporter: philipp, Assigned: tnguyen)

Tracking

({crash})

47 Branch
mozilla50
x86
All
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox47 affected, firefox48 fixed, firefox49 fixed, firefox-esr45 affected, firefox50 fixed)

Details

(crash signature)

Attachments

(1 attachment, 1 obsolete attachment)

Reporter

Description

3 years ago
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

Updated

3 years ago
Assignee: nobody → tnguyen
Flags: needinfo?(tnguyen)
Whiteboard: btpp-followup-2016-07-05
Assignee

Comment 3

3 years ago
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
Assignee

Comment 4

3 years ago
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.
Assignee

Comment 9

3 years ago
Posted 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)
Assignee

Comment 11

3 years ago
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+
Assignee

Updated

3 years ago
Keywords: checkin-needed

Comment 14

3 years ago
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

Comment 15

3 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/4069c4e3a0da
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla50
Thomas, this patch needs uplifting, right? Please fill out the forms and request uplift.
Flags: needinfo?(tnguyen)
Assignee

Comment 17

3 years ago
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.