Closed Bug 116834 Opened 23 years ago Closed 23 years ago

Mozilla always crashes on this page

Categories

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

Product:
Core
Component:
DOM: Core & HTML
Type:
defect

Tracking

()

Status:
VERIFIED FIXED
Milestone:
mozilla0.9.9

People

(Reporter: mantas, Assigned: jst)

References

(
URL
)

Details

(Keywords: crash, Whiteboard: [HAVE FIX])

Attachments

(5 files)

stack trace
(deleted), text/plain
Details
Don't execute script tags when element.innerHTML is set (for IE compat).
(deleted), patch
Details | Diff | Splinter Review
Don't let timeouts execute when re-enabling scripts after .innerHTML is set.
(deleted), patch
bzbarsky
: review+
Details | Diff | Splinter Review
Same as above, but up to date with the tip...
(deleted), patch
jst
: review+
jst
: superreview+
Details | Diff | Splinter Review
Fix the not-so-clear-and-fragile shortcut...
(deleted), patch
rginda
: review+
bugzilla
: superreview+
Details | Diff | Splinter Review
From Bugzilla Helper: User-Agent: Mozilla/5.0 (Windows; U; Win98; en-US; rv:0.9.7) Gecko/20011221 BuildID: 2001122106 I can't open web page http://www.kobra.ktu.lt/~vytis/, because I always got a crash :( Reproducible: Always Steps to Reproduce: 1. Type http://www.kobra.ktu.lt/~vytis/ in url bar 2. Press 'enter' :) 3. Wait a minute Actual Results: Mozilla crashes
Crashes me too, win98SE 2001122208, confirming Talkback ID TB879480H
Status: UNCONFIRMED → NEW
Ever confirmed: true
Keywords: crash
Crashes me too, win98 latest trunk.
Crashes me too, win2k 2001122106 (mozilla 0.9.7) Talkback: TB883829M
Crash for me too! Win XP Pro!
confirming with win2k build 20011224.. -> Js Engine
i should sleep I SAID : -> JS ENGINE
Assignee: asa → rogerl
Component: Browser-General → Javascript Engine
QA Contact: doronr → pschwartau
Attached file stack trace (deleted) —
Distilled test case for this bug. The following HTML/JS crashes Mozilla: <html> <body> AAAAA <script> document.body.innerHTML += "<b> Text1 </b> Text2 "; </script> </body> </html> A similar one does not: <html> <body> <p id="aaaaa"> AAAAA </p> <script> a = document.getElementById("aaaaa") a.innerHTML += "<b> Text1 </b> Text2 "; </script> </body> </html> Both work in IE 5.50, and produce the same output.
The problem is infinite recursion: all the stack traces show stack overflow. When I load the reduced testcase, this appears over and over in the stack: ... ^ ^ ^ nsScriptLoader::EvaluateScript() line 576 nsScriptLoader::ProcessRequest() line 483 + 22 bytes nsScriptLoader::ProcessScriptElement() line 426 + 15 bytes nsHTMLScriptElement::SetDocument() line 159 nsGenericHTMLContainerElement::InsertChildAt() line 3743 nsGenericElement::doInsertBefore() line 2422 + 58 bytes nsGenericHTMLContainerElement::AppendChild() line 553 nsHTMLBodyElement::AppendChild() line 222 + 23 bytes nsGenericHTMLElement::SetInnerHTML() ^ ^ ^ ... Over to DOM Level 0. This is very similar to bug 94308, "document.write document.documentElement properties causes infinite loop"
Assignee: rogerl → jst
Component: Javascript Engine → DOM Level 0
QA Contact: pschwartau → desale
Note: I was referring to R.B.'s reduced testcase in Comment #9 above <html> <body> AAAAA <script> document.body.innerHTML += "<b> Text1 </b> Text2 "; </script> </body> </html>
Severity: critical → major
Status: NEW → ASSIGNED
Priority: -- → P2
Hardware: PC → All
Whiteboard: [HAVE FIX]
Target Milestone: --- → mozilla0.9.8
bz, r=?
Comment on attachment 64098 [details] [diff] [review] Don't let timeouts execute when re-enabling scripts after .innerHTML is set. The second set of changes to nsGenericHTMLElement.cpp seems to be unrelated to this bug... other than that, r=bzbarsky
Attachment #64098 - Flags: review+
Oh yeah, true, but they're just cleanup, are you ok with that cleanup too?
> - if (tag && tag.get() == nsHTMLAtoms::area) { > + if (tag == nsHTMLAtoms::area) { Wouldn't | if (nsHTMLAtoms::area == tag) { | be better? The rest looks fine.
Um, no, that would IMO not be better at all. The question that's asked there is not if nsHTMLAtoms::area is equals to tag, the question is rather whether or not tag happens to be equal to nsHTMLAtoms::area. Big difference :-)
OK. In that case r=bzbarsky for the whole thing. :)
Target Milestone: mozilla0.9.8 → mozilla0.9.9
Keywords: nsbeta1
Comment on attachment 67857 [details] [diff] [review] Same as above, but up to date with the tip... rpotts says sr=rpotts.
Attachment #67857 - Flags: superreview+
Attachment #67857 - Flags: review+
Fixed.
Status: ASSIGNED → RESOLVED
Closed: 23 years ago
Resolution: --- → FIXED
*** Bug 112411 has been marked as a duplicate of this bug. ***
NS_IMETHODIMP -nsJSContext::SetScriptsEnabled(PRBool aEnabled) +nsJSContext::SetScriptsEnabled(PRBool aEnabled, PRBool aFireTimeouts) { mScriptsEnabled = aEnabled; - nsCOMPtr<nsIScriptGlobalObject> global; - GetGlobalObject(getter_AddRefs(global)); + if (aFireTimeouts) { + nsCOMPtr<nsIScriptGlobalObject> global; + GetGlobalObject(getter_AddRefs(global)); - if (global) { - global->SetScriptsEnabled(aEnabled); + if (global) { + global->SetScriptsEnabled(aEnabled); + } } aFireTimeouts controls whether or not we tell the global object about the state change, and the global object just happens to only need the state change for the purpose of firing timeouts. This sounds pretty fragile to me. What if the global object wants to know about the script-enabled state change for some other reason? Shouldn't aFireTimeouts be passed to global->SetScriptsEnabled()?
Yes, agreed. That was kindof a nasty shortcut I decided to take when I wrote this. Patch coming up.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Comment on attachment 68236 [details] [diff] [review] Fix the not-so-clear-and-fragile shortcut... r=rginda
Attachment #68236 - Flags: review+
Crash --> Severity: critical
Severity: major → critical
Um, no, the crash was fixed, we're just doing some cleanup now, so not a critical bug any more...
Severity: critical → normal
Comment on attachment 68236 [details] [diff] [review] Fix the not-so-clear-and-fragile shortcut... sr=blake
Attachment #68236 - Flags: superreview+
Fixed.
Status: REOPENED → RESOLVED
Closed: 23 years ago → 23 years ago
Resolution: --- → FIXED
jst, would this have caused a startup time increase?
Nope, and this was checked in days ago (and the last cleanup patch that was checked in didn't really change anything), this only changes how we're dealing with element.innerHTML, which isn't used at all during startup.
Tested with build 0.9.9 on Win32. Now Mozilla does not crash on this page.
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: