Status

()

defect
RESOLVED FIXED
8 years ago
7 years ago

People

(Reporter: igor, Assigned: igor)

Tracking

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

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 3 obsolete attachments)

+++ 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.
Depends on: 687966
Posted patch v1 (obsolete) — Splinter Review
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
Comment on attachment 561708 [details] [diff] [review]
v1

The patch has passed the try server.
Attachment #561708 - Flags: review?(jorendorff)
Posted patch v2 (obsolete) — Splinter Review
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)
Posted patch v2 for real (obsolete) — Splinter Review
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)
Posted patch v3Splinter Review
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+
(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.
https://hg.mozilla.org/mozilla-central/rev/d6f9285f623e
Status: NEW → RESOLVED
Closed: 8 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.