Closed Bug 1166133 Opened 9 years ago Closed 9 years ago

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

Categories

(Core :: Networking: HTTP, defect)

Unspecified
All
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla41
Tracking Status
firefox40 --- fixed
firefox41 --- fixed
firefox-esr38 --- affected

People

(Reporter: bc, Assigned: dragana)

References

()

Details

(Keywords: assertion)

Attachments

(3 files, 1 obsolete file)

Attached file 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
Can you upload or send a http log. Thank you.
Flags: needinfo?(bob)
Attached file nsprlog.zip
Flags: needinfo?(bob)
Assignee: nobody → dd.mozilla
Status: NEW → ASSIGNED
thanks for grabbing this dragana
Attached patch bug_1166133_v1.patch (obsolete) — Splinter Review
Attachment #8607557 - Flags: review?(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.
Attachment #8607557 - Flags: review?(mcmanus)
(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.
Ok move call of AsyncAbort into BeginConnect.

I have looked into splitting BeginConnect but it is not useful.
Attachment #8607557 - Attachment is obsolete: true
Attachment #8607686 - Flags: review?(mcmanus)
Attachment #8607686 - Flags: review?(mcmanus) → review+
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/fe3723f241a8
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla41
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
Attachment #8607686 - Flags: approval-mozilla-aurora?
Attachment #8607686 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Blocks: 1179154
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.
Attachment #8607686 - Flags: approval-mozilla-esr38?
Dragana, why do you think we should uplift it? Does not seems to be a top crash. thanks
Flags: needinfo?(dd.mozilla)
Sorry my mistake. We do not need to uplift this, it is a rare case.
Flags: needinfo?(dd.mozilla)
Attachment #8607686 - Flags: approval-mozilla-esr38?
You need to log in before you can comment on or make changes to this bug.