The default bug view has changed. See this FAQ.

Trunk topcrash [@ nsXMLHttpRequest::OnStartRequest]

RESOLVED FIXED in mozilla1.9alpha8

Status

()

Core
DOM
--
critical
RESOLVED FIXED
10 years ago
6 years ago

People

(Reporter: bsmedberg, Assigned: sicking)

Tracking

({crash, regression, verified1.8.1.18})

Trunk
mozilla1.9alpha8
crash, regression, verified1.8.1.18
Points:
---
Bug Flags:
blocking1.9 +
blocking1.8.1.18 +
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

(crash signature)

Attachments

(3 attachments)

(Reporter)

Description

10 years ago
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

10 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: 389508
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.

Comment 4

10 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
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
Created attachment 279830 [details] [diff] [review]
Patch to fix

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+

Comment 10

10 years ago
Any reason this wasn't checked in?
This was checked in 10 days ago.
Status: NEW → RESOLVED
Last Resolved: 10 years ago
Resolution: --- → FIXED
Still need a mochitest though.

Comment 13

10 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
Created attachment 281392 [details] [diff] [review]
tests

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+
Attachment #281392 - Flags: approval1.9+
Created attachment 340973 [details] [diff] [review]
What actually got checked in

This applies to 1.8.1 and fixes bug 457411.
Attachment #340973 - Flags: approval1.8.1.18?
Duplicate of this bug: 457411
Flags: blocking1.8.1.18?
These tests landed
Flags: in-testsuite? → in-testsuite+
Duplicate of this bug: 457746
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
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*)
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)
Definitely different.
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

9 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.
Marking verified for 1.8.1.18 since it fixes bug 457411, which is verified.
Keywords: fixed1.8.1.18 → verified1.8.1.18
Crash Signature: [@ nsXMLHttpRequest::OnStartRequest]
You need to log in before you can comment on or make changes to this bug.