Closed Bug 68498 Opened 24 years ago Closed 24 years ago

site segfaults Mozilla

Categories

(Core :: JavaScript Engine, defect, P1)

Product:
Core
Component:
JavaScript Engine
Type:
defect

Tracking

()

Status:
VERIFIED FIXED
Milestone:
mozilla0.8

People

(Reporter: mknjbh, Assigned: brendan)

References

(
URL
)

Details

(Keywords: crash, js1.5, regression)

Attachments

(6 files)

javascript file
5.76 KB, text/html
Details
stack trace
5.30 KB, text/plain
Details
http://derstandard.at/dyn/aktuell/content.asp
46.41 KB, text/plain
Details
proposed fix
3.13 KB, patch
Details | Diff | Splinter Review
better fix, also fixes ECMA compliance of with(o)eval(s)
6.84 KB, patch
Details | Diff | Splinter Review
commented testcase fodder -- please split out and add these tests to js testsuite
1.75 KB, application/x-javascript
Details 
build used:
  Mozilla/5.0 (X11; U; Linux 2.2.18 i686; en-US; 0.8) Gecko/20010210

browsing to:
  http://derstandard.at

results in:
[...]
JavaScript strict warning: 
http://derstandard.at/dyn/frames/clientapp_mac_ie.js line 10: function eval
must be called directly, and not by way of a function of another name.
JavaScript strict warning: 
http://derstandard.at/dyn/frames/clientapp_mac_ie.js line 10: function eval
must be called directly, and not by way of a function of another name.

/somewhere/mozilla/run-mozilla.sh: line 72:  1110 Segmentation fault
$prog ${1+"$@"}


crash also seen with yesterdays build
used to work ok with jan builds in jan
since derstandard.at is quite dynamic, crash may not always be reproduceable
Attached file javascript file — 

    
    

    
  
I have had two crashes on this site at startup with build 2001020906 on NT4

    
    

    
  
This is definitely a JS engine bug based on the stack trace...  confirming, will
attach stack trace from linux build 2001-02-10-06
Severity: normal → critical
Status: UNCONFIRMED → NEW
Ever confirmed: true
Keywords: crash

    
    

    
  

      
      
      
      

        Attached file
          
        stack trace
        — 
      

    


  

    
    

    
  
*** Bug 68519 has been marked as a duplicate of this bug. ***

    
    

    
  
OS from Linux -=> ALL
adding regression keyword. Mozilla 2001012708 don´t crash at this URL.

Keywords: regression
OS: Linux → All

    
    

    
  
here's a talkback from my linux box  tb26165600k  build 2001021106

    
    

    
  
here's a talkback from my win98 machine, 26208533Q  build 2001021204

    
    

    
  
I've found that Mozilla 2001-01-25 WinNT doesn't crash at this site, either. 
Tried 2001-02-01, and that loads the site partially. (It loads incompletely 
because of the NS_ERROR_DOM_PROP_ACCESS_DENIED errors caused by bug 66577.)


Perhaps the regression causing this bug occurred between 2001-02-01 and now -  


cc'ing Brendan to see if JS is the cause or the victim here. 
Is there any frame in the stack trace 2001-02-11 12:07 that 
should be expanded to shed more light on this?
 

    
    

    
  
This is interesting: if I go to the site http://derstandard.at, 
and save the source locally, Mozilla 2001-02-09 crashes on load.
But Mozilla does NOT crash if I add this to the <HEAD> tag in the HTML: 


                  <BASE HREF="http://derstandard.at">

 


Note: when it loads I get the same partial layout as I noted above for
Mozilla 2001-02-01. I get the same NS_ERROR_DOM_PROP_ACCESS_DENIED error
in the JavaScript console: 


            uncaught exception: "Access to property denied"
            code: 1010
            nsresult: NS_ERROR_DOM_PROP_ACCESS_DENIED
            location: http://derstandard.at/dyn/aktuell/content.asp 
            Line: 1


So I was wrong about this being caused by bug 66577 -

    
    

    
  
Line 1 of http://derstandard.at/dyn/aktuell/content.asp, which causes
the NS_ERROR_DOM_PROP_ACCESS_DENIED error, appears to be: 


   if (self.name=='HAUPTFRAME') parent.newContent("SEITE1","seite1");


NOTE: my Mozilla 2001-02-09 build does NOT crash when loading the .asp page
by itself: 

            http://derstandard.at/dyn/aktuell/content.asp


    
    

    
  
Heinous -- I think the patch for bug 33390 has regressed this.  If I can get a
patch fast, I'll propose it for 0.8.

/be

Assignee: rogerl → brendan
Keywords: js1.5, 
          
            mozilla0.8
Priority: -- → P1
Hardware: PC → All
Target Milestone: --- → mozilla0.8

    
  
Status: NEW → ASSIGNED

    
    

    
  
I'm having trouble with my debug build -- can someone find the string being
passed to eval, and the source code of the function calling eval?  Thanks.

/be

    
    

    
  
DumpJSStack shows:

0 fill(str = "var oldChn='';newContOben=function(chn,tree){OBEN.document.bgColor
=colDef[chn].bc;newAdv(tree);setImgSrc(OBEN,'stdLogo','logoimg',myhost+'/img/nav
igation/headerlogo/'+chn+'.gif');if(oldChn!='') setImgSrc(OBEN,'newsNav','news'+
oldChn,myhost+'/img/dot_clear.gif');if('POLITIK,INVESTOR,WEBSTANDARD,SPORT,PANOR
AMA,ETAT,KULTUR,WISSENSCHAFT'.indexOf(chn)!=-1){show(OBEN,'newsNav');setImgSrc(O
BEN,'newsNav','news'+chn,myhost+'/img/icons/'+chn+'_ns.gif');oldChn=chn;}else{ol
dChn='';if(chn!='SEITE1') hide(OBEN,'newsNav');else show(OBEN,'newsNav');}};") [
"http://derstandard.at/dyn/frames/clientapp_ie.js":10]
    this = [object Window]
1 fill(str = "var oldChn='';newContOben=function(chn,tree){OBEN.document.bgColor
=colDef[chn].bc;newAdv(tree);setImgSrc(OBEN,'stdLogo','logoimg',myhost+'/img/nav
igation/headerlogo/'+chn+'.gif');if(oldChn!='') setImgSrc(OBEN,'newsNav','news'+
oldChn,myhost+'/img/dot_clear.gif');if('POLITIK,INVESTOR,WEBSTANDARD,SPORT,PANOR
AMA,ETAT,KULTUR,WISSENSCHAFT'.indexOf(chn)!=-1){show(OBEN,'newsNav');setImgSrc(O
BEN,'newsNav','news'+chn,myhost+'/img/icons/'+chn+'_ns.gif');oldChn=chn;}else{ol
dChn='';if(chn!='SEITE1') hide(OBEN,'newsNav');else show(OBEN,'newsNav');}};") [
"http://derstandard.at/dyn/frames/clientapp_ie.js":10]
    this = [object Window]
2 <TOP LEVEL> ["http://derstandard.at/dyn/frames/oben.asp?channel=SEITE1":11]
    this = [object Window]

The string passed to obj_eval starts: "var OldChn=";newContOben=function ... It 
is 0x21 chars long and claims to be at line 0xa of clientapp_ie.js.

obj is null at the time of the call to js_CheckRedeclaration. But at crash time 
fp->varobj (in the js_Interpret frame) shows a reasonable looking object.

    
    

    
  
Correction: the patch checked in for bug 61898 was to blame.  I'm working on a
fix now.  Sorry about the regression -- I hope to get this fixed for mozilla0.8.

/be

    
    

    
  
Ok, I have a patch.  It turns out that this self.eval(str) inside a function
case used to work for the wrong reason, and the patch for 61898 broke that bit
of righteous wrongness.  Patch coming up.

/be

    
    

    
  

      
      
      
      

        Attached patch
          
          
        proposed fix
        — 
          — Splinter Review
      

    


  

    
    

    
  
Looking for confirmation that this fixes all badness (it does for me; testsuite
passes too); also looking for r= and sr= fast -- rogerl, jband?

/be

    
    

    
  
sr=jband I'll buy that. Post them test cases.

    
    

    
  
I'm trying these two tests in the JS Shell (no patches applied): 


var y='INITIAL_VALUE_DEFINED_GLOBALLY';
print('\nBefore test() functions get called: "y" in "this" ? ' +  ('y' in this)
      + '\nCurrent value of  y ='  +   y  +   '\n' );


test1(); 
test2();


function test1()
{
  function f(s){eval(s); return y;}
  f("var y = 'NEW_VALUE'");
  print('Inside the test1() function: "y" in "this" ?  :  ' +  ('y' in this)   
        + '\nCurrent value of  y ='  +  y  +   '\n' );
}


function test2()
{
  self=this;
  function f(s){self.eval(s); return y;}
  f("var y =  'NEW_VALUE'");
}



RESULTS:  

test1() does not crash and produces this output: 

  Before any test() function gets called: "y" in "this" ? true
  Current value of  y =INITIAL_VALUE_DEFINED_GLOBALLY

  Inside the test1() function: "y" in "this" ?  :  true
  Current value of  y =INITIAL_VALUE_DEFINED_GLOBALLY



test2() CRASHES:

js_CheckRedeclaration(JSContext * 0x00301e80, JSObject * 0x00000000, long 
3170864, unsigned int 5, int * 0x0012c870) line 1092 + 23 bytes
js_Interpret(JSContext * 0x00301e80, long * 0x0012cd14) line 3087 + 37 bytes
js_Execute(JSContext * 0x00301e80, JSObject * 0x002fad98, JSScript * 0x0030ba80, 
JSStackFrame * 0x004200a8, unsigned int 0, long * 0x0012cd14) line 956 + 13 
bytes
obj_eval(JSContext * 0x00301e80, JSObject * 0x002fa740, unsigned int 1, long * 
0x00420118, long * 0x0012cd14) line 957 + 40 bytes
js_Invoke(JSContext * 0x00301e80, unsigned int 1, unsigned int 0) line 777 + 23 
bytes
js_Interpret(JSContext * 0x00301e80, long * 0x0012d834) line 2670 + 15 bytes
js_Invoke(JSContext * 0x00301e80, unsigned int 0, unsigned int 0) line 794 + 13 
bytes
js_Interpret(JSContext * 0x00301e80, long * 0x0012e300) line 2670 + 15 bytes
js_Execute(JSContext * 0x00301e80, JSObject * 0x002fa740, JSScript * 0x0030a940, 
JSStackFrame * 0x00000000, unsigned int 0, long * 0x0012e300) line 956 + 13 
bytes
JS_ExecuteScript(JSContext * 0x00301e80, JSObject * 0x002fa740, JSScript * 
0x0030a940, long * 0x0012e300) line 3120 + 25 bytes
Load(JSContext * 0x00301e80, JSObject * 0x002fa740, unsigned int 1, long * 
0x00420064, long * 0x0012e3b4) line 638 + 22 bytes
js_Invoke(JSContext * 0x00301e80, unsigned int 1, unsigned int 0) line 777 + 23 
bytes
js_Interpret(JSContext * 0x00301e80, long * 0x0012fed8) line 2670 + 15 bytes
js_Execute(JSContext * 0x00301e80, JSObject * 0x002fa740, JSScript * 0x00309b90, 
JSStackFrame * 0x00000000, unsigned int 0, long * 0x0012fed8) line 956 + 13 
bytes
JS_ExecuteScript(JSContext * 0x00301e80, JSObject * 0x002fa740, JSScript * 
0x00309b90, long * 0x0012fed8) line 3120 + 25 bytes
Process(JSContext * 0x00301e80, JSObject * 0x002fa740, char * 0x00000000) line 
372 + 22 bytes
ProcessArgs(JSContext * 0x00301e80, JSObject * 0x002fa740, char * * 0x00300164, 
int 0) line 530 + 17 bytes
main(int 0, char * * 0x00300164) line 2061 + 21 bytes
JS! mainCRTStartup + 227 bytes
KERNEL32! 77f1ba06()


    
    

    
  
RESULTS WITH PATCH 25200 APPLIED (and a print-status added to test2()):

 
The tests do not crash, and produce this output: 

  Before test() functions get called: "y" in "this" ? true
  Current value of  y =INITIAL_VALUE_DEFINED_GLOBALLY

  Inside the test1() function: "y" in "this" ?  :  true
  Current value of  y =INITIAL_VALUE_DEFINED_GLOBALLY

  Inside the test2() function: "y" in "this" ?  :  true
  Current value of  y =NEW_VALUE



    
    

    
  
Phil: you need to set self = this; outside of any function body, so as to
capture a reference to the global object.  I hope my test-fodder attachment
helps -- please ask me anything about it.

The "better fix" extends the first "proposed fix" that jband sr'd as follows:

* JS_BUG_WITH_CLOSURE is defined to 0 in jsconfig.h.  This macro enabled a
non-ECMA behavior from JS1.2 days that was always viewed as a bug.  I would like
to fix this bug now.  If enough content depends on it that cannot be corrected
to comply with ECMA easily, I'll back off, but for mozilla0.8, and I hope for
ever, this should not stand.

* The native method implementation for eval, obj_eval in jsobj.c, was setting
the JSFRAME_EVAL flag in fp->special only if the call to eval was direct
('eval(...)' rather than 'obj.eval(...)').  This flag affects whether variables
are permanent (ECMA DontDelete) or not when instantiated -- variables created by
eval'd variable statements are not permanent.  My reading of ECMA-262 Edition 3
doesn't support a difference between direct and indirect eval, as far as
variable instantiation is concerned, so I'm simplifying things to one case here.

* jsparse.c contains entry points for parsing and compiling a token stream:
js_ParseTokenStream and js_CompileTokenStream.  These are (or rather, the latter
currently is -- the former is not used yet) used from JS API entry points as
well as from obj_eval.  Therefore the jsparse.c functions consider using cx->fp
in order to discover the right ECMA-specified scope chain (fp->scopeChain) and
variable object (fp->varobj).  If the current top-most frame doesn't seem
suitable, these functions push a mostly-zeroed auto frame for the duration of
parsing or compiling, and the rest of the compiler looks in cx->fp to find the
scope chain and the variable object.

Prior to the advent of lightweight (vs. JSFUN_HEAVYWEIGHT -- see bug 23346)
functions, SpiderMonkey did not store the variable object in each frame; rather,
it computed the variable object by examining the scope chain.  But when I added
the varobj member to JSStackFrame, so as to hoist variable object computation
from scope chain out of run-time and into compile-time, I goofed these two
js_{Parse,Compile}TokenStream functions and made their stack frame auto-push
logic depend on fp->varobj != chain as well as on fp->scopeChain != chain.

This goof affects only 'with(o)eval(s)', because only that case calls
js_CompileTokenStream with a cx->fp->varobj not equal to the scope chain head,
the With object wrapping o.  It results in functions declared in s being wrongly
parented by the With object for o, rather than by the variable object of eval's
caller (per ECMA).  So the latest patch eliminates the fp->varobj != chain
clause from the disjunctions.

(Note that JS_BUG_WITH_CLOSURE set to 1 has the effect of causing a function
declared in a with statement body, whether eval'd [with(o)eval(s)] or not
[with(o)function f(){}], to bind the function's name in the object specified in
the head of the with statement [o], rather than in the variables object.)

Finally, I pondered why jsparse.c's FunctionBody (which parses only, and does
not emit bytecode) and jsemit.c's js_EmitFunctionBody (which generates code for
the function into the function's script) both contained identical auto-push
logic for ensuring that cx->fp points to a frame that has valid fun, scopeChain,
and varobj members for the function.  Both of these functions are called by
js_CompileFunctionBody, which is called from the JS API entry points
JS_Compile*Function, and from the Function constructor (jsfun.c).  Rather than
have each subroutine auto-push an identical, mostly zeros frame with the right
fun, scopeChain,and varobj members, I added the same auto-push magic to their
caller, js_CompileFunctionBody.

Whew.  Hope this all makes sense.  I'm looking for r= and sr= again.

/be

    
    

    
  
Oops, two more diffs to jsparse.c that I didn't discuss in the last commment:

* FunctionDef used to select the JSOP_CLOSURE bytecode not only for function
expression statements (if (cond) function f() {...} conditionally declares
function f -- this is an allowed ECMA extension that we support), which are
functions not at the top level (tc->topStmt non-null); but also for the case
where cx->fp->scopeChain != the parent of the function object.  As the comment I
removed says, this was to support JS_BUG_WITH_CLOSURE in the case where the
function definition was eval'd: with (o) eval('function f(){}').

But ECMA-262 Edition 3 is unambiguous: eval(s) treats s as a Program.  And a
function definition at the (apparent) top level of a Program is a
FunctionDeclaration, which should be treated differently from our allowed
function expression statement (or conditional function) extension.  The bytecode
selected for such top-level functions is JSOP_NOP, not JSOP_CLOSURE, so I axed
the (cx->fp->scopeChain != parent) clause from the JSOP_CLOSURE selection test.

* Variables contained a loop

    while (fp->special && fp->down)    /* skip eval and debugger frames */
        fp = fp->down;

that I added early in development of an earlier fix (I forget which bug ;-), but
this loop is unnecessary and confusing: eval frames in particular inherit their
caller's varobj and scopeChain, so there is no need to skip them; the same idea
should govern debugger frames.

/be

    
    

    
  
r=bryner on the "proposed fix" patch (the one which jband sr='d).


    
    

    
  
r=jband. This one scares me. But I'm not seeing anything wrong.

    
    

    
  
jband: are you scared because I'm fixing JS_BUG_WITH_CLOSURE?  I am scared too,
but I'm more scared of not fixing it.  ECMA uber alles, and anyone who counts on
this will have to be a squeaky wheel in the Mozilla community.

Fix checked in.

/be

Status: ASSIGNED → RESOLVED
Closed: 24 years ago
Resolution: --- → FIXED

    
    

    
  
Four testcases split out of Brendan's attachment and added to JS test suite:

             js/tests/js1_5/Regress/regress-68498-001.js 
             js/tests/js1_5/Regress/regress-68498-002.js
             js/tests/js1_5/Regress/regress-68498-003.js
             js/tests/js1_5/Regress/regress-68498-004.js


The only one that was causing a crash was regress-68498-003.js :

* "Backward compatibility: support calling obj.eval(str), which 
*  evaluates str using obj as the scope chain and variable object."


I hope that conforms to expectation. At any rate, after the fix
none of the four tests should crash or produce any errors -

    
    

    
  
Verified using Mozilla builds 2001-02-19-xx on WinNT, Linux, and Mac.
No longer crash on loading http://derstandard.at

In addition, the standalone JS shell is passing all four regression tests
above on WinNT and Linux - 
Status: RESOLVED → VERIFIED

    
  
Keywords: mozilla0.8

          You need to log in
          before you can comment on or make changes to this bug.
        






  

    
  







  

    

      
Attachment

      

      
      
      
      
    

    

      

        

          

            
General

            

              Creator: 



            

            
Created: 

            
Updated: 

            
Size: