Closed Bug 603844 Opened 14 years ago Closed 13 years ago

Leak txUnknownHandler+ with transformToDocument(textnode)

Categories

(Core :: XSLT, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla6
Tracking Status
blocking2.0 --- -

People

(Reporter: jruderman, Assigned: peterv)

References

Details

(Keywords: memory-leak, testcase)

Attachments

(3 files, 2 obsolete files)

Attached file testcase
      No description provided.
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.
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)
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+
Attached patch v1 (obsolete) — Splinter Review
Assignee: jonas → peterv
Status: NEW → ASSIGNED
Attachment #488206 - Flags: review?(jonas)
Fix is in txUnknownHandler::createHandler, the rest is mostly cleanup.
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
Closed: 14 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+ → -
Attached patch v1.1 (obsolete) — Splinter Review
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)
Attached patch v1.2Splinter Review
With fixes for an assertion triggered by the test. Got r=sicking in person.
Attachment #505406 - Attachment is obsolete: true
Attachment #524674 - Flags: review+
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
Closed: 14 years ago13 years ago
OS: Mac OS X → All
Hardware: x86 → All
Resolution: --- → FIXED
Target Milestone: mozilla2.0b8 → mozilla6
Blocks: 667315
No longer blocks: 667315
Depends on: 667315
 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!
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.

Attachment

General

Creator:
Created:
Updated:
Size: