nsXMLHttpRequest leaks if aborted in state 3 ("in progress")

RESOLVED FIXED in mozilla1.9.3a5

Status

()

Core
Networking
RESOLVED FIXED
8 years ago
8 years ago

People

(Reporter: bjarne, Assigned: bjarne)

Tracking

({memory-leak})

Trunk
mozilla1.9.3a5
memory-leak
Points:
---
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 4 obsolete attachments)

(Assignee)

Description

8 years ago
This comes from bug #482935.
(Assignee)

Updated

8 years ago
Blocks: 482935
(Assignee)

Comment 1

8 years ago
Created attachment 432786 [details] [diff] [review]
Testcase and simple fix 

Test taken from bug #482935 and simple fix.

(The change to CParserContext.cpp was something I stumbled across while working on this - it can be removed if not appropriate here.)

Asking sicking for review - feel free to reassign if this is wrong.
Assignee: nobody → bjarne
Status: NEW → ASSIGNED
Attachment #432786 - Flags: review?(jonas)
Of course, the test here has the same "question" about "open vs addEventListener" order as the one in bug 482935.
(But this doesn't prevent fixing the leak ;-))
I don't understand the purpose of cancelXHR.sjs. If you just need to load something from the server, you could just load the test_cancelXHR.html file directly.

Also, given how specific the test is, I'd name it test_bug552651.html.
(Assignee)

Comment 4

8 years ago
Tryserver is happy with this fix, like commented in bug #482935, comment 37.

I'll check out if it is sufficient to just load anything from localhost (dropping cancelXHR.sjs) and rename the test asap.
(Assignee)

Comment 5

8 years ago
Ohh, and I'll address sgauthieres comment as well... :) (although it makes no difference in practice).
The open vs addEventListener thing is bogus. The docs were just plain wrong. I've updated them.
Comment on attachment 432786 [details] [diff] [review]
Testcase and simple fix 

r=me with the test renames/changes.
Attachment #432786 - Flags: review?(jonas) → review+
(Assignee)

Comment 8

8 years ago
> I'll check out if it is sufficient to just load anything from localhost
> (dropping cancelXHR.sjs) and rename the test asap.

Loading "/" (for example) does not trigger this issue, probably because content then is html, but I'm not sure. So current thinking is to keep "cancelXHR.js".
(Assignee)

Comment 9

8 years ago
(For the record : loading the test-file itself does not trigger the leak either.)

Something struck me : Since the content of the aborted document seems to make a difference, the suggested fix may not be the right one...  any input?
Yeah, you probably need something with an XML mimetype, otherwise we won't start a parser. Just loading a file named file_bug552651.xml containing

<foo>bar</foo>

should be enough I'd think.
(Assignee)

Comment 11

8 years ago
Created attachment 433192 [details] [diff] [review]
Updated according to comments from reviewer
Attachment #432786 - Attachment is obsolete: true
Attachment #433192 - Flags: review?(jonas)
Comment on attachment 433192 [details] [diff] [review]
Updated according to comments from reviewer

So I take it these tests leak without the cpp changes?

>diff -r 8606d3764288 content/base/test/file_bug552651.xml
>--- /dev/null	Thu Jan 01 00:00:00 1970 +0000
>+++ b/content/base/test/file_bug552651.xml	Thu Mar 18 00:04:16 2010 +0100
>@@ -0,0 +1,1 @@
>+test
>\ No newline at end of file

Please add a newline at the end. And seems like a good idea to make the contents be valid XML, like "<foo>test</foo>".

Also, I realized that we should make this a crashtest rather than a mochitest given that there are no behavioral tests here. Mochitest actually warns if there are no tests in a test IIRC.

r=me with that.
Attachment #433192 - Flags: review?(jonas) → review+
(Assignee)

Comment 13

8 years ago
Created attachment 433293 [details] [diff] [review]
Updated according to last comments

(In reply to comment #12)
> (From update of attachment 433192 [details] [diff] [review])
> So I take it these tests leak without the cpp changes?

Yes... :)

> >diff -r 8606d3764288 content/base/test/file_bug552651.xml
> >--- /dev/null	Thu Jan 01 00:00:00 1970 +0000
> >+++ b/content/base/test/file_bug552651.xml	Thu Mar 18 00:04:16 2010 +0100
> >@@ -0,0 +1,1 @@
> >+test
> >\ No newline at end of file
> 
> Please add a newline at the end. And seems like a good idea to make the
> contents be valid XML, like "<foo>test</foo>".

Ok.

> Also, I realized that we should make this a crashtest rather than a mochitest
> given that there are no behavioral tests here. Mochitest actually warns if
> there are no tests in a test IIRC.

I converted it to a crashtest, but the test passes even when leaking....  any idea how to resolve..?
Attachment #433192 - Attachment is obsolete: true
Attachment #433293 - Flags: review?(jonas)
Both crashtests and mochitests only check for leaks in debug builds i'm told. If that's not happening, please file a separate bug.
(Assignee)

Comment 15

8 years ago
I asked guys at #developers and apparently the crashtest/reftest will report success but the test-harness will make the tinderbox go orange. Not really sure how to verify this, but they're probably right. Want me to remove the mochitest from the patch and send it to tryserver?
I'd say just make it a crashtest. I'm not too worried about a sudden bug in our test harnesses.
(In reply to comment #13)
> I converted it to a crashtest, but the test passes even when leaking....  any
> idea how to resolve..?

Passing and leaking are two different things.
Obviously, we can only check the test in when it succeeds both.
(which may require the leak to be fixed first.)

(In reply to comment #14)
> Both crashtests and mochitests only check for leaks in debug builds i'm told.

No, a (optimized) build with --enable-trace-malloc is enough.
(Assignee)

Comment 18

8 years ago
Created attachment 433430 [details] [diff] [review]
Removed mochitest

Removed mochitest, left crashtest. Pushed to tryserver...
Attachment #433293 - Attachment is obsolete: true
Attachment #433293 - Flags: review?(jonas)
(Assignee)

Comment 19

8 years ago
Tryserver is unhappy on "WINNT 5.2 try hg unit" and "OS X 10.5.2 try hg unit test". However, the latter is burning at the moment. Moreover, the tests immediately before and after this patch on the WINNT box also has problems (the one before has the exact same crash).

I can push it later when things calm down...
(Assignee)

Comment 20

8 years ago
Figured out why it doesn't apply on tinderbox (although it applies locally). Pushed once, timeout on mochitest-browser-chrome for Linux. Pushed second time to see if it's a coincidence...
(Assignee)

Comment 21

8 years ago
Hmm...  results from two tryserver-runs are confusing. First run is successful on OSX and WinNT and has warnings on Linux. Second run is successful on OSX and Linux but has warnings on WinNT. In both cases, warnings seem to come from a number of application-crashes.

The logs can be found here

http://tinderbox.mozilla.org/showlog.cgi?log=MozillaTry/1271756969.1271767909.6149.gz

http://tinderbox.mozilla.org/showlog.cgi?log=MozillaTry/1271844145.1271857498.12835.gz

If someone can draw any conclusion from these logs, I'd be happy to listen...
Tryserver tends to fall over in odd ways. If the crashes look unrelated to your patch, they quite probably are.
(Assignee)

Comment 23

8 years ago
Hard to tell if they are related or not... threads crash with sigsegv. On WinNT most of these are in 

nptest.dll!IntentionalCrash [nptest.cpp:cdc8bf25220e : 99 + 0x6]

I'll upload an unbitrotted patch and request review again. Maybe some baking on trunk will tell us something...
(Assignee)

Comment 24

8 years ago
Created attachment 440988 [details] [diff] [review]
Unbitrot
Attachment #433430 - Attachment is obsolete: true
Attachment #440988 - Flags: review?(jonas)
(Assignee)

Comment 25

8 years ago
Requesting check-in in order to bake it on trunk for a while
Keywords: checkin-needed
http://hg.mozilla.org/mozilla-central/rev/9bb560f8c8de
Status: ASSIGNED → RESOLVED
Last Resolved: 8 years ago
Flags: in-testsuite+
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → mozilla1.9.3a5
Keywords: mlk
Attachment #440988 - Flags: approval1.9.2.13?
Attachment #440988 - Flags: approval1.9.1.16?
Comment on attachment 440988 [details] [diff] [review]
Unbitrot

This bug says it "comes from bug 482935" and is marked that way in the dependency tree, but it doesn't appear bug 482935 ever landed on the 1.9.2 branch. Don't want to land a patch we don't need.

It's possible we've misread the bug. You can prevent that by including reasons for why this patch is needed and the benefits outweigh the cost and risk when you request approval (which you should do for every branch request).
Attachment #440988 - Flags: approval1.9.2.13?
Attachment #440988 - Flags: approval1.9.2.13-
Attachment #440988 - Flags: approval1.9.1.16?
Attachment #440988 - Flags: approval1.9.1.16-
Comment on attachment 440988 [details] [diff] [review]
Unbitrot

(In reply to comment #27)

> This bug says it "comes from bug 482935" and is marked that way in the

Yes, this leak was narrowed down while investigating bug 482935.

> dependency tree, but it doesn't appear bug 482935 ever landed on the 1.9.2

Yes, fixing this leak had to happen before checking in the mochitest in bug 482935.

> branch. Don't want to land a patch we don't need.

You misread this dependency: this leak is not a regression from bug 482935 and blocks it.

This code already existed in 1.9.1(-), thus I assume the leak exists on branches too and we should want to fix it (fwiw then maybe to land bug 482935 there too later).

I don't think this patch has any high cost nor risk, specially as it has a test of its own.
Attachment #440988 - Flags: approval1.9.2.13?
Attachment #440988 - Flags: approval1.9.2.13-
Attachment #440988 - Flags: approval1.9.1.16?
Attachment #440988 - Flags: approval1.9.1.16-
Comment on attachment 440988 [details] [diff] [review]
Unbitrot

Missed 1.9.2.13 and 1.9.1.16. If we want this in a future release we'd like to see the people involved in creating the patch (Sicking or Bjarne) say this patch applies and is appropriate for the branches. Don't know what this could possibly break, but if it breaks _anything_ that's worse than a leak we can live with.
Attachment #440988 - Flags: approval1.9.2.13?
Attachment #440988 - Flags: approval1.9.2.13-
Attachment #440988 - Flags: approval1.9.1.16?
Attachment #440988 - Flags: approval1.9.1.16-
You need to log in before you can comment on or make changes to this bug.