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
132.18 KB, text/plain
Details
Don't execute script tags when element.innerHTML is set (for IE compat).
1.83 KB, patch
Details | Diff | Splinter Review
Don't let timeouts execute when re-enabling scripts after .innerHTML is set.
8.75 KB, patch
bzbarsky
: review+
Details | Diff | Splinter Review
Same as above, but up to date with the tip...
5.97 KB, patch
jst
: review+
jst
: superreview+
Details | Diff | Splinter Review
Fix the not-so-clear-and-fragile shortcut...
4.90 KB, 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 — 

    
    

    
  
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: