Closed Bug 355820 Opened 18 years ago Closed 17 years ago

Remove non-standard Script object

Categories

(Core :: JavaScript Engine, defect)

defect
Not set
normal

Tracking

()

VERIFIED FIXED

People

(Reporter: Gavin, Assigned: crowderbt)

Details

(Keywords: dev-doc-complete, relnote)

Attachments

(5 files, 8 obsolete files)

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".
Flags: blocking1.9?
Brian, could you please field this one?  Thanks,

/be
Assignee: brendan → crowder
Status: NEW → ASSIGNED
Attached patch no more script object (obsolete) — Splinter Review
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 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-
Attached patch much lighter touch (obsolete) — Splinter Review
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)
Comment on attachment 252628 [details] [diff] [review]
much lighter touch

Disregard, this is broken.
Attachment #252628 - Flags: review?(brendan)
Attached patch not broken version (obsolete) — Splinter Review
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
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)
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.
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?
Oh, and the slotname code must surely be faster this way?
We want fixed proto keys for XDR backward compatibility -- revert that part of the patch, please.

/be
Attached patch toning it down a bit (obsolete) — Splinter Review
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)
Attachment #252734 - Flags: review?(brendan)
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
Attached patch Leaving script XDR in (obsolete) — Splinter Review
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 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
Attached patch Alllllllllmost there! (obsolete) — Splinter Review
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 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
Attached patch one more goSplinter Review
Attachment #252748 - Attachment is obsolete: true
Attachment #252805 - Flags: review?(brendan)
Attachment #252748 - Flags: review?(brendan)
Comment on attachment 252805 [details] [diff] [review]
one more go

Thanks, looks great!

/be
Attachment #252805 - Flags: review?(brendan) → review+
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?
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?
$ 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
Sorry for missing this.
Attachment #252950 - Flags: review?(brendan)
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
Attached patch later initSplinter Review
Attachment #252950 - Attachment is obsolete: true
Attachment #252959 - Flags: review?(brendan)
Attachment #252950 - Flags: review?(brendan)
Comment on attachment 252959 [details] [diff] [review]
later init

Thanks, r=me.

/be
Attachment #252959 - Flags: review?(brendan) → review+
jsobj.c:  3.328
Status: ASSIGNED → RESOLVED
Closed: 17 years ago
Resolution: --- → FIXED
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)
Reopening until I have landed the eval fix.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
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
Attached patch fixed bracingSplinter Review
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)
Attachment #253038 - Flags: review?(brendan) → review+
jsobj.c: 3.329
Status: REOPENED → RESOLVED
Closed: 17 years ago17 years ago
Resolution: --- → FIXED
will fail in gecko prior to 1.9
Flags: blocking1.9? → in-testsuite+
/cvsroot/mozilla/js/tests/js1_5/extensions/regress-355820.js,v  <--  regress-355820.js
verified fixed 2007-02-17 1.9.0 windows/mac*/linux
Status: RESOLVED → VERIFIED
This ended up being relnoted (with a rather confusing wording), but no-one marked it as such (nor as dev-doc-needed) :(
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. :)
Anyone have any thoughts on how to go about documenting this change?
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.
Unfortunately, it was (still is, technically) visible to content until crowder fixed this bug.
(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. :)
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.
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.
OK, this is as documented as it's going to get then.  Marking as doc complete.
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: