Closed
Bug 355820
Opened 18 years ago
Closed 17 years ago
Remove non-standard Script object
Categories
(Core :: JavaScript Engine, defect)
Core
JavaScript Engine
Tracking
()
VERIFIED
FIXED
People
(Reporter: Gavin, Assigned: crowderbt)
Details
(Keywords: dev-doc-complete, relnote)
Attachments
(5 files, 8 obsolete files)
5.43 KB,
patch
|
brendan
:
review+
|
Details | Diff | Splinter Review |
1.04 KB,
patch
|
brendan
:
review+
|
Details | Diff | Splinter Review |
2.08 KB,
patch
|
brendan
:
review+
|
Details | Diff | Splinter Review |
2.35 KB,
text/plain
|
Details | |
2.84 KB,
text/plain
|
Details |
The obsolete Script object has been hanging around since Netscape days. it's not in the ECMAScript standard and should be removed on the road to JavaScript "2".
Reporter | ||
Updated•18 years ago
|
Flags: blocking1.9?
Comment 1•18 years ago
|
||
Brian, could you please field this one? Thanks, /be
Assignee: brendan → crowder
Assignee | ||
Updated•18 years ago
|
Status: NEW → ASSIGNED
Assignee | ||
Comment 2•18 years ago
|
||
Probably not worth of a formal review yet, but needs eyes on it. I basically naively sliced out anything wrapped by JS_HAS_SCRIPT_OBJECT and slashed and burned some things nearby. I have probably missed some XDR pieces that need more knowledgeable attention. What's not to love about this diffstat, though? js.c | 16 - jsapi.c | 30 -- jsconfig.h | 4 jsdbgapi.c | 3 jsfun.c | 3 jsscript.c | 879 ------------------------------------------------------------- jsscript.h | 17 - jsxdrapi.c | 10 8 files changed, 7 insertions(+), 955 deletions(-)
Comment 3•18 years ago
|
||
Comment on attachment 251327 [details] [diff] [review] no more script object We cannot remove the JS_NewScriptObject API, it is needed to GC-root a ref to a JSScript (i.e., you must wrap a JSScript with an object and reference the object from a rooted pointer or jsval, or otherwise GC-protect the object, in order to protect the script and its atoms). Would rather we just #define JS_HAS_SCRIPT_OBJECT 0 and leave the code around for other embeddings that might need it still, and that have no security issues at all, or at least with Script objects. /be
Attachment #251327 -
Flags: review-
Assignee | ||
Comment 4•18 years ago
|
||
This defines JS_HAS_SCRIPT_OBJECT = 0 in jsconfig.h and fixes a couple of link-errors generated by the change.
Attachment #251327 -
Attachment is obsolete: true
Attachment #252628 -
Flags: review?(brendan)
Assignee | ||
Comment 5•18 years ago
|
||
Comment on attachment 252628 [details] [diff] [review] much lighter touch Disregard, this is broken.
Attachment #252628 -
Flags: review?(brendan)
Assignee | ||
Comment 6•18 years ago
|
||
Last version was broken because of a discrepancy in size in the js_proto_strs table. I changed the macro that generates the enum and JSProto_LIMIT to NOT use hard-coded values, and instead let the enum count the included values by itself. This makes the middle column of the jsproto.tbl obsolete, so I removed it, and cleaned up the macros depending on it. The code that acquires slotnames simply uses the js_proto_strs table instead of doing if (code == key) { } in a long chain of if()s. I also updated the XDR bytecode version number, since I think the XDR touches will make us incompatible with old XDR data.
Attachment #252628 -
Attachment is obsolete: true
Assignee | ||
Comment 7•18 years ago
|
||
Comment on attachment 252642 [details] [diff] [review] not broken version I still think my XDR code is broken here, please audit carefully.
Attachment #252642 -
Flags: review?(brendan)
Comment 8•18 years ago
|
||
Can't you leave the JS_PROTO(Script, ...) alone? It's OK for it to have an entry in the list of protos, as long as we never JS_InitClass the class.
Assignee | ||
Comment 9•18 years ago
|
||
I could just remove the extra declaration of js_Script_str in jsscript.c and leave the proto.tbl (I removed it here to fix a link error), but removing the weird dependency on hard-coded numbers from that table doesn't seem like a bad thing. The patch is smaller your way, certainly. What else does that table feed?
Assignee | ||
Comment 10•18 years ago
|
||
Oh, and the slotname code must surely be faster this way?
Comment 11•18 years ago
|
||
We want fixed proto keys for XDR backward compatibility -- revert that part of the patch, please. /be
Assignee | ||
Comment 12•18 years ago
|
||
Please still look at the jsfun.c changes to make sure I am not breaking XDR
Attachment #252642 -
Attachment is obsolete: true
Attachment #252642 -
Flags: review?(brendan)
Assignee | ||
Updated•18 years ago
|
Attachment #252734 -
Flags: review?(brendan)
Comment 13•18 years ago
|
||
Comment on attachment 252734 [details] [diff] [review] toning it down a bit >Index: jsconfig.h >=================================================================== >RCS file: /cvsroot/mozilla/js/src/jsconfig.h,v >retrieving revision 3.42 >diff -u -p -8 -r3.42 jsconfig.h >--- jsconfig.h 6 Jul 2006 01:25:44 -0000 3.42 >+++ jsconfig.h 25 Jan 2007 05:12:59 -0000 >@@ -116,17 +116,17 @@ > > #define JS_HAS_STR_HTML_HELPERS 1 /* has str.anchor, str.bold, etc. */ > #define JS_HAS_PERL_SUBSTR 1 /* has str.substr */ > #define JS_HAS_OBJ_PROTO_PROP 1 /* has o.__proto__ etc. */ > #define JS_HAS_OBJ_WATCHPOINT 1 /* has o.watch and o.unwatch */ > #define JS_HAS_EXPORT_IMPORT 1 /* has export fun; import obj.fun */ > #define JS_HAS_EVAL_THIS_SCOPE 1 /* Math.eval is same as with (Math) */ > #define JS_HAS_SHARP_VARS 1 /* has #n=, #n# for object literals */ >-#define JS_HAS_SCRIPT_OBJECT 1 /* has (new Script("x++")).exec() */ >+#define JS_HAS_SCRIPT_OBJECT 0 /* has (new Script("x++")).exec() */ > #define JS_HAS_XDR 1 /* has XDR API and internal support */ > #define JS_HAS_XDR_FREEZE_THAW 0 /* has XDR freeze/thaw script methods */ > #define JS_HAS_TOSOURCE 1 /* has Object/Array toSource method */ > #define JS_HAS_DEBUGGER_KEYWORD 1 /* has hook for debugger keyword */ > #define JS_HAS_CATCH_GUARD 1 /* has exception handling catch guard */ > #define JS_HAS_SPARSE_ARRAYS 0 /* array methods preserve empty elems */ > #define JS_HAS_GETTER_SETTER 1 /* has JS2 getter/setter functions */ > #define JS_HAS_UNEVAL 1 /* has uneval() top-level function */ >@@ -144,17 +144,17 @@ > > #define JS_HAS_STR_HTML_HELPERS 1 /* has str.anchor, str.bold, etc. */ > #define JS_HAS_PERL_SUBSTR 1 /* has str.substr */ > #define JS_HAS_OBJ_PROTO_PROP 1 /* has o.__proto__ etc. */ > #define JS_HAS_OBJ_WATCHPOINT 1 /* has o.watch and o.unwatch */ > #define JS_HAS_EXPORT_IMPORT 1 /* has export fun; import obj.fun */ > #define JS_HAS_EVAL_THIS_SCOPE 1 /* Math.eval is same as with (Math) */ > #define JS_HAS_SHARP_VARS 1 /* has #n=, #n# for object literals */ >-#define JS_HAS_SCRIPT_OBJECT 1 /* has (new Script("x++")).exec() */ >+#define JS_HAS_SCRIPT_OBJECT 0 /* has (new Script("x++")).exec() */ > #define JS_HAS_XDR 1 /* has XDR API and internal support */ > #define JS_HAS_XDR_FREEZE_THAW 0 /* has XDR freeze/thaw script methods */ > #define JS_HAS_TOSOURCE 1 /* has Object/Array toSource method */ > #define JS_HAS_DEBUGGER_KEYWORD 1 /* has hook for debugger keyword */ > #define JS_HAS_CATCH_GUARD 1 /* has exception handling catch guard */ > #define JS_HAS_SPARSE_ARRAYS 0 /* array methods preserve empty elems */ > #define JS_HAS_GETTER_SETTER 1 /* has JS2 getter/setter functions */ > #define JS_HAS_UNEVAL 1 /* has uneval() top-level function */ >@@ -172,17 +172,17 @@ I would leave the older versions alone, and just change JS1.7 (incompatibly). >@@ -1377,18 +1377,20 @@ fun_xdrObject(JSXDRState *xdr, JSObject > dupflag | SPROP_HAS_SHORTID, > JSVAL_TO_INT(userid))) { > goto bad; > } > } > } > } > >+#if JS_HAS_SCRIPT_OBJECT > if (!js_XDRScript(xdr, &fun->u.i.script, NULL)) > goto bad; >+#endif This is wrong, a function's script can and should be XDR'ed (e.g. as part of XUL fastload). >+#if JS_HAS_SCRIPT_OBJECT > JS_PUBLIC_API(JSBool) > JS_XDRScript(JSXDRState *xdr, JSScript **scriptp) > { > if (!js_XDRScript(xdr, scriptp, NULL)) > return JS_FALSE; > if (xdr->mode == JSXDR_DECODE) > js_CallNewScriptHook(xdr->cx, *scriptp, NULL); > return JS_TRUE; > } >+#endif Don't remove API. What happens if you try to link without the two #if's added, shown above, which should be removed? /be
Assignee | ||
Comment 14•18 years ago
|
||
This is probably more like what you want. I fixed the link errors from restoring those APIs by lifting some things out of "HAS_SCRIPT_OBJECT" guards.
Attachment #252734 -
Attachment is obsolete: true
Attachment #252744 -
Flags: review?(brendan)
Attachment #252734 -
Flags: review?(brendan)
Comment 15•18 years ago
|
||
Comment on attachment 252744 [details] [diff] [review] Leaving script XDR in >@@ -335,16 +335,18 @@ script_exec(JSContext *cx, JSObject *obj > if (!js_CheckPrincipalsAccess(cx, scopeobj, principals, > CLASS_ATOM(cx, Script))) { > return JS_FALSE; > } > > return js_Execute(cx, scopeobj, script, caller, JSFRAME_EVAL, rval); > } > >+#endif If an #endif or #else is far from its #else, #elif or #if, add a /* JS_HAS_SCRIPT_OBJECT */ comment or the like after it on the same line. >+ > #if JS_HAS_XDR > > static JSBool > XDRAtomMap(JSXDRState *xdr, JSAtomMap *map) > { > JSContext *cx; > uint32 natoms, i, index; > JSAtom **atoms; >@@ -819,16 +821,18 @@ out: > return ok; > } > > static const char js_thaw_str[] = "thaw"; > > #endif /* JS_HAS_XDR_FREEZE_THAW */ > #endif /* JS_HAS_XDR */ Aha, the JS_HAS_XDR_FREEZE_THAW stuff should depend on JS_HAS_SCRIPT_OBJECT too. Fix with #if ... || ... or better (synthesize a new macro if the condition is repeated). Thanks for plugging away at this one. /be
Assignee | ||
Comment 16•18 years ago
|
||
I hope you meant && instead of ||; if so, this should be good to go.
Attachment #252744 -
Attachment is obsolete: true
Attachment #252748 -
Flags: review?(brendan)
Attachment #252744 -
Flags: review?(brendan)
Comment 17•18 years ago
|
||
Comment on attachment 252748 [details] [diff] [review] Alllllllllmost there! >+#endif /* JS_HAS_SCRIPT_OBJECT */ Uber-nit: one space between #endif and the comment (two after #else to line up comment with endif comment). >-#else /* !JS_HAS_XDR_FREEZE_THAW */ >+#else /* JS_HAS_SCRIPT_OBJECT && JS_HAS_XDR_FREEZE_THAW */ Invert condition to say what governs the #else clause -- might be nicer to apply De Morgan's theorem too. > #define script_static_methods NULL > > #endif /* !JS_HAS_XDR_FREEZE_THAW */ This comment needs to match the fixed else comment. Yeah, &&, not || -- tired last night. /be
Assignee | ||
Comment 18•17 years ago
|
||
Attachment #252748 -
Attachment is obsolete: true
Attachment #252805 -
Flags: review?(brendan)
Attachment #252748 -
Flags: review?(brendan)
Comment 19•17 years ago
|
||
Comment on attachment 252805 [details] [diff] [review] one more go Thanks, looks great! /be
Attachment #252805 -
Flags: review?(brendan) → review+
Assignee | ||
Comment 20•17 years ago
|
||
Comment on attachment 252805 [details] [diff] [review] one more go Landed on trunk as: Checking in jsconfig.h; /cvsroot/mozilla/js/src/jsconfig.h,v <-- jsconfig.h new revision: 3.43; previous revision: 3.42 done Checking in jsscript.c; /cvsroot/mozilla/js/src/jsscript.c,v <-- jsscript.c new revision: 3.130; previous revision: 3.129 done Probably worth some bake-time, but also worth porting to branches?
Attachment #252805 -
Flags: approval1.8.1.2?
Attachment #252805 -
Flags: approval1.8.0.10?
Assignee | ||
Comment 21•17 years ago
|
||
Comment on attachment 252805 [details] [diff] [review] one more go Ah, sorry. Another bug suggests no API changes for stable branches on an issue related to this. Removing approval flags.
Attachment #252805 -
Flags: approval1.8.1.2?
Attachment #252805 -
Flags: approval1.8.0.10?
Comment 22•17 years ago
|
||
$ grep warn *made made:jsobj.c:1162: warning: 'scopeobj' may be used uninitialized in this function This is a bad bug in the trunk now, due to mislocated initialization (sorry about that). Please check for warnings. /be
Assignee | ||
Comment 23•17 years ago
|
||
Sorry for missing this.
Attachment #252950 -
Flags: review?(brendan)
Comment 24•17 years ago
|
||
Comment on attachment 252950 [details] [diff] [review] moved the init to the declaration Rather move scopeobj = NULL up just a bit. It's not house style to initialize, since in C that can hide initial values in the crowd at the top of a body or block, and also do work earlier than necessary. /be
Assignee | ||
Comment 25•17 years ago
|
||
Attachment #252950 -
Attachment is obsolete: true
Attachment #252959 -
Flags: review?(brendan)
Attachment #252950 -
Flags: review?(brendan)
Comment 26•17 years ago
|
||
Comment on attachment 252959 [details] [diff] [review] later init Thanks, r=me. /be
Attachment #252959 -
Flags: review?(brendan) → review+
Assignee | ||
Comment 27•17 years ago
|
||
jsobj.c: 3.328
Status: ASSIGNED → RESOLVED
Closed: 17 years ago
Resolution: --- → FIXED
Comment 28•17 years ago
|
||
The last checkin for jsconfig.h has broken the two-argument form of eval(). At least Venkman is using this feature. js> o = {x:1}; eval("print(x);", o); typein:1: ReferenceError: x is not defined http://developer.mozilla.org/en/docs/Core_JavaScript_1.5_Reference:Global_Functions:eval http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/extensions/venkman/resources/content/venkman-commands.js&rev=1.44&mark=396#396 http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/extensions/venkman/resources/content/venkman-eval.js&rev=1.7&mark=50#50
Assignee | ||
Comment 29•17 years ago
|
||
The preprocessor guards here had sliced out code that is needed for eval to work, and isn't purely HAS_SCRIPT_OBJ related.
Attachment #253014 -
Flags: review?(brendan)
Assignee | ||
Comment 30•17 years ago
|
||
Reopening until I have landed the eval fix.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Comment 31•17 years ago
|
||
Comment on attachment 253014 [details] [diff] [review] fixes overzealous preprocessor guarding > scopeobj = NULL; >-#if JS_HAS_SCRIPT_OBJECT > /* > * Script.prototype.compile/exec and Object.prototype.eval all take an > * optional trailing argument that overrides the scope object. > */ > if (argc >= 2) { > if (!js_ValueToObject(cx, argv[1], &scopeobj)) > return JS_FALSE; > argv[1] = OBJECT_TO_JSVAL(scopeobj); > } > if (!scopeobj) >-#endif > { Please fix the brace style here to match house style. > #if JS_HAS_EVAL_THIS_SCOPE > /* If obj.eval(str), emulate 'with (obj) eval(str)' in the caller. */ > if (indirectCall) { > callerScopeChain = js_GetScopeChain(cx, caller); > if (!callerScopeChain) > return JS_FALSE; > OBJ_TO_INNER_OBJECT(cx, obj); >@@ -1338,19 +1336,17 @@ obj_eval(JSContext *cx, JSObject *obj, u > JSSTRING_CHARS(str), > JSSTRING_LENGTH(str), > file, line); > if (!script) { > ok = JS_FALSE; > goto out; > } > >-#if JS_HAS_SCRIPT_OBJECT > if (argc < 2) >-#endif > { Here too; thanks. /be > /* Execute using caller's new scope object (might be a Call object). */ > if (caller) > scopeobj = caller->scopeChain; > } > > /* > * Belt-and-braces: check that the lesser of eval's principals and the
Assignee | ||
Comment 32•17 years ago
|
||
I also moved the scopeobj init back to where it was before I hoisted it out of the preproc. guards, since I have now removed them.
Attachment #253014 -
Attachment is obsolete: true
Attachment #253038 -
Flags: review?(brendan)
Attachment #253014 -
Flags: review?(brendan)
Updated•17 years ago
|
Attachment #253038 -
Flags: review?(brendan) → review+
Assignee | ||
Comment 33•17 years ago
|
||
jsobj.c: 3.329
Status: REOPENED → RESOLVED
Closed: 17 years ago → 17 years ago
Resolution: --- → FIXED
Comment 34•17 years ago
|
||
will fail in gecko prior to 1.9
Updated•17 years ago
|
Flags: blocking1.9? → in-testsuite+
Comment 35•17 years ago
|
||
/cvsroot/mozilla/js/tests/js1_5/extensions/regress-355820.js,v <-- regress-355820.js
Comment 36•17 years ago
|
||
Comment 37•17 years ago
|
||
verified fixed 2007-02-17 1.9.0 windows/mac*/linux
Status: RESOLVED → VERIFIED
Comment 38•17 years ago
|
||
This ended up being relnoted (with a rather confusing wording), but no-one marked it as such (nor as dev-doc-needed) :(
Keywords: dev-doc-needed,
relnote
Comment 39•17 years ago
|
||
I'm still unsure of a good place to document this change, given that the Script object isn't documented anywhere on MDC in the first place. :)
Comment 40•17 years ago
|
||
Anyone have any thoughts on how to go about documenting this change?
Comment 41•17 years ago
|
||
I suggest mentioning it (*not* with the wording from the release notes) on the "New in Fx3" and "Updating web pages" pages, perhaps with the typical error messages that the developers may encounter.
Is Script visible to content even in Fx2? I thought we stopped exposing it there a long time ago, but I haven't done the CVS archaeology or testing to be sure.
Comment 43•17 years ago
|
||
Unfortunately, it was (still is, technically) visible to content until crowder fixed this bug.
Comment 44•17 years ago
|
||
(In reply to comment #42) > Is Script visible to content even in Fx2? I thought we stopped exposing it > there a long time ago, but I haven't done the CVS archaeology or testing to be > sure. You may be thinking of freeze/thaw -- they are long gone. /be
(In reply to comment #44) > You may be thinking of freeze/thaw -- they are long gone. Ah yes, I am. My shames, excised. :)
Comment 46•17 years ago
|
||
Can I get a volunteer to write a little snippet (even in rough, socially unacceptable form) on how to update a web site that uses Script to no longer do so? I'll turn it into something legible -- but I can't find any information easily that leads me to believe I could write something credible on my own in short order. I've added a note to the Fx3 for developers page mentioning the Script object is gone, with a link to a new section in http://developer.mozilla.org/en/docs/Updating_web_applications_for_Firefox_3 that needs to be updated to include info on how to get away from Script.
Assignee | ||
Comment 47•17 years ago
|
||
There shouldn't be any websites that use Script(), IE doesn't provide it, and it is very non-standard. If a site IS using it, Script()s can generally be replaced by functions doing the same things.
Comment 48•17 years ago
|
||
OK, this is as documented as it's going to get then. Marking as doc complete.
Keywords: dev-doc-needed → dev-doc-complete
You need to log in
before you can comment on or make changes to this bug.
Description
•