Closed Bug 1466118 Opened 2 years ago Closed 2 years ago

Audit/fix assertSameCompartment calls for same-compartment realms

Categories

(Core :: JavaScript Engine, enhancement, P3)

enhancement

Tracking

()

RESOLVED FIXED
mozilla63
Tracking Status
firefox63 --- fixed

People

(Reporter: jandem, Assigned: jandem)

References

Details

Attachments

(9 files)

These should still hold, but some of them should be changed to assert same-realm instead, a stronger invariant. We should try to come up with a nice API for this.

Especially scripts will usually be same-realm so it would be nice if we could default to realm asserts for those somehow, but I haven't really thought about it  yet.

(Also, I've always wanted to s/assertSameCompartment/AssertSameCompartment/ - not trivial because js::AssertSameCompartment exists as friend API...)
Thinking about it more, assertSameCompartment is already a misnomer, because for strings it checks the zone and for atoms/symbols it checks whether their mark bit is set in the current Zone, completely unrelated to compartments.

One option is to do something like this:

class ContextAsserts
{
  private:
    JSContext* cx_;

  public:
    explicit ContextAsserts(cx);

    ContextAsserts& checkRealm(realm);
    ContextAsserts& checkCompartment(compartment);
    ContextAsserts& checkZone(zone);
    ContextAsserts& checkAtom(atom or symbol);

    // These forward to one of the above.
    ContextAsserts& checkRealm(script);
    ContextAsserts& checkCompartment(object);
    ContextAsserts& checkRealm(object); // maybe

    // These forward to one of the above, depending on what's in them.
    ContextAsserts& check(string);
    ContextAsserts& check(Value);
    ContextAsserts& check(jsid);

    ... maybe some templated variants that accept multiple arguments, for convenience ...
};

Then for a typical SetProperty with obj/id/val arguments, we can do:

  ContextAsserts(cx).checkCompartment(obj).check(id, val);

Another option is to also add check(obj) and have it default to checkCompartment(obj), so then it's just:

  ContextAsserts(cx).check(obj, id, val);

Similarly, check(script) could default to checkRealm(script).

You could still override this default for objects/scripts etc by calling checkRealm/checkCompartment/checkZone explicitly.
Any thoughts on the design in comment 1 and the options mentioned there? Just to avoid writing a patch for this and then rewriting it after review :)
Flags: needinfo?(luke)
Sorry for the delayed feedback!  IIUC, the only case where there is any choice in whether to check same-realm vs. same-compartment is object, with same-compartment being the default/normal case.  Given that, what about:
  cx->check(anything1, anything2, ...)
and using variadics and overloading to do the right thing.  In the rare case of asserting same-object-realm, perhaps just MOZ_ASSERT(cx->realm() == obj->realm()) is better since it calls out the special case where we have, or rely on, a tighter invariant than the default one.
Flags: needinfo?(luke)
That's nice and simple and close to what we already have, so WFM. Thanks!
This also removes the START_ASSERT_SAME_COMPARTMENT macro :)
Assignee: nobody → jdemooij
Status: NEW → ASSIGNED
Attachment #9002715 - Flags: review?(luke)
Just a small optimization.
Attachment #9002721 - Flags: review?(jcoppeard)
I verified compartment checks still work correctly with these patches (by adding cx->check(obj) and similar to Compartment::wrap).
Attachment #9002721 - Flags: review?(jcoppeard) → review+
Comment on attachment 9002715 [details] [diff] [review]
Part 1 - Use variadic templates for assertSameCompartment functions

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

::: js/src/vm/JSContext-inl.h
@@ +180,5 @@
> +    // depends on other objects not having been swept yet.
> +    if (JS::RuntimeHeapIsCollecting())
> +        return;
> +    CompartmentChecker c(cx);
> +    c.check(t1, argIndex);

I don't know how widely the release compartment asserts are used, but I wonder if there's any perf benefit to having a single RuntimeHeapIsCollecting() check and CompartmentChecker object created at the root of the recursive call and having the CompartmentChecker passed downward via reference.
Attachment #9002715 - Flags: review?(luke) → review+
Attachment #9002716 - Flags: review?(luke) → review+
Comment on attachment 9002717 [details] [diff] [review]
Part 3 - Replace assertSameCompartmentDebugOnly with JSContext::debugOnlyCheck

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

::: js/src/vm/JSContext-inl.h
@@ +210,2 @@
>  template <class... Args> inline void
> +JSContext::debugOnlyCheck(const Args&... args)

Maybe worth a MOZ_ALWAYS_INLINE (for the benefit of Interpret() which, I expect due to crossing some limit, is pretty aggressive about not inlining things unless they are marked always-inline).

::: js/src/vm/JSContext.h
@@ +954,5 @@
>                             js::HandleObject incumbentGlobal);
>      void addUnhandledRejectedPromise(JSContext* cx, js::HandleObject promise);
>      void removeUnhandledRejectedPromise(JSContext* cx, js::HandleObject promise);
>  
> +    template <class... Args> inline void debugOnlyCheck(const Args&... args);

nit: IMO 'debugCheck()' would be basically as clear, shorter and symmetric with 'releaseCheck()', but up to you.
Attachment #9002717 - Flags: review?(luke) → review+
Comment on attachment 9002717 [details] [diff] [review]
Part 3 - Replace assertSameCompartmentDebugOnly with JSContext::debugOnlyCheck

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

::: js/src/vm/JSContext.h
@@ +954,5 @@
>                             js::HandleObject incumbentGlobal);
>      void addUnhandledRejectedPromise(JSContext* cx, js::HandleObject promise);
>      void removeUnhandledRejectedPromise(JSContext* cx, js::HandleObject promise);
>  
> +    template <class... Args> inline void debugOnlyCheck(const Args&... args);

Actually, n/m, I see how this contrasts with 'check()' in the next patch.
Comment on attachment 9002718 [details] [diff] [review]
Part 4 - Replace assertSameCompartment with JSContext::check

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

Much prettier.
Attachment #9002718 - Flags: review?(luke) → review+
Attachment #9002719 - Flags: review?(luke) → review+
Attachment #9002720 - Flags: review?(luke) → review+
Attachment #9002722 - Flags: review?(luke) → review+
Attachment #9003084 - Flags: review?(luke)
Attachment #9003084 - Flags: review?(luke) → review+
Pushed by jandemooij@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/3bf5eb6fe16d
part 1 - Use variadic templates for assertSameCompartment functions. r=luke
https://hg.mozilla.org/integration/mozilla-inbound/rev/5cb4cd7c449e
part 2 - Replace releaseAssertSameCompartment with JSContext::releaseCheck. r=luke
https://hg.mozilla.org/integration/mozilla-inbound/rev/224b09c2e661
part 3 - Replace assertSameCompartmentDebugOnly with JSContext::debugOnlyCheck. r=luke
https://hg.mozilla.org/integration/mozilla-inbound/rev/239b363ac50d
part 4 - Replace assertSameCompartment with JSContext::check. r=luke
https://hg.mozilla.org/integration/mozilla-inbound/rev/af49f7a464d5
part 5 - Replace assertSameCompartmentImpl with JSContext::checkImpl. r=luke
https://hg.mozilla.org/integration/mozilla-inbound/rev/ff5cb8442b5d
part 6 - Rename CompartmentChecker to ContextChecks and support realm checks. r=luke
https://hg.mozilla.org/integration/mozilla-inbound/rev/1a263d3e088e
part 7 - Avoid a TLS lookup for each compartment check. r=jonco
https://hg.mozilla.org/integration/mozilla-inbound/rev/64a85b3753ac
part 8 - Change compartment check to realm check for JSScript and AbstractFramePtr. r=luke
https://hg.mozilla.org/integration/mozilla-inbound/rev/87a88fb8b1a6
part 9 - Some more cleanup. r=luke
Depends on: 1487238
Depends on: 1496892
You need to log in before you can comment on or make changes to this bug.