483818 - Leak when script appended via DOM does document.write

Status: VERIFIED FIXED

(Core :: DOM: HTML Parser, defect)

Description

[Boris Zbarsky [:bzbarsky]]

Testcase coming up.  This caused leaks during mochitest on the test for bug 482659.

Comment 1

[Boris Zbarsky [:bzbarsky]]

Oh, and once this is fixed the relevant part of that mochitest should be reenabled.

Comment 2

[Boris Zbarsky [:bzbarsky]]

On all branches, of course.

Comment 3

[Boris Zbarsky [:bzbarsky]]

Attachment: 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

Comment 4

[Boris Zbarsky [:bzbarsky]]

OK, it looks like the nsContentSink gets a ScriptEvaluated but not a ScriptAvailable for that script.  Or something.

Comment 5

[Boris Zbarsky [:bzbarsky]]

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

Comment 6

[Boris Zbarsky [:bzbarsky]]

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?

Comment 7

[Brendan Eich [:brendan]]

Whose bug is this? (I mean who should take assignment?)

/be

Comment 8

[Boris Zbarsky [:bzbarsky]]

Some combination of Blake, sicking, Peter, me, I would think...

Comment 9

[Blake Kaplan (:mrbkap) (inactive)]

I'll take this.

Comment 10

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

Attachment: Patch to fix

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 11

[Blake Kaplan (:mrbkap) (inactive)]

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".

Comment 12

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

Attachment: Review comments fixed

Also renamed the function to 'IsScriptExecuting' per discussion with mrbkap.

Comment 13

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

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

Comment 14

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

Don't think we need a litmus test given that there's an automated test in the patch that landed which should test this

Comment 15

[Martijn Wargers (dead)]

Sorry, pushed the wrong button.

Comment 16

[Daniel Veditz [:dveditz]]

Do we need a separate 1.9.0 patch or will this apply?

Comment 17

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

Needs a separate patch since it's changing interfaces :( I will write one.

Comment 18

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

Attachment: 1.9.0 branch fix

Same as trunk patch except without changing the nsIContentSink interface, so forwarding r/sr after checking with mrbkap.

Comment 19

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

Checked in to 1.9.0 branch. Asking for retroactive approval given that I checked in just a few mins before freeze

Comment 20

[Olli Pettay [:smaug][[e-mail]]]

I think this caused 1.9.0 linux unit test orange.

Comment 21

[Olli Pettay [:smaug][[e-mail]]]

Seems like the commit missed the test file.

Comment 22

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

Indeed, forgetting the crappyness of CVS was something my brain gladly did.

Comment 23

[Samuel Sidler (old account; do not CC)]

Comment on attachment 374235 [details] [diff] [review]
1.9.0 branch fix

Retroactive... Approved for 1.9.0.11. a=ss

(Note: We re-versioned 1.9.0.10 to be 1.9.0.11 and have extended code freeze by a week or so.)

Comment 24

[Aakash Desai [:aakashd]]

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

Comment 25

[Al Billings [:abillings - ex-MoCo]]

Verified fixed for 1.9.0.11. Tested with my debug build (Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.0.11pre) Gecko/2009051111 GranParadiso/3.0.11pre).