910518 - crash in mozilla::net::nsHttpChannel::CallOnStartRequest()

Status: RESOLVED FIXED

(Core :: General, defect)

Description

[Alice0775 White]

This bug was filed from the Socorro interface and is 
report
bp-8857a083-ee82-49d6-afab-1e72e2130829 : Aurora25.0a2
bp-d7a0723b-a472-45f6-bc1e-2a7182130829 : Firefox24b6
bp-dce1f83f-c23c-4c9b-9a82-fa8c92130829 : Firefox23.0.1
=============================================================

I am not sure the crash seems to do not happen in Nightly26.0a1.

Reproducible: intermittently

Steps To Reproduce:

1. Open https://paperc.com/store/overview
2. Click "Allow" button if door hanger was popuped
3. Click "Restrote Privous Session"
4. Exit browser
5. Start Browser again
6. Repeat from step 3, or step 1 if "Restrote Privous Session" disabled

Comment 1

[Alice0775 White]

Sorry, STR in commnet #0 is wrong

Steps Ro Reproduce:
1. Open https://paperc.com/store/overview
2. Click "Allow" button if door hanger was popuped

3. Exit browser
4. Start Browser again
5. Click "Restore Previous Session"
   --- Crashing happens while loading page. if not, continue step 6.

6. Repeat from step 3, or step 1 if "Restore Previous Session" disabled

Comment 2

[Alice0775 White]

Aha, Crashing in Nightly26.0a1 too.
bp-c8e20425-3f40-4ade-af22-c5ec82130829

Comment 3

[Alice0775 White]

Regression window(m-c)
Good:
http://hg.mozilla.org/mozilla-central/rev/c7b8d71aa25d
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:18.0) Gecko/18.0 Firefox/18.0 ID:20120927095854
Bad:
http://hg.mozilla.org/mozilla-central/rev/2d96ee8d9dd4
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:18.0) Gecko/18.0 Firefox/18.0 ID:20120927200536
Pushlog:
http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=c7b8d71aa25d&tochange=2d96ee8d9dd4


Regression window(m-i)
Good:
http://hg.mozilla.org/integration/mozilla-inbound/rev/f16404a61304
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:18.0) Gecko/18.0 Firefox/18.0 ID:20120927112256
Bad:
http://hg.mozilla.org/integration/mozilla-inbound/rev/c6cb52ebb2c8
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:18.0) Gecko/18.0 Firefox/18.0 ID:20120927123555
Pushlog:
http://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=f16404a61304&tochange=c6cb52ebb2c8


In local build:
Last Good: f16404a61304
First Bad: 8e8915a37f5a
Regressed by:
	8e8915a37f5a	Mark Banner — Bug 793580 - Part 1: Use unsigned types in ExceptionArgParser::parseResult; r=ehsan

Comment 4

[Alice0775 White]

Umm, f16404a61304 also crashes

Comment 5

[Alice0775 White]

Regression window(m-c)
Good:
http://hg.mozilla.org/mozilla-central/rev/c7b8d71aa25d
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:18.0) Gecko/18.0 Firefox/18.0 ID:20120927095854
Bad:
http://hg.mozilla.org/mozilla-central/rev/2d96ee8d9dd4
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:18.0) Gecko/18.0 Firefox/18.0 ID:20120927200536
Pushlog:
http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=c7b8d71aa25d&tochange=2d96ee8d9dd4


Regression window(m-i)
Good:
http://hg.mozilla.org/integration/mozilla-inbound/rev/c5a7b7544f12
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:18.0) Gecko/18.0 Firefox/18.0 ID:20120927094355
Bad:
http://hg.mozilla.org/integration/mozilla-inbound/rev/0129800fa8a1
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:18.0) Gecko/18.0 Firefox/18.0 ID:20120927103456
Pushlog:
http://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=c5a7b7544f12&tochange=0129800fa8a1



In local build:
Last Good: 198e780680c1
First Bad: dcae72a1333c
Regressed by:
	dcae72a1333c	Patrick McManus — bug 769764 move proxy resolution to separate thread and remove sync api r=biesi sr=josh

Comment 6

[Alex Keybl [:akeybl]]

Patrick, we see no reason to track this (it's non-critical). Anything here look critical to you?

Comment 7

[Patrick McManus [:mcmanus]]

(In reply to Alex Keybl [:akeybl] from comment #6)
> Patrick, we see no reason to track this (it's non-critical). Anything here
> look critical to you?

its a null deref - so there isn't a security problem. and it appears to have been around for a while at low volume so I don't see the need to track.

however on the positive side I think I know what's going on and it really is just a matter of checking a couple pointers before de-reffing.

I can't reproduce using the STR, so I'll upload a patch to try and let Alice confirm.

Comment 8

[Patrick McManus [:mcmanus]]

Alice, can you try this build and see if it helps?

https://tbpl.mozilla.org/?tree=Try&rev=a537abf7c286

Comment 9

[Alice0775 White]

I cannot reproduce the crash with the try-build.
http://hg.mozilla.org/try/rev/a537abf7c286
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:26.0) Gecko/20100101 Firefox/26.0 ID:20130903155244

Comment 10

[Patrick McManus [:mcmanus]]

Attachment: check httpchannel for listener release on redirect callback

Comment 11

[Jason Duell]

Comment on attachment 799448 [details] [diff] [review]
check httpchannel for listener release on redirect callback

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

Patrick:  So do we know how the culprit in the regression range in comment 5 (ie. bug 769764) changed things in a way that made us have mListener sometimes be null here?  My only concern is that we need to make sure that OnStart/Stop get called.  If OnStart is now called in some error/etc path and by the time we're here it's null, than this patch is fine.  But if OnStop is never called we've got an IDL contract violation.

Comment 12

[Patrick McManus [:mcmanus]]

the regression range is the code that makes a lot of code that used to be synchronous to asyncOpen now be truly async because the proxy lookup is truly async.

I don't know the exact trail of tears that leads to this crash, but I can imagine that the new behavior provides opportunities for exactly the double error path.. especially when mixed with a redirect cocktail. there are only a handful of places ReleaseListeners() is used to set mListener to null. 

its worth noting that the other uses of mListener are all null checked (in onstop and oda), indicating that this problem is pervasive and probly just couldn't be triggered in onstart previously.

Comment 13

[Honza Bambas (:mayhemer)]

Comment on attachment 799448 [details] [diff] [review]
check httpchannel for listener release on redirect callback

Can we at least log or NS_WARN or so?

Comment 14

[Jason Duell]

Comment on attachment 799448 [details] [diff] [review]
check httpchannel for listener release on redirect callback

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

warning would be nice.

Comment 15

[Patrick McManus [:mcmanus]]

added ns_warning()s

  https://hg.mozilla.org/integration/mozilla-inbound/rev/610bbc0fcf01

Comment 16

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/610bbc0fcf01