197052 - crash if modification innerHTML of element in this element [@ js_EmitTree ]

Status: VERIFIED FIXED

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

Description

[Roman Rahman]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.2) Gecko/20020826 MultiZilla/v1.1.22
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.2) Gecko/20020826 MultiZilla/v1.1.22

Some problem in processing recursion with _modification_ innerHTML of element in
this element.

Sample 1.
<div id="uniqid">
<script language="JavaScript" type="text/JavaScript">
document.write("&gt;"+document.getElementById('uniqid').innerHTML+"&lt;");
</script>
</div>

Sample 2.
<div id="uniqid">
<script language="JavaScript" type="text/JavaScript">
alert("&gt;"+document.getElementById('uniqid').innerHTML+"&lt;");
</script>
</div>

Have Mozilla CRASH in Sample 1, and normal work in Sample 2.

Sample 3.
<div id="uniqid">
<script language="JavaScript" type="text/JavaScript">
document.getElementById('uniqid').innerHTML += "xxx";
</script>
</div>

I see "xxx xxx" in place "xxx". Problem in re-render HTML, but imho this bugs
related.

Reproducible: Always

Steps to Reproduce:
1.
2.
3.

Comment 1

[Olivier Cahagne]

can you attach the testcase and reproduce crash with 1.3 release candidate ?
If so, please post a Talkback ID for this crash
"mozilla/bin/components/talkback.exe"

Comment 2

[Chris Casciano]

Attachment: testcase

test case for code snippet 1

Comment 3

[Chris Casciano]

Attachment: mozilla-bin crashlog - OS X

Crash was in mozilla-bin on OS X... Talkback not envoked... crash at
nsJSContext::EvaluateString (?)

Comment 4

[Chris Casciano]

-> All/All (OS X 1.3RC Build)

didn't go looking for dupes, so not confirming yet

Comment 5

[Zibi Braniecki [:zbraniecki][:gandalf]]

Confirming with Mozilla 1.3RC 20030311 Windows

Making as NEW. (Could not find and dupe for this) dupeme!

TB18021740Y

Comment 6

[Johnny Stenback (:jst)]

Hmm, I don't immediately see how this could be fixed w/o limiting the nesting
depth of document.write(). Doing that would be easy enough, I'll see what I can do.

Comment 7

[Zibi Braniecki [:zbraniecki][:gandalf]]

It's similar behaviour as with loading document into iframe of this document. So
probably limiting the nesting is the only fix.

Comment 8

[Johnny Stenback (:jst)]

Mass-reassigning bugs.

Comment 9

[Olivier Cahagne]

confirming it also crashes Mozilla on Linux (20030323 CVS) in js_EmitTree:

#0  0x400429dc in js_EmitTree (cx=0x8a5aa48, cg=0xbfe024b0, pn=0x92fadf0)
    at jsemit.c:1956
#1  0x40042281 in EmitPropOp (cx=0x8a5aa48, pn=0x92fae20, op=JSOP_GETPROP,
    cg=0xbfe024b0) at jsemit.c:1747
#2  0x40046524 in js_EmitTree (cx=0x8a5aa48, cg=0xbfe024b0, pn=0x92fae20)
    at jsemit.c:3733
#3  0x4004656b in js_EmitTree (cx=0x8a5aa48, cg=0xbfe024b0, pn=0x8b26f60)
    at jsemit.c:3754
#4  0x4004587a in js_EmitTree (cx=0x8a5aa48, cg=0xbfe024b0, pn=0x8b27110)
    at jsemit.c:3324
#5  0x40076530 in Statements (cx=0x8a5aa48, ts=0x92faa78, tc=0xbfe024b0)
    at jsparse.c:974
#6  0x40075721 in js_CompileTokenStream (cx=0x8a5aa48, chain=0x88541d0,
    ts=0x92faa78, cg=0xbfe024b0) at jsparse.c:452
#7  0x4002c0f5 in CompileTokenStream (cx=0x8a5aa48, obj=0x88541d0,
    ts=0x92faa78, tempMark=0x8a5aac8, eofp=0x0) at jsapi.c:2945
#8  0x4002c317 in JS_CompileUCScriptForPrincipals (cx=0x8a5aa48,
    obj=0x88541d0, principals=0x8b69668, chars=0x908f228, length=76,
    filename=0x8e48f80
"http://bugzilla.mozilla.org/attachment.cgi?id=117037&action=view", lineno=1040)
at jsapi.c:3025
#9  0x4002cea3 in JS_EvaluateUCScriptForPrincipals (cx=0x8a5aa48,
    obj=0x88541d0, principals=0x8b69668, chars=0x908f228, length=76,
    filename=0x8e48f80
"http://bugzilla.mozilla.org/attachment.cgi?id=117037&action=view", lineno=1040,
rval=0xbfe02628) at jsapi.c:3474
#10 0x418c909b in nsJSContext::EvaluateString (this=0x8a5aa08,
    aScript=@0xbfe02860, aScopeObject=0x88541d0, aPrincipal=0x8b69664,
    aURL=0x8e48f80
"http://bugzilla.mozilla.org/attachment.cgi?id=117037&action=view",
aLineNo=1040, aVersion=0x40097989 "default", aRetValue=@0xbfe02770,
    aIsUndefined=0xbfe02704) at nsJSEnvironment.cpp:708
#11 0x40f99fc2 in nsScriptLoader::EvaluateScript (this=0x8bc6688,
    aRequest=0x88b2760, aScript=@0xbfe02860) at nsScriptLoader.cpp:656
[...]

Comment 10

[Johnny Stenback (:jst)]

Brendan, does this look familiar?

Comment 11

[Brendan Eich [:brendan]]

Phil, can you try to reduce a testcase into pure-JS, no DOM required?  Thanks,

/be

Comment 12

[Phil Schwartau]

Trying to reduce this to pure JS. Anyway, I have reduced 
the original DOM testcase to this:

<script>
var s = '\<script>document.write(s);<\/script>';
document.write(s);
</script>

The stack trace for the original testcase (Comment #2)
and the stack trace for this differ in only trivial ways.

Both crashes occur due to stack overflow, and both feature
this pattern, which repeats over and over:


                    ^
                    ^
nsGenericHTMLContainerElement::AppendChildTo() line 3750
HTMLContentSink::ProcessSCRIPTTag() line 5715
HTMLContentSink::AddLeaf() line 3592 + 12 bytes
                    ^
                    ^


The line |document.write(s)| adds another <script> element to
the content model containing the instruction |document.write(s)|,
which adds another <script> element containing |document.write(s)|,
and so on ...

If we execute all these intructions we run out of stack. Note,
IE6 does not seem to execute all of it, and loads all variations
of the testcase quickly.

Comment 13

[Phil Schwartau]

Spoke with jst about this; it doesn't look to be a JS Engine issue.
The testcase is creating an infinite number of JS compilation units;
that is something the embedding has to guard against, not JS Engine.

Comment 14

[Anders Pedersen]

(In reply to comment #2)
> Created an attachment (id=117037)
> testcase
> 
> test case for code snippet 1

Confirming crash using:

Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.7b) Gecko/20040330
Microsoft Windows 2000 Pro 5.00.2195 SP4

talkbackid: TB8920G

Comment 15

[Brendan Eich [:brendan]]

Need some talkback lovin' here.

/be

Comment 16

[chris hofmann]

Incident ID: 8920
Stack Signature	js_GetGCThingFlags 36d4dbfd
Email Address	[e-mail]
Product ID	MozillaTrunk
Build ID	2004033009
Trigger Time	2004-03-30 23:44:36.0
Platform	Win32
Operating System	Windows NT 5.0 build 2195
Module	js3250.dll + (00019c16)
URL visited	http://bugzilla.mozilla.org/attachment.cgi?id=117037&action=view
User Comments	- Opening the following testcase
http://bugzilla.mozilla.org/attachment.cgi?id=117037&action=view - Mozilla
crashes Bug http://bugzilla.mozilla.org/show_bug.cgi?id=197052
Since Last Crash	null sec
Total Uptime	null sec
Trigger Reason	Access violation
Source File Name
c:/builds/vc6/release-tinderbox/WINNT_5.1_Clobber/mozilla/js/src/jsgc.c
Trigger Line No.	219
Stack Trace 	
js_GetGCThingFlags
[c:/builds/vc6/release-tinderbox/WINNT_5.1_Clobber/mozilla/js/src/jsgc.c, line 219]
js_MarkGCThing
[c:/builds/vc6/release-tinderbox/WINNT_5.1_Clobber/mozilla/js/src/jsgc.c, line 834]
js_GC [c:/builds/vc6/release-tinderbox/WINNT_5.1_Clobber/mozilla/js/src/jsgc.c,
line 1213]
js_ForceGC
[c:/builds/vc6/release-tinderbox/WINNT_5.1_Clobber/mozilla/js/src/jsgc.c, line 1001]
JS_GC [c:/builds/vc6/release-tinderbox/WINNT_5.1_Clobber/mozilla/js/src/jsapi.c,
line 1682]
JS_MaybeGC
[c:/builds/vc6/release-tinderbox/WINNT_5.1_Clobber/mozilla/js/src/jsapi.c, line
1702]
nsJSContext::ScriptExecuted
[c:/builds/vc6/release-tinderbox/WINNT_5.1_Clobber/mozilla/dom/src/base/nsJSEnvironment.cpp,
line 1799]
nsXPCWrappedJSClass::CallQueryInterfaceOnJSObject
[c:/builds/vc6/release-tinderbox/WINNT_5.1_Clobber/mozilla/js/src/xpconnect/src/xpcwrappedjsclass.cpp,
line 256]
nsXPCWrappedJSClass::CallQueryInterfaceOnJSObject
[c:/builds/vc6/release-tinderbox/WINNT_5.1_Clobber/mozilla/js/src/xpconnect/src/xpcwrappedjsclass.cpp,
line 256]
nsXPCWrappedJSClass::GetRootJSObject
[c:/builds/vc6/release-tinderbox/WINNT_5.1_Clobber/mozilla/js/src/xpconnect/src/xpcwrappedjsclass.cpp,
line 603]
nsXPCWrappedJS::GetNewOrUsed
[c:/builds/vc6/release-tinderbox/WINNT_5.1_Clobber/mozilla/js/src/xpconnect/src/xpcwrappedjs.cpp,
line 237]
XPCConvert::JSObject2NativeInterface
[c:/builds/vc6/release-tinderbox/WINNT_5.1_Clobber/mozilla/js/src/xpconnect/src/xpcconvert.cpp,
line 1135]
nsXPCWrappedJSClass::DelegatedQueryInterface
[c:/builds/vc6/release-tinderbox/WINNT_5.1_Clobber/mozilla/js/src/xpconnect/src/xpcwrappedjsclass.cpp,
line 560]
nsXPCWrappedJS::QueryInterface
[c:/builds/vc6/release-tinderbox/WINNT_5.1_Clobber/mozilla/js/src/xpconnect/src/xpcwrappedjs.cpp,
line 98]
nsQueryInterface::operator()
[c:/builds/vc6/release-tinderbox/WINNT_5.1_Clobber/mozilla/xpcom/glue/nsCOMPtr.cpp,
line 52]
nsCOMPtr_base::assign_from_qi
[c:/builds/vc6/release-tinderbox/WINNT_5.1_Clobber/mozilla/xpcom/glue/nsCOMPtr.cpp,
line 97]
nsContentTreeOwner::SetStatus
[c:/builds/vc6/release-tinderbox/WINNT_5.1_Clobber/mozilla/xpfe/appshell/src/nsContentTreeOwner.cpp,
line 351]
nsWebShell::OnLeaveLink
[c:/builds/vc6/release-tinderbox/WINNT_5.1_Clobber/mozilla/docshell/base/nsWebShell.cpp,
line 696]
nsGenericElement::LeaveLink
[c:/builds/vc6/release-tinderbox/WINNT_5.1_Clobber/mozilla/content/base/src/nsGenericElement.cpp,
line 3140]
nsGenericHTMLElement::HandleDOMEventForAnchors
[c:/builds/vc6/release-tinderbox/WINNT_5.1_Clobber/mozilla/content/html/content/src/nsGenericHTMLElement.cpp,
line 1488]
nsHTMLAnchorElement::HandleDOMEvent
[c:/builds/vc6/release-tinderbox/WINNT_5.1_Clobber/mozilla/content/html/content/src/nsHTMLAnchorElement.cpp,
line 326]
nsEventStateManager::DispatchMouseEvent
[c:/builds/vc6/release-tinderbox/WINNT_5.1_Clobber/mozilla/content/events/src/nsEventStateManager.cpp,
line 2455]
nsEventStateManager::GenerateMouseEnterExit
[c:/builds/vc6/release-tinderbox/WINNT_5.1_Clobber/mozilla/content/events/src/nsEventStateManager.cpp,
line 2542]
nsEventStateManager::PreHandleEvent
[c:/builds/vc6/release-tinderbox/WINNT_5.1_Clobber/mozilla/content/events/src/nsEventStateManager.cpp,
line 436]
PresShell::HandleEventInternal
[c:/builds/vc6/release-tinderbox/WINNT_5.1_Clobber/mozilla/layout/html/base/src/nsPresShell.cpp,
line 6020]
PresShell::HandleEvent
[c:/builds/vc6/release-tinderbox/WINNT_5.1_Clobber/mozilla/layout/html/base/src/nsPresShell.cpp,
line 5948]
nsViewManager::HandleEvent
[c:/builds/vc6/release-tinderbox/WINNT_5.1_Clobber/mozilla/view/src/nsViewManager.cpp,
line 2281]
nsViewManager::DispatchEvent
[c:/builds/vc6/release-tinderbox/WINNT_5.1_Clobber/mozilla/view/src/nsViewManager.cpp,
line 2025]
HandleEvent
[c:/builds/vc6/release-tinderbox/WINNT_5.1_Clobber/mozilla/view/src/nsView.cpp,
line 79]
nsWindow::DispatchEvent
[c:/builds/vc6/release-tinderbox/WINNT_5.1_Clobber/mozilla/widget/src/windows/nsWindow.cpp,
line 1068]
nsWindow::DispatchWindowEvent
[c:/builds/vc6/release-tinderbox/WINNT_5.1_Clobber/mozilla/widget/src/windows/nsWindow.cpp,
line 1085]
nsWindow::DispatchMouseEvent
[c:/builds/vc6/release-tinderbox/WINNT_5.1_Clobber/mozilla/widget/src/windows/nsWindow.cpp,
line 5225]
ChildWindow::DispatchMouseEvent
[c:/builds/vc6/release-tinderbox/WINNT_5.1_Clobber/mozilla/widget/src/windows/nsWindow.cpp,
line 5478]
nsWindow::ProcessMessage
[c:/builds/vc6/release-tinderbox/WINNT_5.1_Clobber/mozilla/widget/src/windows/nsWindow.cpp,
line 3998]
nsWindow::WindowProc
[c:/builds/vc6/release-tinderbox/WINNT_5.1_Clobber/mozilla/widget/src/windows/nsWindow.cpp,
line 1347]
USER32.dll + 0x2a2d0 (0x77e3a2d0)
USER32.dll + 0x45e5 (0x77e145e5)
USER32.dll + 0xa816 (0x77e1a816)
nsAppShellService::Run
[c:/builds/vc6/release-tinderbox/WINNT_5.1_Clobber/mozilla/xpfe/appshell/src/nsAppShellService.cpp,
line 524]
main1
[c:/builds/vc6/release-tinderbox/WINNT_5.1_Clobber/mozilla/xpfe/bootstrap/nsAppRunner.cpp,
line 1309]
main
[c:/builds/vc6/release-tinderbox/WINNT_5.1_Clobber/mozilla/xpfe/bootstrap/nsAppRunner.cpp,
line 1713]
WinMain
[c:/builds/vc6/release-tinderbox/WINNT_5.1_Clobber/mozilla/xpfe/bootstrap/nsAppRunner.cpp,
line 1735]
WinMainCRTStartup()
KERNEL32.DLL + 0x287e7 (0x7c5987e7)

Comment 17

[Sébastien CRAMATTE]

I've got an xml file processed by an "<?xsl-stylesheet ?>" PI. 
My stylesheet gerenate strict XHTML 1.1 file and call external Javascript file.
If I create an element on the fly I can't set innerHTML property.

---- code ------
this.tip = document.createElementNS('http://www.w3.org/1999/xhtml','div');
		
var bodyElement = document.getElementsByTagName('body')[0];
bodyElement.appendChild(this.tip);		
		
this.tip.innerHTML = this.html;
-------------

----- error ----------

Erreur : [Exception... "Component returned failure code: 0x80004005
(NS_ERROR_FAILURE) [nsIDOMNSHTMLElement.innerHTML]"  nsresult: "0x80004005
(NS_ERROR_FAILURE)"  location: "JS frame ::
http://xforms.zeninteractif.com/release/xml/js/hint.js :: Xhint :: line 43" 
data: no]
Fichier Source : http://xforms.zeninteractif.com/release/xml/js/hint.js
Ligne : 43

Comment 18

[timeless]

[e-mail]: that's an entirely unrelated problem

Comment 19

[Wayne Mery (:wsmwk)]

hangs FF and SM trunk, but testcase doesn't crash for me

Comment 20

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

Attachment: modified testcase

Testing whether document.write works after too-deep-recursion

Comment 21

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

Attachment: proposed patch - preventing too deep recursion in a simple way

I think 10 is enough for the recursion level, though it can be more.
In these test cases recursion warnings from spidermonkey (or crash(?)) starts 
coming when the level is closer to 90, but some other tests might be using more 
stack.

Comment 22

[Brendan Eich [:brendan]]

Could you use the JS_SetThreadStackLimit API and perhaps a new API to get that limit, so you could test (given int dummy local variable) &dummy against it?

/be

Comment 23

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

The problem is that JS recursion error comes when a new <script> element is evaluated. So too-deep-stack error doesn't happen yet in HTMLDocument::Write.
But after js recursion error happens, HTML parsing still continues and new 
<script> elements are created and they are evaluated and new errors happen if too deep stack and so on ...
I *think* the simple check for recursion level is enough here. And that is fast.

Comment 24

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

*** Bug 341671 has been marked as a duplicate of this bug. ***

Comment 25

[Johnny Stenback (:jst)]

Comment on attachment 223680 [details] [diff] [review]
proposed patch - preventing too deep recursion in a simple way

- At the end of WriteCommon():

+  mTooDeepWriteRecursion = (mWriteLevel != 0 && mTooDeepWriteRecursion);

As written, this makes document.write()'s stop working if any nested document.write() nested too deep. Wouldn't it be less likely to break sites if we'd only block document.write()'s that do nest too deep, but permit following document.write()'s, i.e. simply make document.write() a no-op if mWriteLevel is greater than 10, or does that break in some cases that didn't come to my mind yet?

Comment 26

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

Hmm, this seems to work now...
something between 2006-06-04 and 2006-06-05, perhaps Bug 326466

Comment 27

[Thomas Bertels]

This is now reproducible with Firefox 2 and 3 alpha 1.
See also (do not click with JavaScript enabled or freeze will occur !) http://paradiso.cn/user/killfirefox.html.

Comment 28

[Johnny Stenback (:jst)]

Smaug, can you check what's going on here, and get a patch together that reflects the last discussion here for 1.9 if needed?

Comment 29

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

(In reply to comment #25)
> As written, this makes document.write()'s stop working if any nested
> document.write() nested too deep. Wouldn't it be less likely to break sites if
> we'd only block document.write()'s that do nest too deep, but permit following
> document.write()'s, i.e. simply make document.write() a no-op if mWriteLevel is
> greater than 10, or does that break in some cases that didn't come to my mind
> yet?
> 

the problem is that write calls with mWriteLevel < whatever_max_depth may generate new write calls; too-much-recursion is prevented, but there is still endless loop.

Comment 30

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

Attachment: proposed patch

Adding #define NS_MAX_DOCUMENT_WRITE_DEPTH 20 .

Comment 31

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

Btw, the testcase seems to kill Opera 9.x and Konqueror 3.5.x too

Comment 32

[Johnny Stenback (:jst)]

Comment on attachment 254659 [details] [diff] [review]
proposed patch

The endless loop would *eventually* be stopped by the JS branch callback (or should at least), but I could imagine it taking a while as 4k branches would need to be executed, so I could see that taking a while.

r+sr=jst for this change, which just kills a document.write() nesting spree dead in the water.

Comment 33

[Daniel Veditz [:dveditz]]

Can we get this fixed on the 1.8 branch, too?

Comment 34

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

Attachment: for 1.8

for 1.8

Comment 35

[Daniel Veditz [:dveditz]]

Comment on attachment 293907 [details] [diff] [review]
for 1.8

approved for 1.8.1.12, a=dveditz for release-drivers

Comment 36

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

Verified for branch with Mozilla/5.0 (Macintosh; U; Intel Mac OS X; en-US; rv:1.8.1.12pre) Gecko/2008011803 BonEcho/2.0.0.12pre. Verified crash with 2.0.0.11.

Comment 37

[Alexander Sack]

distro patches block 1.8.0.15.

Comment 38

[Alexander Sack]

Comment on attachment 293907 [details] [diff] [review]
for 1.8

distros ship this patch unmodified. caillon, please sign off.

Comment 39

[Christopher Aillon (sabbatical, not receiving bugmail)]

Comment on attachment 293907 [details] [diff] [review]
for 1.8

a=caillon

Comment 40

[Christopher Aillon (sabbatical, not receiving bugmail)]

Fix committed to 1.8.0.15 (a header changed so there was one conflict needed resolving)

Comment 41

[Marcia Knous [:marcia]]

verified fixed using  Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9pre) Gecko/2008050704 Minefield/3.0pre. No crash using testcases in the bug.

Comment 42

[Jesse Ruderman]

Crashtest added as part of http://hg.mozilla.org/mozilla-central/rev/afc662d52ab1