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

VERIFIED FIXED in mozilla1.4alpha

Status

()

Core
DOM: Core & HTML
P2
critical
VERIFIED FIXED
15 years ago
6 years ago

People

(Reporter: Roman Rahman, Assigned: smaug)

Tracking

(5 keywords)

Trunk
mozilla1.4alpha
crash, fixed1.8.0.15, hang, testcase, verified1.8.1.12
Points:
---
Bug Flags:
blocking1.9 +
blocking1.8.1.12 +
wanted1.8.1.x +
blocking1.8.0.next +
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

(crash signature)

Attachments

(5 attachments, 1 obsolete attachment)

(Reporter)

Description

15 years ago
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

15 years ago
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"
Keywords: crash, stackwanted

Comment 2

15 years ago
Created attachment 117037 [details]
testcase

test case for code snippet 1

Comment 3

15 years ago
Created attachment 117038 [details]
mozilla-bin crashlog - OS X

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

Comment 4

15 years ago
-> All/All (OS X 1.3RC Build)

didn't go looking for dupes, so not confirming yet
OS: Windows 2000 → All
Hardware: PC → All
Confirming with Mozilla 1.3RC 20030311 Windows

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

TB18021740Y
Status: UNCONFIRMED → NEW
Ever confirmed: true

Updated

15 years ago
Whiteboard: TB18021740Y
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.
Status: NEW → ASSIGNED
Component: DOM Mozilla Extensions → DOM HTML
Priority: -- → P2
Target Milestone: --- → mozilla1.4alpha
It's similar behaviour as with loading document into iframe of this document. So
probably limiting the nesting is the only fix.
Mass-reassigning bugs.
Assignee: jst → dom_bugs
Status: ASSIGNED → NEW

Comment 9

15 years ago
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
[...]
Keywords: stackwanted
Summary: crash if modification innerHTML of element in this element → crash if modification innerHTML of element in this element [@ js_EmitTree ]
Whiteboard: TB18021740Y
Brendan, does this look familiar?
Phil, can you try to reduce a testcase into pure-JS, no DOM required?  Thanks,

/be
QA Contact: ian → pschwartau

Comment 12

15 years ago
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

15 years ago
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

13 years ago
(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
Need some talkback lovin' here.

/be

Comment 16

13 years ago
Incident ID: 8920
Stack Signature	js_GetGCThingFlags 36d4dbfd
Email Address	anders_pedersen@webmail.no
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)

Updated

13 years ago
Keywords: testcase
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

12 years ago
contact@zeninteractif.com: that's an entirely unrelated problem
hangs FF and SM trunk, but testcase doesn't crash for me
Keywords: hang
QA Contact: pschwartau → ian
(Assignee)

Comment 20

11 years ago
Created attachment 223678 [details]
modified testcase

Testing whether document.write works after too-deep-recursion
Assignee: general → Olli.Pettay
Status: NEW → ASSIGNED
(Assignee)

Comment 21

11 years ago
Created attachment 223680 [details] [diff] [review]
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.
Attachment #223680 - Flags: superreview?(jst)
Attachment #223680 - Flags: review?(jst)
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
(Assignee)

Comment 23

11 years ago
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.

(Assignee)

Comment 24

11 years ago
*** Bug 341671 has been marked as a duplicate of this bug. ***
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?
(Assignee)

Comment 26

11 years ago
Hmm, this seems to work now...
something between 2006-06-04 and 2006-06-05, perhaps Bug 326466
Status: ASSIGNED → RESOLVED
Last Resolved: 11 years ago
Resolution: --- → WORKSFORME
(Assignee)

Updated

11 years ago
Attachment #223680 - Flags: superreview?(jst)
Attachment #223680 - Flags: review?(jst)

Comment 27

11 years ago
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.
Status: RESOLVED → REOPENED
Flags: blocking1.9?
Resolution: WORKSFORME → ---
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?
Flags: blocking1.9? → blocking1.9+
(Assignee)

Comment 29

11 years ago
(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.
(Assignee)

Comment 30

11 years ago
Created attachment 254659 [details] [diff] [review]
proposed patch

Adding #define NS_MAX_DOCUMENT_WRITE_DEPTH 20 .
Attachment #223680 - Attachment is obsolete: true
Attachment #254659 - Flags: superreview?(jst)
Attachment #254659 - Flags: review?(jst)
(Assignee)

Comment 31

11 years ago
Btw, the testcase seems to kill Opera 9.x and Konqueror 3.5.x too
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.
Attachment #254659 - Flags: superreview?(jst)
Attachment #254659 - Flags: superreview+
Attachment #254659 - Flags: review?(jst)
Attachment #254659 - Flags: review+
(Assignee)

Updated

11 years ago
Status: REOPENED → RESOLVED
Last Resolved: 11 years ago11 years ago
Resolution: --- → FIXED
Blocks: 400792
Can we get this fixed on the 1.8 branch, too?
Flags: wanted1.8.1.x+
Flags: blocking1.8.1.12?
(Assignee)

Comment 34

10 years ago
Created attachment 293907 [details] [diff] [review]
for 1.8

for 1.8
Attachment #293907 - Flags: approval1.8.1.12?
Comment on attachment 293907 [details] [diff] [review]
for 1.8

approved for 1.8.1.12, a=dveditz for release-drivers
Attachment #293907 - Flags: approval1.8.1.12? → approval1.8.1.12+
Flags: blocking1.8.1.12? → blocking1.8.1.12+
(Assignee)

Updated

10 years ago
Keywords: fixed1.8.1.12
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.
Keywords: fixed1.8.1.12 → verified1.8.1.12

Comment 37

9 years ago
distro patches block 1.8.0.15.
Flags: blocking1.8.0.15+

Comment 38

9 years ago
Comment on attachment 293907 [details] [diff] [review]
for 1.8

distros ship this patch unmodified. caillon, please sign off.
Attachment #293907 - Flags: approval1.8.0.15?
Comment on attachment 293907 [details] [diff] [review]
for 1.8

a=caillon
Attachment #293907 - Flags: approval1.8.0.15? → approval1.8.0.15+
Fix committed to 1.8.0.15 (a header changed so there was one conflict needed resolving)
Keywords: fixed1.8.0.15
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.
Status: RESOLVED → VERIFIED

Updated

9 years ago
Component: DOM: HTML → DOM: Core & HTML
QA Contact: ian → general

Comment 42

9 years ago
Crashtest added as part of http://hg.mozilla.org/mozilla-central/rev/afc662d52ab1
Flags: in-testsuite+
Crash Signature: [@ js_EmitTree ]
You need to log in before you can comment on or make changes to this bug.