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

RESOLVED FIXED in mozilla26

Status

()

Core
General
--
critical
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: Alice0775 White, Assigned: mcmanus)

Tracking

({crash})

23 Branch
mozilla26
x86
Windows 7
crash
Points:
---

Firefox Tracking Flags

(firefox23-, firefox24-, firefox25-, firefox26-)

Details

(crash signature)

Attachments

(1 attachment)

(Reporter)

Description

5 years ago
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
(Reporter)

Comment 1

5 years ago
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
(Reporter)

Comment 2

5 years ago
Aha, Crashing in Nightly26.0a1 too.
bp-c8e20425-3f40-4ade-af22-c5ec82130829
tracking-firefox26: --- → ?
(Reporter)

Comment 3

5 years ago
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
Blocks: 793580
(Reporter)

Comment 4

5 years ago
Umm, f16404a61304 also crashes
No longer blocks: 793580
(Reporter)

Comment 5

5 years ago
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
Blocks: 769764
(Reporter)

Updated

5 years ago
Crash Signature: [@ mozilla::net::nsHttpChannel::CallOnStartRequest()] → [@ mozilla::net::nsHttpChannel::CallOnStartRequest() ]

Comment 6

5 years ago
Patrick, we see no reason to track this (it's non-critical). Anything here look critical to you?
Flags: needinfo?(mcmanus)
(Assignee)

Comment 7

5 years ago
(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.
Flags: needinfo?(mcmanus)
(Assignee)

Comment 8

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

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

Comment 9

5 years ago
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
(Assignee)

Comment 10

5 years ago
Created attachment 799448 [details] [diff] [review]
check httpchannel for listener release on redirect callback
Attachment #799448 - Flags: review?(jduell.mcbugs)
(Assignee)

Updated

5 years ago
Assignee: nobody → mcmanus
Status: NEW → ASSIGNED

Updated

5 years ago
tracking-firefox23: ? → -
tracking-firefox24: ? → -
tracking-firefox25: ? → -
tracking-firefox26: ? → -
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.
(Assignee)

Comment 12

5 years ago
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 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 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.
Attachment #799448 - Flags: review?(jduell.mcbugs) → review+
https://hg.mozilla.org/mozilla-central/rev/610bbc0fcf01
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla26
You need to log in before you can comment on or make changes to this bug.