1360332 - Crash in mozilla::net::HttpChannelParent::NotifyDiversionFailed

Status: RESOLVED FIXED

(Core :: Networking, defect)

Description

[Marcia Knous [:marcia]]

This bug was filed from the Socorro interface and is 
report bp-85719858-3a04-4a30-8c77-a4bb60170427.
=============================================================

Seen while looking at nightly crash stats - started using 20170426030329 build: http://bit.ly/2oNydmP. Fairly low volume crash so far.

Possible regression range based on Build ID: https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=a30dc237c3a600a5231f2974fc2b85dfb5513414&tochange=0f5ba06c4c5959030a05cb852656d854065e2226

URLs:

http://www.ceramicrecords.net/yzcr001/
http://www.chip.de/downloads/Steam_35910421.html

Comment 1

[u408661]

Dragana - this is a long shot to be sure, but you're the only one touching necko code I see in the regression range above. Do you think the TFO stuff from bug 1188435 could have anything to do with this? If not, just clear ni? and I'll get this re-triaged. Thanks!

Comment 2

[Dragana Damjanovic [:dragana]]

(In reply to Nicholas Hurley [:nwgh][:hurley] (also [e-mail]) from comment #1)
> Dragana - this is a long shot to be sure, but you're the only one touching
> necko code I see in the regression range above. Do you think the TFO stuff
> from bug 1188435 could have anything to do with this? If not, just clear ni?
> and I'll get this re-triaged. Thanks!

That one was backed out already in m.-i. :)

Comment 3

[Honza Bambas (:mayhemer)]

correct me if I'm wrong

Comment 4

[Dragana Damjanovic [:dragana]]

This is not TFO. there are crashes even before TFO landed.

I will unassigned my self, I am not sure I have time to take a look.
Long shot but I will ask. sc, have you change something a 25.4.?

Comment 5

[Dragana Damjanovic [:dragana]]

sc, you were doing stuff around parent-child process.

Comment 6

[Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz)]

Here is my analysis.

Taking a one-year-long query: https://crash-stats.mozilla.com/signature/?signature=mozilla%3A%3Anet%3A%3AHttpChannelParent%3A%3ANotifyDiversionFailed&date=%3E%3D2016-06-09T01%3A23%3A00.000Z&date=%3C2017-06-08T01%3A23%3A35.000Z#graphs

1) This crash signature exists long ago but with a relatively low frequency (about 1~2 crash per week).
2) Most of the crashes happens in Version 55 (48.7%)
3) Crash rate ramps up after buildID 20170418004027 on nightly channel (version 54)
4) Crash rate peak between buildID 20170504105526 and 20170507030205 on nightly channel
5) Crash report stops at buildID 20170523030206 on nightly channel

The PBg-Http modification that touches HTTP IPC lands on buildID 20170328095415 and 20170602030204, which have very little correlation with this crash IMO.

Comment 7

[Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz)]

By looking at the crash symptom, it is complaining about mParentListener is removed before the HTTPFailDiversionEvent runnable get executed.

There are only 3 functions that will remove mParentListener:
1) HttpChannelParent::AsyncOpenFailed
  Not sure if channel diversion can happen before async open complete.

2) HttpChannelParent::DivertComplete
  Not sure if there is any race condition that between DivertComplete and FailDiversion. We should add diagnostic assertion while we detect it.

3) HttpChannelParent::NotifyDiversionFailed
  Will only cause problem if HttpChannelParent::FailDiversion is called multiple times. This is possible because our diversion IPC messsage handler will invoke FailDiversion if anything go wrong and usually there will be more diversion IPC messages waiting for process (e.g. multiple DivertODA). We should be able to ensure there is only one HTTPFailDiversionEvent fired.

Comment 8

[Shian-Yow Wu [:swu]]

(In reply to Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz) from comment #7)
> 3) HttpChannelParent::NotifyDiversionFailed
>   Will only cause problem if HttpChannelParent::FailDiversion is called
> multiple times. This is possible because our diversion IPC messsage handler
> will invoke FailDiversion if anything go wrong and usually there will be
> more diversion IPC messages waiting for process (e.g. multiple DivertODA).
> We should be able to ensure there is only one HTTPFailDiversionEvent fired.

There is also another possiblity for this.  If FailDiversion() is called inside ResumeForDiversion(), it will always be called again after ResumeForDiversion() returns error.

https://searchfox.org/mozilla-central/rev/a798ee4fc323f9387b7576dbed177859d29d09b7/netwerk/protocol/http/HttpChannelParent.cpp#1316

Comment 9

[Gary Chen [:xeonchen]]

Attachment: Bug 1360332 - Part 0: remove unused function parameter;

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

Comment 10

[Gary Chen [:xeonchen]]

Attachment: Bug 1360332 - Part 1: avoid calling FailDiversion() twice;

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

Comment 11

[Dragana Damjanovic [:dragana]]

Comment on attachment 8877405 [details]
Bug 1360332 - Part 0: remove unused function parameter;

https://reviewboard.mozilla.org/r/148802/#review153894

Comment 12

[Dragana Damjanovic [:dragana]]

Comment on attachment 8877406 [details]
Bug 1360332 - Part 1: avoid calling FailDiversion() twice;

https://reviewboard.mozilla.org/r/148804/#review153900

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/69564ef15348
Part 0: remove unused function parameter; r=dragana
https://hg.mozilla.org/integration/autoland/rev/2e0028f31611
Part 1: avoid calling FailDiversion() twice; r=dragana

Comment 14

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/69564ef15348
https://hg.mozilla.org/mozilla-central/rev/2e0028f31611

Comment 15

[Gary Chen [:xeonchen]]

ni? myself to check this several days later

Comment 16

[Gary Chen [:xeonchen]]

No recent crash on Nightly, basically crashes are on aurora/beta, so I think we should close this first.