1166133 - Assertion failure: !mOnStartRequestCalled (We should not call OnStartRequest twice)

Status: RESOLVED FIXED

(Core :: Networking: HTTP, defect)

Description

[Bob Clary [:bc] (inactive)]

Attachment: Crash report

1. http://liga.record.xl.pt/GerirEquipas/html5/alinhamento.aspx?id_team=105847

2. Assertion failure: !mOnStartRequestCalled (We should not call OnStartRequest twice), at c:\mozilla\builds\nightly\mozilla\netwerk\protocol\http\HttpBaseChannel.cpp:2136

This occurs on Windows only for Aurora/40 just loading the url but for Nightly/41 I need to run it via the spider extension [1] as firefox  -spider -start -quit -wait 5 -url 'http://liga.record.xl.pt/GerirEquipas/html5/alinhamento.aspx?id_team=105847' 

[1] http://bclary.com/projects/spider/spider/spider.xpi

Comment 1

[Dragana Damjanovic [:dragana]]

Can you upload or send a http log. Thank you.

Comment 2

[Bob Clary [:bc] (inactive)]

Attachment: nsprlog.zip

Comment 3

[Patrick McManus [:mcmanus]]

thanks for grabbing this dragana

Comment 4

[Dragana Damjanovic [:dragana]]

Attachment: bug_1166133_v1.patch

Comment 5

[Patrick McManus [:mcmanus]]

Comment on attachment 8607557 [details] [diff] [review]
bug_1166133_v1.patch

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

::: netwerk/protocol/http/nsHttpChannel.cpp
@@ +5168,5 @@
>  
>      if (NS_FAILED(rv)) {
>          Cancel(rv);
> +
> +        // if ContinueBeginConnect has already been called DoNotifyListener had been

I understand this patch except for this bit.

ContinueBeginConnect() certainly may have called DoNotifyListener on failure (and I can see how it leads to the stacktrace).. but if it succeeds then it won't do that.. I guess it would probably be hard for it to get to this line if it wasn't ContinueBeginConnect() returning the failed rv being dealt with here, but the !Active() check a few lines above would seem to be a counter example.

Comment 6

[Dragana Damjanovic [:dragana]]

(In reply to Patrick McManus [:mcmanus] from comment #5)
> Comment on attachment 8607557 [details] [diff] [review]
> bug_1166133_v1.patch
> 
> Review of attachment 8607557 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: netwerk/protocol/http/nsHttpChannel.cpp
> @@ +5168,5 @@
> >  
> >      if (NS_FAILED(rv)) {
> >          Cancel(rv);
> > +
> > +        // if ContinueBeginConnect has already been called DoNotifyListener had been
> 
> I understand this patch except for this bit.
> 
> ContinueBeginConnect() certainly may have called DoNotifyListener on failure
> (and I can see how it leads to the stacktrace).. but if it succeeds then it
> won't do that.. I guess it would probably be hard for it to get to this line
> if it wasn't ContinueBeginConnect() returning the failed rv being dealt with
> here, but the !Active() check a few lines above would seem to be a counter
> example.

The comment is not really good, but I think the code is correct.
If it is !Active then ContinueBeginConnect() will not be called and mContinueBeginConnectCalled will be false and AsyncAbort will be called.
If it is not !Active() BeginConnect will be called. The last return value is from calling ContinueBeginConnect().
ContinueBeginConnect() is really short http://mxr.mozilla.org/mozilla-central/source/netwerk/protocol/http/nsHttpChannel.cpp#5095

I know that this is not a perfect solution, the other solution would be to call AsyncAbort each time BeginConnect returns with an error, or cleaner splitting BeginConnect into two functions.
I will do that it is safer for later changes.

Comment 7

[Dragana Damjanovic [:dragana]]

Attachment: bug_1166133_v2.patch

Ok move call of AsyncAbort into BeginConnect.

I have looked into splitting BeginConnect but it is not useful.

Comment 8

[Dragana Damjanovic [:dragana]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=1018c2da0090

Comment 9

[Pulsebot]

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

Comment 10

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

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

Comment 11

[Dragana Damjanovic [:dragana]]

Comment on attachment 8607686 [details] [diff] [review]
bug_1166133_v2.patch

Approval Request Comment
[Feature/regressing bug #]: The check is introduced in Bug 1136969. so this crach is possible in firefox 40.0
[User impact if declined]: crash
[Describe test coverage new/current, TreeHerder]:
[Risks and why]: Low, it is in Nigthly for a week
[String/UUID change made/needed]:none

Comment 12

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/63fb6ce91b3c

Comment 13

[Dragana Damjanovic [:dragana]]

Comment on attachment 8607686 [details] [diff] [review]
bug_1166133_v2.patch

[Approval Request Comment]
If this is not a sec:{high,crit} bug, please state case for ESR consideration: this patch will solve the crash described in bug 1179154
User impact if declined: crash
Fix Landed on Version: 41 and it was uplifted to 40
Risk to taking this patch (and alternatives if risky): Low risk, it is in for a month and a half
String or UUID changes made by this patch: none

See https://wiki.mozilla.org/Release_Management/ESR_Landing_Process for more info.

Comment 14

[Sylvestre Ledru [:Sylvestre]]

Dragana, why do you think we should uplift it? Does not seems to be a top crash. thanks

Comment 15

[Dragana Damjanovic [:dragana]]

Sorry my mistake. We do not need to uplift this, it is a rare case.