Closed Bug 1463163 Opened Last year Closed Last year

Make ArraySpeciesCreate realm check work with 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

(1 file)

If I understand the spec correctly, this involves:

* IsArraySpecies can take the fast path if cx->realm() != constructor->realm().

* IsWrappedArrayConstructor should be renamed to IsCrossRealmArrayConstructor and it should also check for non-wrapped array constructors with realm() != cx->realm().

* Ion inlining of this intrinsic needs to check group->realm() in addition to checking for proxy classes.

This should be straight-forward but we should just wait until we can actually test this or get test failures.
Priority: -- → P3
Assignee: nobody → jdemooij
Status: NEW → ASSIGNED
Summary: Make CreateArraySpecies realm check work with same-compartment realms → Make ArraySpeciesCreate realm check work with same-compartment realms
This makes the changes described in comment 0.

Note that TemporaryTypeSet::getKnownRealm is based on TemporaryTypeSet::getKnownClass, maybe that makes it easier to review.

This also fixes the remaining jsreftest failures when I change the test262 shell harness to create same-compartment realms for $262.createRealm().
Attachment #8989145 - Flags: review?(andrebargull)
Comment on attachment 8989145 [details] [diff] [review]
Make ArraySpeciesCreate realm check work with same-compartment realms

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

LGTM!

::: js/src/builtin/Array.cpp
@@ +1028,3 @@
>      return obj->is<JSFunction>() &&
>             obj->as<JSFunction>().isNative() &&
>             obj->as<JSFunction>().native() == ArrayConstructor;

Pre-existing: Can you change this to call |IsNativeFunction| to avoid a bit of code duplication? And always using IsNativeFunction for these check should also help to discover more places where realm checks are needed for same-compartment-realms.

::: js/src/vm/TypeInference.cpp
@@ +2367,5 @@
> +        // return |false| so fail now before attaching any constraints.
> +        if (clasp->isProxy() || getObject(i)->unknownProperties())
> +            return nullptr;
> +
> +        Realm* nrealm = hasSingleton(i) ? getSingleton(i)->nonCCWRealm() : getGroup(i)->realm();

Maybe assert `MOZ_ASSERT(hasSingleton(i) || hasGroup(i));` to give the reader a hint that only these two options can occur here?
Attachment #8989145 - Flags: review?(andrebargull) → review+
Pushed by jandemooij@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/07b0a9838f2d
Make ArraySpeciesCreate realm check work with same-compartment realms. r=anba
https://hg.mozilla.org/mozilla-central/rev/07b0a9838f2d
Status: ASSIGNED → RESOLVED
Closed: Last year
Resolution: --- → FIXED
Target Milestone: --- → mozilla63
You need to log in before you can comment on or make changes to this bug.