Closed
Bug 603844
Opened 14 years ago
Closed 13 years ago
Leak txUnknownHandler+ with transformToDocument(textnode)
Categories
(Core :: XSLT, defect)
Core
XSLT
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)
640 bytes,
text/html
|
Details | |
16.15 KB,
patch
|
sicking
:
review+
dbaron
:
superreview+
|
Details | Diff | Splinter Review |
25.72 KB,
patch
|
peterv
:
review+
|
Details | Diff | Splinter Review |
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)
Reporter | ||
Updated•14 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•14 years ago
|
||
Assignee | ||
Comment 7•14 years ago
|
||
Fix is in txUnknownHandler::createHandler, the rest is mostly cleanup.
Assignee | ||
Comment 8•14 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
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.
Comment 14•14 years ago
|
||
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•13 years ago
|
||
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•13 years ago
|
||
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•13 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
Closed: 14 years ago → 13 years ago
OS: Mac OS X → All
Hardware: x86 → All
Resolution: --- → FIXED
Target Milestone: mozilla2.0b8 → mozilla6
Updated•13 years ago
|
Comment 18•13 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•13 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.
Description
•