Leak txUnknownHandler+ with transformToDocument(textnode)

RESOLVED FIXED in mozilla6

Status

()

Core
XSLT
RESOLVED FIXED
7 years ago
6 years ago

People

(Reporter: Jesse Ruderman, Assigned: peterv)

Tracking

(Blocks: 2 bugs, {mlk, testcase})

Trunk
mozilla6
mlk, testcase
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(blocking2.0 -)

Details

Attachments

(3 attachments, 2 obsolete attachments)

(Reporter)

Description

7 years ago
Created attachment 482745 [details]
testcase
We're actually leaking quite a bit more than this, but none of these classes are annotated.
With more classes marked, the root of the leak becomes apparent.  We're leaking a txUnknownHandler when the call at http://mxr.mozilla.org/mozilla-central/source/content/xslt/src/xslt/txUnknownHandler.cpp#67 because we return before deleting the txUnknownHandler.  Hopefully this is enough information for someone who knows this code to fix this.
Created attachment 483732 [details] [diff] [review]
Mark classes for leak logging

This makes the leak here more apparent.  I also added MOZ_COUNT_[C|D]TOR_INHERITED for more accurate stats on classes that inherit from a class that is already marked for leak logging.
Attachment #483732 - Flags: superreview?(dbaron)
Attachment #483732 - Flags: review?(jonas)
(Reporter)

Updated

7 years ago
Summary: Leak nsStringBuffer, nsTArray_base with transformToDocument(textnode) → Leak txUnknownHandler+ with transformToDocument(textnode)
Comment on attachment 483732 [details] [diff] [review]
Mark classes for leak logging

r=me, but definitely want dbarons input on the new macros
Attachment #483732 - Flags: review?(jonas) → review+
This should be easy enough to fix now that the problem is known.  Note that this same pattern happens several times in that file, so it's probably possible to similar testcases that leak different things.
blocking2.0: --- → ?
Assignee: nobody → jonas
blocking2.0: ? → final+
(Assignee)

Comment 6

7 years ago
Created attachment 488206 [details] [diff] [review]
v1
Assignee: jonas → peterv
Status: NEW → ASSIGNED
Attachment #488206 - Flags: review?(jonas)
(Assignee)

Comment 7

7 years ago
Fix is in txUnknownHandler::createHandler, the rest is mostly cleanup.
(Assignee)

Comment 8

7 years ago
There's still an assertion that we're hitting (because we don't have a root node in the source, style or result documents).
Comment on attachment 488206 [details] [diff] [review]
v1

Huh?? So none of the flushToHandler implementations replaced the handler? Was that just a leftover?
Attachment #488206 - Flags: review?(jonas) → review+
Comment on attachment 483732 [details] [diff] [review]
Mark classes for leak logging

sr=dbaron
Attachment #483732 - Flags: superreview?(dbaron) → superreview+
http://hg.mozilla.org/mozilla-central/rev/40b718853f48
http://hg.mozilla.org/mozilla-central/rev/feb478675a92
Status: ASSIGNED → RESOLVED
Last Resolved: 7 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla2.0b8
Backed out due to

64 ERROR TEST-UNEXPECTED-FAIL | /tests/content/xslt/tests/mochitest/test_bug551412.html | Result of transform should be '1 <b>2</b> 3' - got "<b></b>", expected "1 <b>2</b> 3"

http://hg.mozilla.org/mozilla-central/rev/c994a3574125
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
The test assert too, which needs to be noted.
Not going to block on this at this late point. If a safe fix for this appears, we would consider taking it.
blocking2.0: final+ → -
(Assignee)

Comment 15

6 years ago
Created attachment 505406 [details] [diff] [review]
v1.1

The patch did this:

            nsAFlatString::const_char_iterator& start = aIter;
            nsAFlatString::const_char_iterator end =
                start + charTransaction->mLength;
            aIter = end;

start is a reference to aIter, so at this point start == aIter == end, and then we do:

            return aHandler->characters(Substring(start, end),

Changing start to not be a reference fixed it.
Attachment #488206 - Attachment is obsolete: true
Attachment #505406 - Flags: review?(jonas)
Attachment #505406 - Flags: review?(jonas) → review+
(Assignee)

Comment 16

6 years ago
Created attachment 524674 [details] [diff] [review]
v1.2

With fixes for an assertion triggered by the test. Got r=sicking in person.
Attachment #505406 - Attachment is obsolete: true
Attachment #524674 - Flags: review+
(Assignee)

Comment 17

6 years ago
http://hg.mozilla.org/mozilla-central/rev/a6822a5df633
http://hg.mozilla.org/mozilla-central/rev/c1ef3c3740cb
http://hg.mozilla.org/mozilla-central/rev/47f6e81257f9
Status: REOPENED → RESOLVED
Last Resolved: 7 years ago6 years ago
OS: Mac OS X → All
Hardware: x86 → All
Resolution: --- → FIXED
Target Milestone: mozilla2.0b8 → mozilla6

Updated

6 years ago
Blocks: 667315
No longer blocks: 667315
Depends on: 667315

Comment 18

6 years ago
 Mozilla/5.0 (X11; Linux i686; rv:8.0a1) Gecko/20110719 Firefox/8.0a1

Running the test-case from Comment 0, displays a Rectangle with a 0 inside of it. Was that the intended behavior for this patch?

Thanks!
(Reporter)

Comment 19

6 years ago
I'm guessing the patch wasn't intended to change the appearance of the testcase, just whether it causes Firefox to report a memory leak when it shuts down after displaying the testcase.
You need to log in before you can comment on or make changes to this bug.