Closed Bug 952885 Opened 6 years ago Closed 6 years ago

JS shell: Supplying an owning element for a cross-global compilation violates compartment rules

Categories

(Core :: JavaScript Engine, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla29

People

(Reporter: jimb, Assigned: jimb)

References

Details

Attachments

(1 file)

Patch includes a test that crashes without the included fix.
I do not love thee, Dr. Bugzilla
Assignee: nobody → jimb
Status: NEW → ASSIGNED
Attachment #8351088 - Flags: review?(n.nethercote)
Try run seems okay.
Blocks: 944121
Comment on attachment 8351088 [details] [diff] [review]
When doing cross-global compilations in the shell, properly wrap CompileOptions members for the new global.

Review of attachment 8351088 [details] [diff] [review]:
-----------------------------------------------------------------

Seems reasonable, though I'm not the best person to review this -- I don't know much about CCWs.  But it's simple enough that I'm ok to r+ it.

::: js/src/jsapi.cpp
@@ +4449,5 @@
> +    if (elementPropertyRoot) {
> +        if (!compartment->wrap(cx, elementPropertyRoot.address()))
> +            return false;
> +    }
> +    return true;

This could be shorter:

  if (!compartment->wrap(cx, &elementRoot))
      return false;
  if (elementPropertyRoot)
      return compartment->wrap(cx, elementPropertyRoot.address()))
  return true;

@@ +4474,5 @@
> +    if (elementPropertyRoot) {
> +        if (!compartment->wrap(cx, elementPropertyRoot.address()))
> +            return false;
> +    }
> +    return true;

Ditto.

::: js/src/jsapi.h
@@ +3539,5 @@
>      OwningCompileOptions &setSelfHostingMode(bool shm) { selfHostingMode = shm; return *this; }
>      OwningCompileOptions &setCanLazilyParse(bool clp) { canLazilyParse = clp; return *this; }
>      OwningCompileOptions &setSourcePolicy(SourcePolicy sp) { sourcePolicy = sp; return *this; }
> +
> +    bool wrap(JSContext *cx, JSCompartment *compartment) MOZ_OVERRIDE;

I'd put |virtual| on this, even though it's not necessary.  (I don't think the SM style guide says anything about this, but the Gecko style guide recommends doing it.)

@@ +3597,5 @@
>      CompileOptions &setSelfHostingMode(bool shm) { selfHostingMode = shm; return *this; }
>      CompileOptions &setCanLazilyParse(bool clp) { canLazilyParse = clp; return *this; }
>      CompileOptions &setSourcePolicy(SourcePolicy sp) { sourcePolicy = sp; return *this; }
> +
> +    bool wrap(JSContext *cx, JSCompartment *compartment) MOZ_OVERRIDE;

Ditto.
Attachment #8351088 - Flags: review?(n.nethercote) → review+
(In reply to Nicholas Nethercote [:njn] from comment #4)
> @@ +4449,5 @@
> > +    if (elementPropertyRoot) {
> > +        if (!compartment->wrap(cx, elementPropertyRoot.address()))
> > +            return false;
> > +    }
> > +    return true;
> 
> This could be shorter:
> 
>   if (!compartment->wrap(cx, &elementRoot))
>       return false;
>   if (elementPropertyRoot)
>       return compartment->wrap(cx, elementPropertyRoot.address()))
>   return true;

That's true, but I think it's also important to give error returns a standard "shape" - an early return. Also, since we will be adding more object members to ReadOnlyCompilationOptions, this function will grow to look more and more like a list of member-rewrapping calls, and the return you suggest conditional only on elementPropertyRoot will become inappropriate. These further members are already specified, and I think some have been implemented; I'm just revising Eddy's patches now.

Would it be all right to leave the code as it is?
Flags: needinfo?(n.nethercote)
(In reply to Nicholas Nethercote [:njn] from comment #4)
> ::: js/src/jsapi.h
> @@ +3539,5 @@
> >      OwningCompileOptions &setSelfHostingMode(bool shm) { selfHostingMode = shm; return *this; }
> >      OwningCompileOptions &setCanLazilyParse(bool clp) { canLazilyParse = clp; return *this; }
> >      OwningCompileOptions &setSourcePolicy(SourcePolicy sp) { sourcePolicy = sp; return *this; }
> > +
> > +    bool wrap(JSContext *cx, JSCompartment *compartment) MOZ_OVERRIDE;
> 
> I'd put |virtual| on this, even though it's not necessary.  (I don't think
> the SM style guide says anything about this, but the Gecko style guide
> recommends doing it.)
> 
> @@ +3597,5 @@
> >      CompileOptions &setSelfHostingMode(bool shm) { selfHostingMode = shm; return *this; }
> >      CompileOptions &setCanLazilyParse(bool clp) { canLazilyParse = clp; return *this; }
> >      CompileOptions &setSourcePolicy(SourcePolicy sp) { sourcePolicy = sp; return *this; }
> > +
> > +    bool wrap(JSContext *cx, JSCompartment *compartment) MOZ_OVERRIDE;
> 
> Ditto.

Ah, good thought. Changed as suggested.
> Would it be all right to leave the code as it is?

Sure.
Flags: needinfo?(n.nethercote)
Duplicate of this bug: 952381
https://hg.mozilla.org/integration/mozilla-inbound/rev/3743ea445b81
Flags: in-testsuite+
Target Milestone: --- → mozilla29
https://hg.mozilla.org/mozilla-central/rev/3743ea445b81
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.