1337502 - js::AutoCompartment should assert the same things JSAutoCompartment does

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Boris Zbarsky [:bzbarsky]]

Compare:

  JSAutoCompartment::JSAutoCompartment(JSContext* cx, JSObject* target
                                       MOZ_GUARD_OBJECT_NOTIFIER_PARAM_IN_IMPL)
...
    AssertHeapIsIdleOrIterating();
    MOZ_GUARD_OBJECT_NOTIFIER_INIT;
    MOZ_ASSERT(!JS::ObjectIsMarkedGray(target));
    cx_->enterCompartment(target->compartment());

and

  js::AutoCompartment::AutoCompartment(JSContext* cx, JSObject* target,
                                       js::AutoLockForExclusiveAccess* maybeLock /* = nullptr */)
...
    cx_->enterCompartment(target->compartment(), maybeLock);

In particular, note the lack of MOZ_ASSERT(!JS::ObjectIsMarkedGray(target)).  Not sure whether the "idle or iterating" assert is relevant here too.

Comment 1

[Boris Zbarsky [:bzbarsky]]

Of course it would be even snazzier if we could manage to have only _one_ class to enter compartments...

Comment 2

[Jon Coppeard (:jonco)]

Attachment: bug1337502-auto-compartment-asserts

AutoCompartment constructors are inlined whereas JSAutoCompartment ones are not, so I made these all call new JSCompartment::enterCompartmentOf methods which assert their argument is not gray.

The wrapper remapping code was a special case because this does operate on wrappers that may be gray so I added a special AutoCompartmentMaybeGray for use there and changed JSObject::swap to require being called in the right compartment.

Comment 3

[Steve Fink [:sfink] [:s:]]

Comment on attachment 8836103 [details] [diff] [review]
bug1337502-auto-compartment-asserts

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

::: js/src/jscntxt.h
@@ +199,5 @@
>  
>      // If |c| or |oldCompartment| is the atoms compartment, the
>      // |exclusiveAccessLock| must be held.
>      inline void enterCompartment(JSCompartment* c,
>                                   const js::AutoLockForExclusiveAccess* maybeLock = nullptr);

Maybe I'm wrong, but enterCompartmentOf just seems like an all-around better API and enterCompartment should not be exposed. Can you make enterCompartment private? I only see a very small handful of uses, but most (in jsapi.cpp) have now-redundant !gray asserts. I *think* the helper thread one should be asserting !gray too?

You will need to friend AutoCompartmentAllowGray, which is unfortunate, but then again it kind of describes the intent here. I'd almost be tempted to rename enterCompartment to changeCompartmentTo or setCompartment, but I think that might be a step too far given that it calls a bunch of things named "enter*".

::: js/src/jscntxtinlines.h
@@ +455,5 @@
> +    JSObject* obj,
> +    const js::AutoLockForExclusiveAccess* maybeLock /* = nullptr */)
> +{
> +    MOZ_ASSERT(!JS::ObjectIsMarkedGray(obj));
> +    enterCompartment(obj->compartment());

Shouldn't you be passing through maybeLock here?

@@ +464,5 @@
> +    JSScript* script,
> +    const js::AutoLockForExclusiveAccess* maybeLock /* = nullptr */)
> +{
> +    MOZ_ASSERT(!JS::ScriptIsMarkedGray(script));
> +    enterCompartment(script->compartment());

...and here?

Comment 4

[Jon Coppeard (:jonco)]

(In reply to Steve Fink [:sfink] [:s:] from comment #3)
> Maybe I'm wrong, but enterCompartmentOf just seems like an all-around better
> API and enterCompartment should not be exposed. Can you make
> enterCompartment private?

Good plan, I'll try that.

> Shouldn't you be passing through maybeLock here?

Ugh, yeah.  Thanks.

Comment 5

[Jon Coppeard (:jonco)]

Attachment: bug1337502-auto-compartment-asserts v2

Updated patch.

As suggested I made enterCompartment private and I also removed the AutoCompartment constructor that took a JSCompartment*.  I added a separate class to enter the atoms compartment.  There were a few places where we do really need to pass a compartment (including the wrapper remapping case and when the compartment is empty) so I made AutoCompartmentUnchecked for these.  I was able to make enterCompartmentOf and the AutoCompartment constructor templates, but I had to pass the argument by const reference so that it can be used with Rooted<>.

Comment 6

[Steve Fink [:sfink] [:s:]]

Comment on attachment 8836755 [details] [diff] [review]
bug1337502-auto-compartment-asserts v2

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

::: js/src/jscompartment.h
@@ +962,4 @@
>  
>    public:
> +    template <typename T>
> +    inline AutoCompartment(JSContext* cx, const T& target);

Oh wow, you went the whole way and templatized this, not just overloaded it. Hopefully that won't cause issues for servo.

@@ +984,5 @@
> +};
> +
> +// Enter a compartment directly. Only use this where there's no target GC thing
> +// to pass to AutoCompartment or where you need to avoid the assertions in
> +// JS::Compartment::enterCompartmentOf().

The Unchecked part makes me wonder what is being checked, and that perhaps this should be called AutoMaybeGrayCompartment to make it explicit, but then that makes it sound like the whole compartment is gray or something. So never mind.

::: js/src/vm/Debugger.cpp
@@ +2574,5 @@
>      }
>  
>      // See comment in unsetPrevUpToDateUntil.
>      if (oldestEnabledFrame) {
> +        AutoCompartment ac(cx, oldestEnabledFrame.environmentChain());

I kinda wish environmentChain returned a reference so I wouldn't have to check whether it can be null. Because it can't. Anyway, it's good here.

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/1851854d9dd5
AutoCompartment should have the same asserts as JSAutoCompartment r=sfink

Comment 8

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/1851854d9dd5