Last Comment Bug 603844 - Leak txUnknownHandler+ with transformToDocument(textnode)
: Leak txUnknownHandler+ with transformToDocument(textnode)
Status: RESOLVED FIXED
: mlk, testcase
Product: Core
Classification: Components
Component: XSLT (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla6
Assigned To: Peter Van der Beken [:peterv]
:
Mentors:
Depends on: 667315
Blocks: 326633 594645
  Show dependency treegraph
 
Reported: 2010-10-12 18:12 PDT by Jesse Ruderman
Modified: 2011-07-19 22:45 PDT (History)
8 users (show)
khuey: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
-


Attachments
testcase (640 bytes, text/html)
2010-10-12 18:12 PDT, Jesse Ruderman
no flags Details
Mark classes for leak logging (16.15 KB, patch)
2010-10-16 08:55 PDT, Kyle Huey [:khuey] (khuey@mozilla.com)
jonas: review+
dbaron: superreview+
Details | Diff | Review
v1 (14.28 KB, patch)
2010-11-04 09:39 PDT, Peter Van der Beken [:peterv]
jonas: review+
Details | Diff | Review
v1.1 (14.39 KB, patch)
2011-01-20 07:10 PST, Peter Van der Beken [:peterv]
jonas: review+
Details | Diff | Review
v1.2 (25.72 KB, patch)
2011-04-08 10:44 PDT, Peter Van der Beken [:peterv]
peterv: review+
Details | Diff | Review

Description Jesse Ruderman 2010-10-12 18:12:32 PDT
Created attachment 482745 [details]
testcase
Comment 1 Kyle Huey [:khuey] (khuey@mozilla.com) 2010-10-16 08:27:12 PDT
We're actually leaking quite a bit more than this, but none of these classes are annotated.
Comment 2 Kyle Huey [:khuey] (khuey@mozilla.com) 2010-10-16 08:53:58 PDT
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.
Comment 3 Kyle Huey [:khuey] (khuey@mozilla.com) 2010-10-16 08:55:57 PDT
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.
Comment 4 Jonas Sicking (:sicking) 2010-10-16 12:04:49 PDT
Comment on attachment 483732 [details] [diff] [review]
Mark classes for leak logging

r=me, but definitely want dbarons input on the new macros
Comment 5 Kyle Huey [:khuey] (khuey@mozilla.com) 2010-10-21 18:18:58 PDT
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.
Comment 6 Peter Van der Beken [:peterv] 2010-11-04 09:39:06 PDT
Created attachment 488206 [details] [diff] [review]
v1
Comment 7 Peter Van der Beken [:peterv] 2010-11-04 09:39:55 PDT
Fix is in txUnknownHandler::createHandler, the rest is mostly cleanup.
Comment 8 Peter Van der Beken [:peterv] 2010-11-04 09:41:14 PDT
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 9 Jonas Sicking (:sicking) 2010-11-10 19:31:22 PST
Comment on attachment 488206 [details] [diff] [review]
v1

Huh?? So none of the flushToHandler implementations replaced the handler? Was that just a leftover?
Comment 10 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2010-11-22 10:22:45 PST
Comment on attachment 483732 [details] [diff] [review]
Mark classes for leak logging

sr=dbaron
Comment 12 Kyle Huey [:khuey] (khuey@mozilla.com) 2010-12-02 09:32:23 PST
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
Comment 13 Kyle Huey [:khuey] (khuey@mozilla.com) 2010-12-02 10:07:44 PST
The test assert too, which needs to be noted.
Comment 14 Johnny Stenback (:jst, jst@mozilla.com) 2010-12-09 17:24:50 PST
Not going to block on this at this late point. If a safe fix for this appears, we would consider taking it.
Comment 15 Peter Van der Beken [:peterv] 2011-01-20 07:10:47 PST
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.
Comment 16 Peter Van der Beken [:peterv] 2011-04-08 10:44:48 PDT
Created attachment 524674 [details] [diff] [review]
v1.2

With fixes for an assertion triggered by the test. Got r=sicking in person.
Comment 18 George Carstoiu 2011-07-19 06:27:50 PDT
 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!
Comment 19 Jesse Ruderman 2011-07-19 22:45:03 PDT
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.

Note You need to log in before you can comment on or make changes to this bug.