Closed
Bug 197052
Opened 21 years ago
Closed 18 years ago
crash if modification innerHTML of element in this element [@ js_EmitTree ]
Categories
(Core :: DOM: Core & HTML, defect, P2)
Core
DOM: Core & HTML
Tracking
()
VERIFIED
FIXED
mozilla1.4alpha
People
(Reporter: rar, Assigned: smaug)
References
Details
(5 keywords)
Crash Data
Attachments
(5 files, 1 obsolete file)
375 bytes,
text/html
|
Details | |
39.44 KB,
text/plain
|
Details | |
370 bytes,
text/html
|
Details | |
2.64 KB,
patch
|
jst
:
review+
jst
:
superreview+
|
Details | Diff | Splinter Review |
2.78 KB,
patch
|
dveditz
:
approval1.8.1.12+
caillon
:
approval1.8.0.next+
|
Details | Diff | Splinter Review |
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(">"+document.getElementById('uniqid').innerHTML+"<"); </script> </div> Sample 2. <div id="uniqid"> <script language="JavaScript" type="text/JavaScript"> alert(">"+document.getElementById('uniqid').innerHTML+"<"); </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•21 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•21 years ago
|
||
test case for code snippet 1
Comment 3•21 years ago
|
||
Crash was in mozilla-bin on OS X... Talkback not envoked... crash at nsJSContext::EvaluateString (?)
Comment 4•21 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
Comment 5•21 years ago
|
||
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•21 years ago
|
Whiteboard: TB18021740Y
Comment 6•21 years ago
|
||
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
Comment 7•21 years ago
|
||
It's similar behaviour as with loading document into iframe of this document. So probably limiting the nesting is the only fix.
Comment 9•21 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
Comment 10•21 years ago
|
||
Brendan, does this look familiar?
Comment 11•21 years ago
|
||
Phil, can you try to reduce a testcase into pure-JS, no DOM required? Thanks, /be
QA Contact: ian → pschwartau
Comment 12•21 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•21 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•20 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
Comment 15•20 years ago
|
||
Need some talkback lovin' here. /be
Comment 16•20 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)
Comment 17•19 years ago
|
||
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•19 years ago
|
||
contact@zeninteractif.com: that's an entirely unrelated problem
Comment 19•18 years ago
|
||
hangs FF and SM trunk, but testcase doesn't crash for me
Keywords: hang
QA Contact: pschwartau → ian
Assignee | ||
Comment 20•18 years ago
|
||
Testing whether document.write works after too-deep-recursion
Assignee: general → Olli.Pettay
Status: NEW → ASSIGNED
Assignee | ||
Comment 21•18 years ago
|
||
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)
Comment 22•18 years ago
|
||
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•18 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•18 years ago
|
||
*** Bug 341671 has been marked as a duplicate of this bug. ***
Comment 25•18 years ago
|
||
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•18 years ago
|
||
Hmm, this seems to work now... something between 2006-06-04 and 2006-06-05, perhaps Bug 326466
Status: ASSIGNED → RESOLVED
Closed: 18 years ago
Resolution: --- → WORKSFORME
Assignee | ||
Updated•18 years ago
|
Attachment #223680 -
Flags: superreview?(jst)
Attachment #223680 -
Flags: review?(jst)
Comment 27•18 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 → ---
Comment 28•18 years ago
|
||
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•18 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•18 years ago
|
||
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•18 years ago
|
||
Btw, the testcase seems to kill Opera 9.x and Konqueror 3.5.x too
Comment 32•18 years ago
|
||
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•18 years ago
|
Status: REOPENED → RESOLVED
Closed: 18 years ago → 18 years ago
Resolution: --- → FIXED
Comment 33•17 years ago
|
||
Can we get this fixed on the 1.8 branch, too?
Flags: wanted1.8.1.x+
Flags: blocking1.8.1.12?
Comment 35•17 years ago
|
||
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+
Updated•17 years ago
|
Flags: blocking1.8.1.12? → blocking1.8.1.12+
Assignee | ||
Updated•17 years ago
|
Keywords: fixed1.8.1.12
Comment 36•17 years ago
|
||
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 38•16 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 39•16 years ago
|
||
Comment on attachment 293907 [details] [diff] [review] for 1.8 a=caillon
Attachment #293907 -
Flags: approval1.8.0.15? → approval1.8.0.15+
Comment 40•16 years ago
|
||
Fix committed to 1.8.0.15 (a header changed so there was one conflict needed resolving)
Keywords: fixed1.8.0.15
Comment 41•16 years ago
|
||
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
Comment 42•15 years ago
|
||
Crashtest added as part of http://hg.mozilla.org/mozilla-central/rev/afc662d52ab1
Flags: in-testsuite+
Updated•13 years ago
|
Crash Signature: [@ js_EmitTree ]
You need to log in
before you can comment on or make changes to this bug.
Description
•