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

Status: RESOLVED FIXED

(Core :: DOM: Service Workers, defect)

Description

[Nick Alexander :nalexander [he/him]]

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 ?? ()

Comment 1

[Nick Alexander :nalexander [he/him]]

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?

Comment 2

[Ben Kelly [:bkelly, not reviewing]]

I think this just needs to check for nullptr mLoadInfo.  I'll write a patch.

Comment 3

[Ben Kelly [:bkelly, not reviewing]]

Attachment: 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

Comment 5

[Christoph Kerschbaumer [:ckerschb]]

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?

Comment 6

[Nick Alexander :nalexander [he/him]]

> 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.

Comment 7

[Christoph Kerschbaumer [:ckerschb]]

(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!

Comment 8

[Ben Kelly [:bkelly, not reviewing]]

Attachment: HttpBaseChannel::ShouldIntercept() should not assume every channel has a LoadInfo. r=ckerschb

Comment 9

[Christoph Kerschbaumer [:ckerschb]]

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!

Comment 10

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/bc0c60cd2fb1

Comment 11

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/bc0c60cd2fb1