294274 - [FIXr]Memory leak if content sink appends script to node not in document

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect, P1)

Description

[Boris Zbarsky [:bzbarsky]]

See the test page in bug 292033.  I'll also attach a minimal-ish testcase.  We
never stop the throbber, and leak the document...

Comment 1

[Boris Zbarsky [:bzbarsky]]

Attachment: Testcase

Comment 2

[Boris Zbarsky [:bzbarsky]]

Attachment: Proposed patch

Comment 3

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Comment on attachment 183657 [details] [diff] [review]
Proposed patch

won't the xml sink need the same fix? As should the xslt output handler.

Comment 4

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

opps, didn't mean to mark that r+ just yet. Lets just say it's an r+ pending an
answer to that question :)

Comment 5

[Boris Zbarsky [:bzbarsky]]

Attachment: XML testcase

ML will need a similar fix, though it's a bit more work there because we don't
really have the parent node at the place where we currently decide to block the
parser....  I'm not quite sure what the best approach here is.

For XSLT, I really don't know what the right approach is; help there would be
much appreciated.

Comment 6

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

IMHO you can just kill ProcessEndSCRIPTTag. Then forward |parent| to
CloseElement and you should be good to go. Just keep in mind that parent can be
null when script is the root element.

For XSLT the class you want is txMozillaXMLOutput. Check if mCurrentNode ==
mNonAddedNode and then the parent is mNonAddedParent. You'll have to add this
code twice, once for svg-script (in endElement) and once for html (in
endHTMLElement)

Comment 7

[Boris Zbarsky [:bzbarsky]]

> For XSLT the class you want is txMozillaXMLOutput. Check if mCurrentNode ==
> mNonAddedNode and then the parent is mNonAddedParent.

I don't see how that helps....  Unless this class is listening to DOM mutations
and updating those members as needed?

Comment 8

[Boris Zbarsky [:bzbarsky]]

Attachment: not quite there yet

This fixes the throbbing for XML, but it still leaks.. no idea why.

And then there's XUL, of course... :(

Comment 9

[Boris Zbarsky [:bzbarsky]]

Ah, with XML I leak even if there is no second script.  Any idea why?

Comment 10

[Boris Zbarsky [:bzbarsky]]

Attachment: Leaking testcase for XML

Comment 11

[Boris Zbarsky [:bzbarsky]]

Not going to have time to really debug this anytime soon....

Comment 12

[Boris Zbarsky [:bzbarsky]]

Attachment: Real leaking testcase for XML

Comment 13

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

(In reply to comment #7)
> > For XSLT the class you want is txMozillaXMLOutput. Check if mCurrentNode ==
> > mNonAddedNode and then the parent is mNonAddedParent.
> 
> I don't see how that helps....  Unless this class is listening to DOM mutations
> and updating those members as needed?

I mean that we should check if mNonAddedParent->GetCurrentDoc() returns the
appropriate document.

Comment 14

[Boris Zbarsky [:bzbarsky]]

So for XSLT the script should be added to the notifier if:

1)  mCurrentNode != mNonAddedNode or
2)  mNonAddedParent is in the right document,

right?

Any testcases I can use to test what effect that has?

Comment 15

[Boris Zbarsky [:bzbarsky]]

Attachment: Patch that fixes the XML leaks

The PopContent() changes in the XML fragment sink are what fix the XML leaks.

The XSLT changes are untested; if you have tests you want me to run, please let
me know...

Comment 16

[Boris Zbarsky [:bzbarsky]]

sicking, peterv, could we possibly get this reviewed?  I've got less than a week
left to land it... and jst's already pretty swamped with review requests.

Comment 17

[Peter Van der Beken [:peterv]]

Comment on attachment 183695 [details] [diff] [review]
Patch that fixes the XML leaks

As discussed, let's split off the transformiix stuff.

Comment 18

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 183695 [details] [diff] [review]
Patch that fixes the XML leaks

Requesting approval for this memory leak fix (just the HTML/XML parts).

Comment 19

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Yeah, those are the only parts i'm unsure about too, i'd already reviewed the rest.

Comment 20

[chris hofmann]

Comment on attachment 183695 [details] [diff] [review]
Patch that fixes the XML leaks

a=chofmann

Comment 21

[Boris Zbarsky [:bzbarsky]]

Filed bug 296334 for the XSLT issue; checked in the rest for 1.8b3.