Closed Bug 116834 Opened 18 years ago Closed 18 years ago

Mozilla always crashes on this page

Categories

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

defect

Tracking

()

VERIFIED FIXED
mozilla0.9.9

People

(Reporter: mantas, Assigned: jst)

References

()

Details

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

Attachments

(5 files)

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
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: 18 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: 18 years ago18 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.