1199862 - [e10s] crash in nsCOMPtr_base::assign_with_AddRef(nsISupports*zilla::net::HttpChannelParent::RecvDivertOnStopRequest(nsresult const&))

Status: RESOLVED FIXED

(Core :: Networking: HTTP, defect)

Description

[Loic]

This bug was filed from the Socorro interface and is 
report bp-0db3adb3-c266-4749-bde5-977732150829.
=============================================================

I hit this crash when testing bug 1199782.

1) Start Nightly in e10s mode
2) Open http://agpl.fsf.org/emailselfdefense.fsf.org/edward/CURRENT/ and click on the file "edward.tgz.sig" to download it
3) Close immediately the error dialog box (yellow triangle) by clicking on the X button

Sometimes it doesn't crash, just spam the X button of the error dialog box to crash, it's a matter of "timing", I guess.

Result: all the browser crashes!

2 CR with similar signatures:

Firefox 43.0a1 Crash Report [@ nsCOMPtr_base::assign_with_AddRef(nsISupports*) | PL_HashTableLookupConst | mozilla::net::HttpChannelParent::RecvDivertOnStopRequest(nsresult const&) ]
https://crash-stats.mozilla.com/report/index/54875785-4431-49ac-b012-91cd02150829

Firefox 43.0a1 Crash Report [@ nsCOMPtr_base::assign_with_AddRef(nsISupports*) | mozilla::net::HttpChannelParentListener::OnStopRequest(nsIRequest*, nsISupports*, nsresult) ]
https://crash-stats.mozilla.com/report/index/8e1c1ea6-ae8d-4c02-877a-73e142150829

Comment 1

[Dragana Damjanovic [:dragana]]

The analysis:

HttpChannelParent calls StopRequest of the listener which prompt the error dialog and does not return from the function but calls next function in the queue.
On of the next functions is RecvDivertComplited which deletes the listener. So when the prompt returns it finish the function on already destroyed listener and that crashes.

Comment 2

[Josh Matthews [:jdm]]

This sounds like the problem with sync XHR/alert that we fixed back in 2010 by creating a queue of IPDL necko events and deferring running them until after we return from the listener calls. What changed?

Comment 3

[Josh Matthews [:jdm]]

Oh, this is happening in the parent this time :(

Comment 4

[Dragana Damjanovic [:dragana]]

Attachment: bug_1199862_v1.patch

Comment 5

[Dragana Damjanovic [:dragana]]

Attachment: bug_1199862_v1.patch

Comment 6

[Dragana Damjanovic [:dragana]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=48b47970f0e0

Comment 7

[Jason Duell]

Comment on attachment 8655451 [details] [diff] [review]
bug_1199862_v1.patch

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

This looks good to me.  I assume you've tested it against the STR in comment 0?

In HttpChannelChild we hook :Suspend() so it also calls Suspend on the ChannelEventQueue.  I was wondering if we need something like that here.  Looking over the existing code (and with this patch) I'm under the impression that our DivertTo logic does not handle the case where we 1) DivertTo() a channel on the parent, and then 2) some external code (download manager) suspends that channel during the IPDL diversion process.  It looks to me like we just call OnStartRequest/OnDataAvailable as the diverted IPDL messages arrive.  I think that's a bug (though one that's unlikely to happen in practice, unless a user tried to suspend a download immediately after they start it--even that might be hard to human fingers to do in time?).  Fortunately enough, adding mEventQueue to the parent should make fixing that easier.  I assume we'd need to check the suspendCount of mChannel each time a diverted message arrives, and if it's >1 (i.e. if the channel has been suspended more than just when we did it as part of SuspendForDiversion()), then we could queue the message.  Except we'd need some notification of when the channel was resumed back down to count==1.  That may be a little messy.  Anyway, if I'm right about this, it's clearly a new bug, and shouldn't block this one.   But think over it, and file a bug if you also agree we've got a bug here.

Comment 8

[Dragana Damjanovic [:dragana]]

(In reply to Jason Duell [:jduell] (needinfo? me) from comment #7)
> Comment on attachment 8655451 [details] [diff] [review]
> bug_1199862_v1.patch
> 
> Review of attachment 8655451 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> This looks good to me.  I assume you've tested it against the STR in comment
> 0?
> 
> In HttpChannelChild we hook :Suspend() so it also calls Suspend on the
> ChannelEventQueue.  I was wondering if we need something like that here. 
> Looking over the existing code (and with this patch) I'm under the
> impression that our DivertTo logic does not handle the case where we 1)
> DivertTo() a channel on the parent, and then 2) some external code (download
> manager) suspends that channel during the IPDL diversion process.  It looks
> to me like we just call OnStartRequest/OnDataAvailable as the diverted IPDL
> messages arrive.  I think that's a bug (though one that's unlikely to happen
> in practice, unless a user tried to suspend a download immediately after
> they start it--even that might be hard to human fingers to do in time?). 
> Fortunately enough, adding mEventQueue to the parent should make fixing that
> easier.  I assume we'd need to check the suspendCount of mChannel each time
> a diverted message arrives, and if it's >1 (i.e. if the channel has been
> suspended more than just when we did it as part of SuspendForDiversion()),
> then we could queue the message.  Except we'd need some notification of when
> the channel was resumed back down to count==1.  That may be a little messy. 
> Anyway, if I'm right about this, it's clearly a new bug, and shouldn't block
> this one.   But think over it, and file a bug if you also agree we've got a
> bug here.

I will file another bug for this.

Comment 9

[Carsten Book [:Tomcat]]

Hi, this patch failed to apply:

patching file netwerk/protocol/http/HttpChannelParent.h
Hunk #1 FAILED at 152
1 out of 2 hunks FAILED -- saving rejects to file netwerk/protocol/http/HttpChannelParent.h.rej

could you take a look, thanks!

Comment 10

[Dragana Damjanovic [:dragana]]

Attachment: bug_1199862_v1.patch

Rebased. 
Thanks!

Comment 11

[Pulsebot]

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

Comment 12

[Carsten Book [:Tomcat]]

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