Closed Bug 390219 Opened 17 years ago Closed 17 years ago

Trunk topcrash [@ nsXMLHttpRequest::OnStartRequest]

Categories

(Core :: DOM: Core & HTML, defect)

defect
Not set
critical

Tracking

()

RESOLVED FIXED
mozilla1.9alpha8

People

(Reporter: benjamin, Assigned: sicking)

References

Details

(Keywords: crash, regression, verified1.8.1.18)

Crash Data

Attachments

(3 files)

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
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
Would be great if someone could attach a testcase or give steps to reproduce.
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
Btw, I think that line should set the owner on |channel|, not |mChannel|. The two are different for multipart requests. Need mochitests here.
Sicking: I'm assigning this to you, because I've seen you write a patch :-).
Assignee: nobody → jonas
Attached patch Patch to fixSplinter Review
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)
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 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+
Any reason this wasn't checked in?
This was checked in 10 days ago.
Status: NEW → RESOLVED
Closed: 17 years ago
Resolution: --- → FIXED
Still need a mochitest though.
Attached patch testsSplinter Review
This adds tests for this bug and bug 326337
Attachment #281392 - Flags: superreview?(peterv)
Attachment #281392 - Flags: review?(peterv)
Attachment #281392 - Flags: superreview?(peterv)
Attachment #281392 - Flags: superreview+
Attachment #281392 - Flags: review?(peterv)
Attachment #281392 - Flags: review+
This applies to 1.8.1 and fixes bug 457411.
Attachment #340973 - Flags: approval1.8.1.18?
Flags: blocking1.8.1.18?
These tests landed
Flags: in-testsuite? → in-testsuite+
Flags: blocking1.8.1.18? → blocking1.8.1.18+
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+
Checked in on 1.8 branch.
Keywords: fixed1.8.1.18
Henrik, that link shows no results...  In any case you'd want to file a separate bug on it if it's happening.
(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)
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.
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.
Marking verified for 1.8.1.18 since it fixes bug 457411, which is verified.
Crash Signature: [@ nsXMLHttpRequest::OnStartRequest]
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.