Closed Bug 68498 Opened 25 years ago Closed 25 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).
sr=shaver on the most recent fix (http://bugzilla.mozilla.org/showattachment.cgi?attach_id=25250), in all its documented beauty.
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: 25 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: