Closed Bug 1350680 Opened 6 years ago Closed 5 years ago

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

Categories

(WebExtensions :: General, defect)

Unspecified
Windows 8
defect
Not set
critical

Tracking

(firefox52 unaffected, firefox-esr52 unaffected, firefox53 unaffected, firefox54 unaffected, firefox55 fixed)

RESOLVED FIXED
mozilla55
Tracking Status
firefox52 --- unaffected
firefox-esr52 --- unaffected
firefox53 --- unaffected
firefox54 --- unaffected
firefox55 --- fixed

People

(Reporter: calixte, Assigned: kmag)

References

(Blocks 1 open bug)

Details

(Keywords: crash, Whiteboard: [clouseau])

Crash Data

Attachments

(1 file)

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
Flags: needinfo?(kmaglione+bmo)
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?
Assignee: nobody → kmaglione+bmo
Flags: needinfo?(kmaglione+bmo) → needinfo?(ckerschb)
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.
Duplicate of this bug: 1350901
(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
Flags: needinfo?(ckerschb)
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.
Flags: needinfo?(ckerschb)
(Soccoro is the internal name for crashstats, by the way. Comment 0 was referring to the crash reports.)
(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.
Flags: needinfo?(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.
See Also: → 1351443
Comment on attachment 8852176 [details]
Bug 1350680: Correctly handle null loadInfo in SubstituteChannel.

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

thanks for fixing!
Attachment #8852176 - Flags: review?(ckerschb) → review+
Pushed by maglione.k@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/f474ff323cdd
Correctly handle null loadInfo in SubstituteChannel. r=ckerschb
https://hg.mozilla.org/mozilla-central/rev/f474ff323cdd
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
Product: Toolkit → WebExtensions
You need to log in before you can comment on or make changes to this bug.