Closed Bug 483818 Opened 13 years ago Closed 13 years ago

Leak when script appended via DOM does document.write


(Core :: DOM: HTML Parser, defect)

Not set





(Reporter: bzbarsky, Assigned: sicking)



(4 keywords)


(3 files, 1 obsolete file)

Testcase coming up.  This caused leaks during mochitest on the test for bug 482659.
Oh, and once this is fixed the relevant part of that mochitest should be reenabled.
On all branches, of course.
Attached file Testcase
This leaks.  It also asserts:

###!!! ASSERTION: Too many calls to ScriptDidExecute: 'mScriptsExecuting > 0', file /Users/bzbarsky/mozilla/debug/mozilla/parser/htmlparser/src/nsParser.cpp, line 1824
OK, it looks like the nsContentSink gets a ScriptEvaluated but not a ScriptAvailable for that script.  Or something.
Flags: blocking1.9.1?
And the key to that is this stack:

#0  nsScriptLoader::AddObserver (this=0xa973e0, aObserver=0x32d10c0) at nsScriptLoader.h:88
#1  0x0381117a in nsContentSink::Init (this=0x1440a00, aDoc=0x161f400, aURI=0xd803490, aContainer=0xc70f070, aChannel=0xd8c0060) at /Users/bzbarsky/mozilla/debug/mozilla/content/base/src/nsContentSink.cpp:251
#2  0x039c8a16 in HTMLContentSink::Init (this=0x1440a00, aDoc=0x161f400, aURI=0xd803490, aContainer=0xc70f070, aChannel=0xd8c0060) at /Users/bzbarsky/mozilla/debug/mozilla/content/html/document/src/nsHTMLContentSink.cpp:1642
#3  0x039c91e0 in NS_NewHTMLContentSink (aResult=0xbfff9e20, aDoc=0x161f400, aURI=0xd803490, aContainer=0xc70f070, aChannel=0xd8c0060) at /Users/bzbarsky/mozilla/debug/mozilla/content/html/document/src/nsHTMLContentSink.cpp:1519
#4  0x039dc00c in nsHTMLDocument::OpenCommon (this=0x161f400, aContentType=@0xbfff9f84, aReplace=0) at /Users/bzbarsky/mozilla/debug/mozilla/content/html/document/src/nsHTMLDocument.cpp:2003
#5  0x039dc362 in nsHTMLDocument::Open (this=0x161f400, aContentType=@0xbfff9f84, aReplace=0, aReturn=0xbfff9f7c) at /Users/bzbarsky/mozilla/debug/mozilla/content/html/document/src/nsHTMLDocument.cpp:2059
#6  0x039d3cd6 in nsHTMLDocument::Open (this=0x161f400) at /Users/bzbarsky/mozilla/debug/mozilla/content/html/document/src/nsHTMLDocument.cpp:2052
#7  0x039d24e9 in nsHTMLDocument::WriteCommon (this=0x161f400, aText=@0xbfffa11c, aNewlineTerminate=0) at /Users/bzbarsky/mozilla/debug/mozilla/content/html/document/src/nsHTMLDocument.cpp:2160
If I add a check to avoid decrementing mScriptsExecuting when it's 0, the leak goes away.

Almost certainly a regression from bug 444322, so needed on 1.9.0 too.

As far as I can see, an arbitrary number of scripts can be in the middle of executing when the content sink is suddenly created.  Would it make sense to keep track of this "number of scripts executing" thing on the document instead of on the parser or something?
Flags: blocking1.9.0.9?
Blocks: 444322
Whose bug is this? (I mean who should take assignment?)

OS: Mac OS X → All
Hardware: x86 → All
Some combination of Blake, sicking, Peter, me, I would think...
Flags: blocking1.9.1? → blocking1.9.1+
I'll take this.
Assignee: nobody → mrbkap
Assignee: mrbkap → jonas
Flags: wanted1.9.0.x+
Flags: blocking1.9.0.10?
Flags: blocking1.9.0.10+
Keywords: regression
Attached patch Patch to fix (obsolete) — Splinter Review
Patch + testcase.

Instead of having the parser keep track of if we're executing scripts or not, simply ask the scriptloader, it already knows!
Attachment #372979 - Flags: superreview?(mrbkap)
Attachment #372979 - Flags: review?(mrbkap)
Comment on attachment 372979 [details] [diff] [review]
Patch to fix

>+   * Returns true if there's currently scripts executing that we need to hold

"there's" should be "there are".
Attachment #372979 - Flags: superreview?(mrbkap)
Attachment #372979 - Flags: superreview+
Attachment #372979 - Flags: review?(mrbkap)
Attachment #372979 - Flags: review+
Also renamed the function to 'IsScriptExecuting' per discussion with mrbkap.
Attachment #372979 - Attachment is obsolete: true
Attachment #372994 - Flags: superreview+
Attachment #372994 - Flags: review+
Checked in to m-c and 1.9.1 branches.
Closed: 13 years ago
Keywords: fixed1.9.1
Resolution: --- → FIXED
Flags: in-litmus?
Don't think we need a litmus test given that there's an automated test in the patch that landed which should test this
Flags: in-testsuite+
Flags: in-litmus?
Flags: in-litmus-
Sorry, pushed the wrong button.
Do we need a separate 1.9.0 patch or will this apply?
Needs a separate patch since it's changing interfaces :( I will write one.
Attached patch 1.9.0 branch fixSplinter Review
Same as trunk patch except without changing the nsIContentSink interface, so forwarding r/sr after checking with mrbkap.
Attachment #374235 - Flags: superreview+
Attachment #374235 - Flags: review+
Checked in to 1.9.0 branch. Asking for retroactive approval given that I checked in just a few mins before freeze
Keywords: fixed1.9.0
I think this caused 1.9.0 linux unit test orange.
Seems like the commit missed the test file.
Indeed, forgetting the crappyness of CVS was something my brain gladly did.
Comment on attachment 374235 [details] [diff] [review]
1.9.0 branch fix

Retroactive... Approved for a=ss

(Note: We re-versioned to be and have extended code freeze by a week or so.)
Attachment #374235 - Flags: approval1.9.0.11? → approval1.9.0.11+
verified FIXED with the attached testcase on builds:

Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.2a1pre) Gecko/20090430 Minefield/3.6a1pre ID:20090430031133


Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.1b5pre) Gecko/20090430 Shiretoko/3.5b5pre ID:20090430031222
Verified fixed for Tested with my debug build (Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv: Gecko/2009051111 GranParadiso/3.0.11pre).
You need to log in before you can comment on or make changes to this bug.