Remove script object

RESOLVED FIXED in mozilla10

Status

()

Core
JavaScript Engine
RESOLVED FIXED
6 years ago
5 years ago

People

(Reporter: Igor Bukanov, Assigned: Igor Bukanov)

Tracking

Other Branch
mozilla10
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 3 obsolete attachments)

(Assignee)

Description

6 years ago
+++ 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.
(Assignee)

Updated

6 years ago
Depends on: 687966
(Assignee)

Comment 1

6 years ago
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.
Assignee: general → igor
(Assignee)

Comment 2

6 years ago
Comment on attachment 561708 [details] [diff] [review]
v1

The patch has passed the try server.
Attachment #561708 - Flags: review?(jorendorff)
(Assignee)

Comment 3

6 years ago
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.
Attachment #561708 - Attachment is obsolete: true
Attachment #561708 - Flags: review?(jorendorff)
Attachment #562012 - Flags: review?(jorendorff)
(Assignee)

Comment 4

6 years ago
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.
Attachment #562012 - Attachment is obsolete: true
Attachment #562012 - Flags: review?(jorendorff)
Attachment #562103 - Flags: review?(jorendorff)
(Assignee)

Comment 5

6 years ago
Created attachment 564983 [details] [diff] [review]
v3

Here is a rebased patch
Attachment #562103 - Attachment is obsolete: true
Attachment #562103 - Flags: review?(jorendorff)
Attachment #564983 - Flags: review?(jorendorff)
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.
Attachment #564983 - Flags: review?(jorendorff) → review+
(Assignee)

Comment 7

6 years ago
(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.
(Assignee)

Comment 8

6 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/d6f9285f623e
https://hg.mozilla.org/mozilla-central/rev/d6f9285f623e
Status: NEW → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla10
Depends on: 806522
You need to log in before you can comment on or make changes to this bug.