1350680 - Crash in nsCOMPtr<T>::nsCOMPtr<T> | mozilla::net::LoadInfo::LoadInfo

Status: RESOLVED FIXED

(WebExtensions :: General, defect)

Description

[Calixte Denizet (:calixte)]

This bug was filed from the Socorro interface and is 
report bp-748785f7-1311-4451-a9d9-0effc2170325.
=============================================================

There are 19 crashes (11 from the same installation) in nightly 55 with buildid 20170325030203. In analyzing the backtrace, patch [1], to fix bug 1348442, may be the cause.

[1] https://hg.mozilla.org/mozilla-central/rev?node=dda3d09783a5b5fe61c6591aa53d77105147b123

Comment 1

[Kris Maglione [:kmag]]

Ah. Only the synchronous branch checked for null loadinfo in the old code.

But I wonder how we're winding up without loadinfo in that call chain. There's an assertion a couple of frames up:

https://hg.mozilla.org/mozilla-central/annotate/65b0ac174753/netwerk/base/nsIOService.cpp#l886

So it looks like something strange is going on.

Christoph, any idea how we're winding up without loadinfo here?

Comment 2

[Kris Maglione [:kmag]]

Ah, I missed:

        rv = handler->NewChannel2(aURI, aLoadInfo, getter_AddRefs(channel));
        // if calling newChannel2() fails we try to fall back to
        // creating a new channel by calling NewChannel().
        if (NS_FAILED(rv)) {
            rv = handler->NewChannel(aURI, getter_AddRefs(channel));

So I guess it's a question of why NewChannel failed in the first place. ... and why we do that fallback.

Comment 4

[Christoph Kerschbaumer [:ckerschb]]

(In reply to Kris Maglione [:kmag] from comment #2)
> Ah, I missed:
> 
>         rv = handler->NewChannel2(aURI, aLoadInfo, getter_AddRefs(channel));
>         // if calling newChannel2() fails we try to fall back to
>         // creating a new channel by calling NewChannel().
>         if (NS_FAILED(rv)) {
>             rv = handler->NewChannel(aURI, getter_AddRefs(channel));
> 
> So I guess it's a question of why NewChannel failed in the first place. ...
> and why we do that fallback.

Hey Kris, in the old code [1] there was a null check for aLoadInfo which seems to be responsible for the crash. So I suppose as a first measure we need to bring that back. Regarding comment 2, addons can register their on protocolHandlers and hence potentially not implemented NewChannel2(), hence we added to fallback to not break those addons. Within Gecko we have assertions to make sure we are not breaking things, but we have no control over those addons. I don't know what the Socorro interface is, but presumably it creates a new channel without a loadInfo attached - hence the crash.

[1] https://hg.mozilla.org/mozilla-central/rev?node=dda3d09783a5b5fe61c6591aa53d77105147b123#l1.12

Comment 5

[Kris Maglione [:kmag]]

Can we change the failure check to explicitly only do the fallback for NS_ERROR_NOT_IMPLEMENTED, or the failure code for unimplemented JS functions? I'll fix the null check either way, but I'd really rather not have the fallback in this case, especially since it involves trying to open a channel both times.

I think we're winding up here when trying to open a channel for a nonexistent URL, and then falling back to NewChannel rather than NewChannel2 when that fails. I hadn't considered the possibility that we'd be trying to open these channels without loadInfo at this point.

Comment 6

[Kris Maglione [:kmag]]

(Soccoro is the internal name for crashstats, by the way. Comment 0 was referring to the crash reports.)

Comment 7

[Christoph Kerschbaumer [:ckerschb]]

(In reply to Kris Maglione [:kmag] from comment #5)
> Can we change the failure check to explicitly only do the fallback for
> NS_ERROR_NOT_IMPLEMENTED, or the failure code for unimplemented JS
> functions? I'll fix the null check either way, but I'd really rather not
> have the fallback in this case, especially since it involves trying to open
> a channel both times.

I suppose we can (not entirely sure though). I think it's worth filing a bug and we start a discussion around it there. At this point I think that sounds reasonable, but I am not entirely sure what other implications that change might have. (Please CC me on the bug and I'll ping the right folks to get the discussion started).

> I think we're winding up here when trying to open a channel for a
> nonexistent URL, and then falling back to NewChannel rather than NewChannel2
> when that fails. I hadn't considered the possibility that we'd be trying to
> open these channels without loadInfo at this point.

Adding the null check back sounds definitely right to fix this bug.

Comment 8

[Christoph Kerschbaumer [:ckerschb]]

(In reply to Kris Maglione [:kmag] from comment #6)
> (Soccoro is the internal name for crashstats, by the way. Comment 0 was
> referring to the crash reports.)

Ah, thanks, now I know.

Comment 9

[Kris Maglione [:kmag]]

Attachment: Bug 1350680: Correctly handle null loadInfo in SubstituteChannel.

Review commit: https://reviewboard.mozilla.org/r/124400/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/124400/

Comment 10

[Christoph Kerschbaumer [:ckerschb]]

Comment on attachment 8852176 [details]
Bug 1350680: Correctly handle null loadInfo in SubstituteChannel.

https://reviewboard.mozilla.org/r/124400/#review127254

thanks for fixing!

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/f474ff323cdd
Correctly handle null loadInfo in SubstituteChannel. r=ckerschb

Comment 12

[Carsten Book [:Tomcat]]

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