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

RESOLVED FIXED in Firefox 55

Status

WebExtensions
General
--
critical
RESOLVED FIXED
a year ago
5 days ago

People

(Reporter: calixte, Assigned: kmag)

Tracking

(Blocks: 1 bug, {crash})

unspecified
mozilla55
Unspecified
Windows 8
crash
Dependency tree / graph

Firefox Tracking Flags

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

Details

(Whiteboard: [clouseau], crash signature)

MozReview Requests

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(1 attachment)

(Reporter)

Description

a year ago
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)
(Assignee)

Comment 1

a year ago
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)
(Assignee)

Comment 2

a year ago
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)
(Assignee)

Comment 5

a year ago
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)
(Assignee)

Comment 6

a year ago
(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.
(Assignee)

Updated

a year ago
See Also: → bug 1351443
Comment hidden (mozreview-request)

Comment 10

a year ago
mozreview-review
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+

Comment 11

a year ago
Pushed by maglione.k@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/f474ff323cdd
Correctly handle null loadInfo in SubstituteChannel. r=ckerschb

Comment 12

a year ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/f474ff323cdd
Status: NEW → RESOLVED
Last Resolved: a year ago
status-firefox55: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
status-firefox52: --- → unaffected
status-firefox53: --- → unaffected
status-firefox54: --- → unaffected
status-firefox-esr52: --- → unaffected
Blocks: 1396527

Updated

5 days ago
Product: Toolkit → WebExtensions
You need to log in before you can comment on or make changes to this bug.