Closed Bug 294274 Opened 19 years ago Closed 19 years ago

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

Categories

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

defect

Tracking

()

RESOLVED FIXED
mozilla1.8beta3

People

(Reporter: bzbarsky, Assigned: bzbarsky)

References

Details

(Keywords: helpwanted)

Attachments

(4 files, 3 obsolete files)

See the test page in bug 292033.  I'll also attach a minimal-ish testcase.  We
never stop the throbber, and leak the document...
Attached file Testcase
Attached patch Proposed patch (obsolete) — Splinter Review
Attachment #183657 - Flags: superreview?(peterv)
Attachment #183657 - Flags: review?(bugmail)
Priority: -- → P1
Summary: Memory leak if content sink appends script to node not in document → [FIx]Memory leak if content sink appends script to node not in document
Target Milestone: --- → mozilla1.8beta2
Comment on attachment 183657 [details] [diff] [review]
Proposed patch

won't the xml sink need the same fix? As should the xslt output handler.
Attachment #183657 - Flags: review?(bugmail) → review+
opps, didn't mean to mark that r+ just yet. Lets just say it's an r+ pending an
answer to that question :)
Attached file 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.
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)
> 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?
Attached patch not quite there yet (obsolete) — Splinter Review
This fixes the throbbing for XML, but it still leaks.. no idea why.

And then there's XUL, of course... :(
Attachment #183664 - Attachment is obsolete: true
Ah, with XML I leak even if there is no second script.  Any idea why?
Attached file Leaking testcase for XML (obsolete) —
Not going to have time to really debug this anytime soon....
Assignee: bzbarsky → general
OS: Linux → All
Priority: P1 → --
Hardware: PC → All
Target Milestone: mozilla1.8beta2 → ---
Summary: [FIx]Memory leak if content sink appends script to node not in document → Memory leak if content sink appends script to node not in document
Attachment #183657 - Attachment is obsolete: true
Attachment #183671 - Attachment is obsolete: true
Attachment #183673 - Attachment is obsolete: true
Attachment #183664 - Attachment is obsolete: false
Attachment #183657 - Flags: superreview?(peterv) → superreview-
Keywords: helpwanted
(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.
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?

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...
Attachment #183695 - Flags: superreview?(peterv)
Attachment #183695 - Flags: review?(bugmail)
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.
Assignee: general → bzbarsky
Priority: -- → P1
Summary: Memory leak if content sink appends script to node not in document → [FIX]Memory leak if content sink appends script to node not in document
Target Milestone: --- → mozilla1.8beta3
Comment on attachment 183695 [details] [diff] [review]
Patch that fixes the XML leaks

As discussed, let's split off the transformiix stuff.
Attachment #183695 - Flags: superreview?(peterv)
Attachment #183695 - Flags: superreview+
Attachment #183695 - Flags: review?(bugmail)
Attachment #183695 - Flags: review+
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).
Attachment #183695 - Flags: approval1.8b3?
Yeah, those are the only parts i'm unsure about too, i'd already reviewed the rest.
Summary: [FIX]Memory leak if content sink appends script to node not in document → [FIXr]Memory leak if content sink appends script to node not in document
Comment on attachment 183695 [details] [diff] [review]
Patch that fixes the XML leaks

a=chofmann
Attachment #183695 - Flags: approval1.8b3? → approval1.8b3+
Filed bug 296334 for the XSLT issue; checked in the rest for 1.8b3.
Status: NEW → RESOLVED
Closed: 19 years ago
Resolution: --- → FIXED
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: