crash in mozilla::net::HttpBaseChannel::ShouldIntercept()

RESOLVED FIXED in Firefox 44

Status

()

Core
DOM: Service Workers
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: nalexander, Assigned: bkelly)

Tracking

unspecified
mozilla44
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox44 fixed)

Details

Attachments

(1 attachment, 1 obsolete attachment)

(Reporter)

Description

3 years ago
This is a Fennec build, although I don't think that's important.  Stack is below.

What is happening is that mLoadInfo can be null at https://dxr.mozilla.org/mozilla-central/rev/94c804ef40d890b93062826c911755831edb51e4/netwerk/protocol/http/HttpBaseChannel.cpp#2276.

I arranged this in previously working code that implemented a protocol, like the following:

    let channel = Services.io.newChannelFromURI(uri, null).QueryInterface(Ci.nsIHttpChannel);

    channel.owner = principal;
    channel.loadFlags &= ~Ci.nsIChannel.LOAD_REPLACE;
    channel.originalURI = aURI;

That used to work, and now crashes.  It can be fixed by doing:

    let channel = Services.io.newChannelFromURI2(uri,
                                         null,      // aLoadingNode
                                         principal,
                                         null,      // aTriggeringPrincipal
                                         Ci.nsILoadInfo.SEC_ALLOW_CROSS_ORIGIN_DATA_IS_NULL | Ci.nsILoadInfo.SEC_CHROME,
                                         Ci.nsIContentPolicy.TYPE_OTHER);

I can provide more context about the failing code if needed.

13:49 nalexander: #0  0x7aa3e16c in mozilla::net::HttpBaseChannel::ShouldIntercept() () from /Users/nalexander/Mozilla/gecko/objdir-droid2/dist/bin/libxul.so
13:49 nalexander: #1  0x7aa6c080 in mozilla::net::nsHttpChannel::AsyncOpen(nsIStreamListener*, nsISupports*) () from /Users/nalexander/Mozilla/gecko/objdir-droid2/dist/bin/libxul.so
13:49 nalexander: #2  0x7b6cc628 in mozilla::css::Loader::LoadSheet(mozilla::css::SheetLoadData*, mozilla::css::StyleSheetState, bool) () from /Users/nalexander/Mozilla/gecko/objdir-droid2/dist/bin/libxul.so
13:49 nalexander: #3  0x7b6cca82 in mozilla::css::Loader::LoadStyleLink(nsIContent*, nsIURI*, nsAString_internal const&, nsAString_internal const&, bool, mozilla::CORSMode, mozilla::net::ReferrerPolicy, nsAString_internal const&, nsICSSLoaderObserver*, bool*) () from /Users/nalexander/Mozilla/gecko/objdir-droid2/dist/bin/libxul.so
13:49 nalexander: #4  0x7afa82ca in nsStyleLinkElement::DoUpdateStyleSheet(nsIDocument*, mozilla::dom::ShadowRoot*, nsICSSLoaderObserver*, bool*, bool*, bool) ()
13:49 nalexander:   from /Users/nalexander/Mozilla/gecko/objdir-droid2/dist/bin/libxul.so
13:49 nalexander: #5  0x7afa83de in nsStyleLinkElement::UpdateStyleSheet(nsICSSLoaderObserver*, bool*, bool*, bool) () from /Users/nalexander/Mozilla/gecko/objdir-droid2/dist/bin/libxul.so
13:49 nalexander: #6  0x7b5d1c16 in nsXMLContentSink::CloseElement(nsIContent*) () from /Users/nalexander/Mozilla/gecko/objdir-droid2/dist/bin/libxul.so
13:49 nalexander: #7  0x7b5d31c8 in nsXMLContentSink::HandleEndElement(char16_t const*, bool) () from /Users/nalexander/Mozilla/gecko/objdir-droid2/dist/bin/libxul.so
13:49 nalexander: #8  0x7adb1e98 in nsExpatDriver::HandleEndElement(char16_t const*) () from /Users/nalexander/Mozilla/gecko/objdir-droid2/dist/bin/libxul.so
13:49 nalexander: #9  0x7bb2ea7e in doContent () from /Users/nalexander/Mozilla/gecko/objdir-droid2/dist/bin/libxul.so
13:49 nalexander: #10 0x7bb2edec in contentProcessor () from /Users/nalexander/Mozilla/gecko/objdir-droid2/dist/bin/libxul.so
13:49 nalexander: #11 0x7bb2f51a in doProlog () from /Users/nalexander/Mozilla/gecko/objdir-droid2/dist/bin/libxul.so
13:49 nalexander: #12 0x7bb3020e in prologProcessor () from /Users/nalexander/Mozilla/gecko/objdir-droid2/dist/bin/libxul.so
13:49 nalexander: #13 0x7bb30fe2 in MOZ_XML_Parse () from /Users/nalexander/Mozilla/gecko/objdir-droid2/dist/bin/libxul.so
13:49 nalexander: #14 0x7adb1daa in nsExpatDriver::ParseBuffer(char16_t const*, unsigned int, bool, unsigned int*) () from /Users/nalexander/Mozilla/gecko/objdir-droid2/dist/bin/libxul.so
13:49 nalexander: #15 0x7adb51fe in nsExpatDriver::ConsumeToken(nsScanner&, bool&) () from /Users/nalexander/Mozilla/gecko/objdir-droid2/dist/bin/libxul.so
13:49 nalexander: #16 0x00640074 in ?? ()
13:49 nalexander: #17 0x00640074 in ?? ()
(Reporter)

Comment 1

3 years ago
This is fallout from the interception handling added in Bug 1184798, but it might be better triaged into Networking.  bkelly or jdm, can you redirect this?
Flags: needinfo?(josh)
Flags: needinfo?(bkelly)
(Assignee)

Comment 2

3 years ago
I think this just needs to check for nullptr mLoadInfo.  I'll write a patch.
Assignee: nobody → bkelly
Status: NEW → ASSIGNED
Flags: needinfo?(josh)
Flags: needinfo?(bkelly)
(Assignee)

Comment 3

3 years ago
Created attachment 8666863 [details] [diff] [review]
HttpBaseChannel::ShouldIntercept() should not assume every channel has a LoadInfo. r=jdm

This makes it so we never intercept any channel without a LoadInfo object.  I think this is reasonable as all content should definitely have a LoadInfo.

An alternative would be to fallback to TYPE_OTHER if mLoadInfo is nullptr, but lets try this first.

Nick, does this fix the problem for you?

https://treeherder.mozilla.org/#/jobs?repo=try&revision=861c1b44e9b2
Attachment #8666863 - Flags: review?(josh)
Attachment #8666863 - Flags: feedback?(nalexander)
(Assignee)

Updated

3 years ago
Duplicate of this bug: 1209694
Thanks Ben, please note that there is another access to mLoadInfo within nsJarChannel:
> http://hg.mozilla.org/mozilla-central/rev/5061c446e169
which also doesn't do the nullcheck - can you please update that as well?

Updated

3 years ago
Blocks: 1209927
(Reporter)

Comment 6

3 years ago
> Nick, does this fix the problem for you?

bkelly: I don't really have time to investigate this further.  I'm sure avoiding the mLoadInfo == null is sufficient -- I was in the debugger, watching it fail -- but I'm mostly concerned about whether the method itself should be deprecated.
(Reporter)

Updated

3 years ago
Attachment #8666863 - Flags: feedback?(nalexander)
(In reply to Christoph Kerschbaumer [:ckerschb] from comment #5)
> Thanks Ben, please note that there is another access to mLoadInfo within
> nsJarChannel:
> > http://hg.mozilla.org/mozilla-central/rev/5061c446e169
> which also doesn't do the nullcheck - can you please update that as well?

Ben, just making sure you have seen what I point out in comment 5 before you land the patch. Thanks!
Flags: needinfo?(bkelly)
(Assignee)

Comment 8

3 years ago
Created attachment 8668005 [details] [diff] [review]
HttpBaseChannel::ShouldIntercept() should not assume every channel has a LoadInfo. r=ckerschb
Attachment #8666863 - Attachment is obsolete: true
Attachment #8666863 - Flags: review?(josh)
Flags: needinfo?(bkelly)
Attachment #8668005 - Flags: review?(mozilla)
Comment on attachment 8668005 [details] [diff] [review]
HttpBaseChannel::ShouldIntercept() should not assume every channel has a LoadInfo. r=ckerschb

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

Great - thanks!
Attachment #8668005 - Flags: review?(mozilla) → review+
https://hg.mozilla.org/mozilla-central/rev/bc0c60cd2fb1
Status: ASSIGNED → RESOLVED
Last Resolved: 3 years ago
status-firefox44: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla44
You need to log in before you can comment on or make changes to this bug.