Last Comment Bug 684529 - Remove script object
: Remove script object
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: Other Branch
: All All
: -- normal (vote)
: mozilla10
Assigned To: Igor Bukanov
:
Mentors:
Depends on: 678830 687966 806522
Blocks:
  Show dependency treegraph
 
Reported: 2011-09-03 12:43 PDT by Igor Bukanov
Modified: 2012-10-29 13:16 PDT (History)
16 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
v1 (40.61 KB, patch)
2011-09-22 05:28 PDT, Igor Bukanov
no flags Details | Diff | Splinter Review
v2 (61.07 KB, patch)
2011-09-23 03:49 PDT, Igor Bukanov
no flags Details | Diff | Splinter Review
v2 for real (68.90 KB, patch)
2011-09-23 10:47 PDT, Igor Bukanov
no flags Details | Diff | Splinter Review
v3 (67.83 KB, patch)
2011-10-05 13:29 PDT, Igor Bukanov
jorendorff: review+
Details | Diff | Splinter Review

Description Igor Bukanov 2011-09-03 12:43:07 PDT
+++ This bug was initially created as a clone of Bug #678830 +++

The bug 678830 mostly stops exposing JSScript object wrappers through JS API. But those objects are still created as a mean to reach global objects corresponding to the script and do security checks. 

We should remove the object wrappers and instead make sure that we always record the global object in the script and use it for security checks.
Comment 1 Igor Bukanov 2011-09-22 05:28:41 PDT
Created attachment 561708 [details] [diff] [review]
v1

Instead of creating the script objects the patch records compile-time global for the script and uses that in all the users of JSScript::u.object. With one global per compartment that would not be unnecessary but for now this avoids the extra memory bloat.
Comment 2 Igor Bukanov 2011-09-22 13:29:57 PDT
Comment on attachment 561708 [details] [diff] [review]
v1

The patch has passed the try server.
Comment 3 Igor Bukanov 2011-09-23 03:49:36 PDT
Created attachment 562012 [details] [diff] [review]
v2

The new version records in JSScript::u.globalObject (former u.object) the global objects for function scripts if any. That removed the need to store the global in a separated slot in DebuggerObject and eliminated the global object parmeter from a few Debugger functions that was present in v1.
Comment 4 Igor Bukanov 2011-09-23 10:47:32 PDT
Created attachment 562103 [details] [diff] [review]
v2 for real

v2 was an incomplete patch, this should the real one build on top of the patch from the bug 687966.
Comment 5 Igor Bukanov 2011-10-05 13:29:25 PDT
Created attachment 564983 [details] [diff] [review]
v3

Here is a rebased patch
Comment 6 Jason Orendorff [:jorendorff] 2011-10-11 15:23:02 PDT
Comment on attachment 564983 [details] [diff] [review]
v3

r=me with some nits.

In jscompartment.h:
>-}
> 
> /* Defined in jsapi.cpp */
>-extern JSClass js_dummy_class;
>+extern Class js_dummy_class;
>+
>+} /* namespace js */

If it's going to live in namespace js, it shouldn't have js_ at the
beginning of the name.

In jsscript.cpp, JSScript::NewScriptFromCG:
>+        script->u.globalObject = fun->getParent() ? fun->getParent()->getGlobal() : NULL;

I assume fun->getParent() is null here iff compilation is not compile-and-go?

In jsscript.h, in struct JSScript:
>     union {
>         /*
>-         * A script object of class ScriptClass, to ensure the script is GC'd.
>+         * A global object for the script.
>          * - All scripts returned by JSAPI functions (JS_CompileScript,
>+         *   JS_CompileFile, etc.) have these globals.

"have a globalObject" or "have a non-null globalObject".

>+         * - Function scripts have these global if the function comes from
>+         *   compile and go scrits.

"A function script has a globalObject if the function comes from a
compile-and-go script."

>          * - Temporary scripts created by obj_eval, JS_EvaluateScript, and
>+         *   similar functions never have these globals; for such scripts
>+         *   the global should be extracted from the JS frame that execute
>+         *   scripts.

"never have the globalObject field set;"

>+    /*
>+     * Return creation time global or null. FIXME - avoid duplication with
>+     * global() that extracts the global from the type information.
>+     */
>+    js::GlobalObject *getGlobalObjectOrNull() const {

How were you planning to fix this?

In vm/Debugger.cpp, DebuggerScript_check:
>-    JS_ASSERT(GetScriptReferent(thisobj));
>-
>+ 
>     return thisobj;
> }

Stray space charater on that line.
Comment 7 Igor Bukanov 2011-10-24 05:19:36 PDT
(In reply to Jason Orendorff [:jorendorff] from comment #6)
> 
> In jsscript.cpp, JSScript::NewScriptFromCG:
> >+        script->u.globalObject = fun->getParent() ? fun->getParent()->getGlobal() : NULL;
> 
> I assume fun->getParent() is null here iff compilation is not compile-and-go?

Yes, this is true.

> >+    /*
> >+     * Return creation time global or null. FIXME - avoid duplication with
> >+     * global() that extracts the global from the type information.
> >+     */
> >+    js::GlobalObject *getGlobalObjectOrNull() const {
> 
> How were you planning to fix this?

I will remove those comments. Compartment-per-global would address this in a better way.
Comment 9 Marco Bonardo [::mak] (Away 6-20 Aug) 2011-10-25 04:58:16 PDT
https://hg.mozilla.org/mozilla-central/rev/d6f9285f623e

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