Closed
Bug 390219
Opened 17 years ago
Closed 17 years ago
Trunk topcrash [@ nsXMLHttpRequest::OnStartRequest]
Categories
(Core :: DOM: Core & HTML, defect)
Core
DOM: Core & HTML
Tracking
()
RESOLVED
FIXED
mozilla1.9alpha8
People
(Reporter: benjamin, Assigned: sicking)
References
Details
(Keywords: crash, regression, verified1.8.1.18)
Crash Data
Attachments
(3 files)
4.19 KB,
patch
|
jst
:
review+
jst
:
superreview+
jst
:
approval1.9+
|
Details | Diff | Splinter Review |
6.86 KB,
patch
|
peterv
:
review+
peterv
:
superreview+
sicking
:
approval1.9+
|
Details | Diff | Splinter Review |
2.30 KB,
patch
|
dveditz
:
approval1.8.1.18+
|
Details | Diff | Splinter Review |
Trunk topcrash #32 is a new regression. http://crash-stats.mozilla.com/report/list?range_unit=weeks&branch=1.9&range_value=2&signature=nsXMLHttpRequest%3A%3AOnStartRequest(nsIRequest+*%2CnsISupports+*) I'm going to presumptively blame bug 389508
Flags: blocking1.9+
Comment 1•17 years ago
|
||
Actually, this crash started happening before bug 389508 landed. First crash appeared on 2007-07-09: http://bonsai.mozilla.org/cvsquery.cgi?treeid=default&module=PhoenixTinderbox&branch=HEAD&branchtype=match&dir=&file=&filetype=match&who=&whotype=match&sortby=Date&hours=2&date=explicit&mindate=2007-07-08+03&maxdate=2007-07-09+06&cvsroot=%2Fcvsroot. The patch for bug 373231 and bug 382947 look suspicious.
No longer blocks: xxx
Severity: normal → critical
Keywords: crash,
regression
Summary: Trunk topcrash [nsXMLHttpRequest::OnStartRequest(nsIRequest *,nsISupports *)] → Trunk topcrash [@ nsXMLHttpRequest::OnStartRequest]
Version: unspecified → Trunk
Comment 2•17 years ago
|
||
I think this isn't bad enough to block M7, its #17 for 3.0a7pre, with 1/40th of the crashes of the #1 topcrash. Definitely needed for M8.
Target Milestone: mozilla1.9 M7 → mozilla1.9 M8
Assignee | ||
Comment 3•17 years ago
|
||
Would be great if someone could attach a testcase or give steps to reproduce.
Comment 4•17 years ago
|
||
I'm not sure what graph I was looking at before, but this is the one I wanted: http://crash-stats.mozilla.com/report/list?range_unit=months&query_search=signature&query_type=contains&product=Firefox&branch=1.9&signature=nsXMLHttpRequest%3A%3AOnStartRequest(nsIRequest+*%2CnsISupports+*)&query=nsXMLHttpRequest%3A%3AOnStartRequest&range_value=3. This started on 2007-07-02, which is when the patch for bug 326337 landed. In fact, it's crashing on one of the lines of code that peterv added.
Blocks: 326337
Assignee | ||
Comment 5•17 years ago
|
||
Btw, I think that line should set the owner on |channel|, not |mChannel|. The two are different for multipart requests. Need mochitests here.
Comment 6•17 years ago
|
||
Sicking: I'm assigning this to you, because I've seen you write a patch :-).
Assignee: nobody → jonas
Assignee | ||
Comment 7•17 years ago
|
||
What's happening is that we get OnStartRequest/OnStopRequests from old requests that has been canceled. These requests come in after we have reopened a new request, so the current checks for being in the uninitialized state don't work as a new request has been opened.
Attachment #279830 -
Flags: superreview?(jst)
Attachment #279830 -
Flags: review?(jst)
Assignee | ||
Comment 8•17 years ago
|
||
I've verified that the patch does fix the problem, but i'd like to create a mochitest too. Still working on that part.
Flags: in-testsuite?
Comment 9•17 years ago
|
||
Comment on attachment 279830 [details] [diff] [review] Patch to fix - In nsXMLHttpRequest::OnStartRequest(): + nsCOMPtr<nsIMultiPartChannel> mpChannel = do_QueryInterface(request); + if (mpChannel) { + nsCOMPtr<nsIChannel> baseChannel; + rv = mpChannel->GetBaseChannel(getter_AddRefs(baseChannel)); + NS_ENSURE_SUCCESS(rv, rv); + + if (baseChannel != mChannel) { + return NS_OK; + } + } + else if (request != mChannel) { + return NS_OK; + } This hunk of code is duplicated in OnStopRequest(). Not a big deal, but it could be split out for better maintainability, maybe. r+sr=jst either way.
Attachment #279830 -
Flags: superreview?(jst)
Attachment #279830 -
Flags: superreview+
Attachment #279830 -
Flags: review?(jst)
Attachment #279830 -
Flags: review+
Attachment #279830 -
Flags: approval1.9+
Comment 10•17 years ago
|
||
Any reason this wasn't checked in?
Comment 11•17 years ago
|
||
This was checked in 10 days ago.
Status: NEW → RESOLVED
Closed: 17 years ago
Resolution: --- → FIXED
Comment 12•17 years ago
|
||
Still need a mochitest though.
Comment 13•17 years ago
|
||
Sorry, got confused by the fact that jonas did the suggested refactoring before the check-in: http://bonsai.mozilla.org/cvsview2.cgi?diff_mode=context&whitespace_mode=show&file=nsXMLHttpRequest.cpp&branch=&root=/cvsroot&subdir=/mozilla/content/base/src&command=DIFF_FRAMESET&rev1=1.195&rev2=1.196
Assignee | ||
Comment 14•17 years ago
|
||
This adds tests for this bug and bug 326337
Attachment #281392 -
Flags: superreview?(peterv)
Attachment #281392 -
Flags: review?(peterv)
Updated•17 years ago
|
Attachment #281392 -
Flags: superreview?(peterv)
Attachment #281392 -
Flags: superreview+
Attachment #281392 -
Flags: review?(peterv)
Attachment #281392 -
Flags: review+
Assignee | ||
Updated•17 years ago
|
Attachment #281392 -
Flags: approval1.9+
Comment 15•16 years ago
|
||
This applies to 1.8.1 and fixes bug 457411.
Attachment #340973 -
Flags: approval1.8.1.18?
Updated•16 years ago
|
Flags: blocking1.8.1.18?
Updated•16 years ago
|
Flags: blocking1.8.1.18? → blocking1.8.1.18+
Comment 19•16 years ago
|
||
Comment on attachment 340973 [details] [diff] [review] What actually got checked in Approved for 1.8.1.18, a=dveditz for release-drivers
Attachment #340973 -
Flags: approval1.8.1.18? → approval1.8.1.18+
Comment 21•16 years ago
|
||
Boris, this crash still happens for Firefox 3.0.3 even there is a much smaller amount of frames (lets say only 2): http://crash-stats.mozilla.com/report/list?product=Firefox&query_search=signature&query_type=contains&query=nsXMLHttpRequest%3A%3AOnStartRequest&date=&range_value=1&range_unit=months&do_query=1&signature=nsXMLHttpRequest%3A%3AOnStartRequest(nsIRequest*%2C%20nsISupports*)
Comment 22•16 years ago
|
||
Henrik, that link shows no results... In any case you'd want to file a separate bug on it if it's happening.
Comment 23•16 years ago
|
||
(In reply to comment #22) > Henrik, that link shows no results... In any case you'd want to file a > separate bug on it if it's happening. Boris, just to double check before filing a new bug. Could you have a short look at these crashes? bp-ae3452ae-9965-11dd-8cb4-001a4bd43e5c bp-25bf57e8-9633-11dd-80b7-001cc45a2c28 bp-57b2ca5e-9eb3-11dd-b91c-001321b13766 (suspicious module)
Assignee | ||
Comment 24•16 years ago
|
||
Definitely different.
Comment 25•16 years ago
|
||
The first one is crashing on this line: 1501 hpradhan 1.90 if (mState & XML_HTTP_REQUEST_PARSEBODY) { The second one on this one: 1460 jst 1.101 nsCOMPtr<nsIDOMEventListener> proxy = new nsLoadListenerProxy(requestWeak); The third at: 1474 nsCOMPtr<nsIHttpChannel> httpChannel(do_QueryInterface(mChannel)); None are the crash location for this bug, and while the first and third may be due to the |this| object being garbage, the second can only happen due to OOM or something. At least assuming the line numbers are correct. If they are, I would wager that these are cases when we crashed with a corrupted stack and that the stack info is just bogus or something.
Comment 26•16 years ago
|
||
the mState crash looks more like a dead this than a null this based on the crash address... not certain.... the second is a null pointer crash 1458 nsWeakPtr requestWeak = 1459 jwalden 1.186 do_GetWeakReference(static_cast<nsIXMLHttpRequest*>(this)); 1460 jst 1.101 nsCOMPtr<nsIDOMEventListener> proxy = new nsLoadListenerProxy(requestWeak); my guess is that this is null, the weakreference would return an error in the omitted out parameter and you'd have a null pointer which is probably . the one w/ crash address 0 implies a null this pointer - the third is almost certainly here: 1471 request->GetStatus(&status); with request being null. -- these are probably not related to this bug.
Comment 27•16 years ago
|
||
Marking verified for 1.8.1.18 since it fixes bug 457411, which is verified.
Keywords: fixed1.8.1.18 → verified1.8.1.18
Updated•13 years ago
|
Crash Signature: [@ nsXMLHttpRequest::OnStartRequest]
Updated•5 years ago
|
Component: DOM → DOM: Core & HTML
You need to log in
before you can comment on or make changes to this bug.
Description
•