Closed Bug 483818 Opened 11 years ago Closed 11 years ago
Leak when script appended via DOM does document
298 bytes, text/html
9.48 KB, patch
|Details | Diff | Splinter Review|
13.73 KB, patch
|Details | Diff | Splinter Review|
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.
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.
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?
Whose bug is this? (I mean who should take assignment?) /be
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
Patch + testcase. Instead of having the parser keep track of if we're executing scripts or not, simply ask the scriptloader, it already knows!
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".
Also renamed the function to 'IsScriptExecuting' per discussion with mrbkap.
Checked in to m-c and 1.9.1 branches. http://hg.mozilla.org/mozilla-central/rev/4ec870426c6a http://hg.mozilla.org/releases/mozilla-1.9.1/rev/894e9e3bdf3e
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Don't think we need a litmus test given that there's an automated test in the patch that landed which should test this
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.
Same as trunk patch except without changing the nsIContentSink interface, so forwarding r/sr after checking with mrbkap.
Checked in to 1.9.0 branch. Asking for retroactive approval given that I checked in just a few mins before freeze
Attachment #374235 - Flags: approval188.8.131.52?
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 184.108.40.206. a=ss (Note: We re-versioned 220.127.116.11 to be 18.104.22.168 and have extended code freeze by a week or so.)
Attachment #374235 - Flags: approval22.214.171.124? → approval126.96.36.199+
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 and 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 188.8.131.52. Tested with my debug build (Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:184.108.40.206pre) Gecko/2009051111 GranParadiso/3.0.11pre).
You need to log in before you can comment on or make changes to this bug.