Closed Bug 1797750 Opened 2 years ago Closed 2 years ago

Fix some issues in the ShadowRealm implementation

Categories

(Core :: JavaScript: Standard Library, task)

Product:
Core
Component:
JavaScript: Standard Library
Type:
task

Tracking

()

Status:
RESOLVED FIXED
Milestone:
108 Branch
Tracking Flags:
Tracking Status
firefox108 --- fixed

People

(Reporter: anba, Assigned: anba)

References

Details

Attachments

(11 files)

Bug 1797750 - Part 1: Add trailing comma for nicer clang-format output. r=mgaudet!
48 bytes, text/x-phabricator-request
Details | Review
Bug 1797750 - Part 2: Remove unnecessary rooting. r=mgaudet!
48 bytes, text/x-phabricator-request
Details | Review
Bug 1797750 - Part 3: Prefer internal API instead of the public JS API. r=mgaudet!
48 bytes, text/x-phabricator-request
Details | Review
Bug 1797750 - Part 4: Clean-up CopyNameAndLength. r=mgaudet!
48 bytes, text/x-phabricator-request
Details | Review
Bug 1797750 - Part 5: Fix a typo. r=mgaudet!
48 bytes, text/x-phabricator-request
Details | Review
Bug 1797750 - Part 6: Use an array to store extra handler values. r=mgaudet!
48 bytes, text/x-phabricator-request
Details | Review
Bug 1797750 - Part 7: Use NewBuiltinClassInstance instead of explicitly passing the prototype. r=mgaudet!
48 bytes, text/x-phabricator-request
Details | Review
Bug 1797750 - Part 8: Don't attempt to use the current name property for the toString output. r=mgaudet!
48 bytes, text/x-phabricator-request
Details | Review
Bug 1797750 - Part 9: Implement OrdinaryWrappedFunctionCall. r=mgaudet!
48 bytes, text/x-phabricator-request
Details | Review
Bug 1797750 - Part 10: Correct compartment/realm handling in WrappedFunctionCreate. r=mgaudet!
48 bytes, text/x-phabricator-request
Details | Review
Bug 1797750 - Part 11: Use UnwrapAndTypeCheckValue to correctly unwrap non-CCW wrappers. r=mgaudet!
48 bytes, text/x-phabricator-request
Details | Review
No description provided.

Clang-format writes each entry on a separate line when a trailing comma is
present. This produces nicer output in some cases.

  • CallArgs::operator[] returns a rooted value, so it's not necessary to
    re-root it.
  • It's not necessary to root a value when it's stored in CallArgs::rval().

Depends on D160504

Prefer the internal API for consistency with the rest of SpiderMonkey. This
also avoids some extra overhead, for example we don't have to store the call
arguments first in RootedVector<Valu> and then copy them into InvokeArgs.

Directly using InvokeArgs also fixes some cross-compartment assertions in the
current code, because it skips the compartment checks from JS::Call when it
directly calls js::Call. Part 10 will fix this code to use the correct
compartment for all values.

Depends on D160505

Avoid to call the resolve hook for the "name" property and avoid unnecessary
atomisation of the "name" string: IdToFunctionName returns its input when
no function prefix is passed and the input is a JSAtom. And JS_StringToId only
atomises the input string. Both operations are unnecessary here.

Also avoid single letter, upper case variable names for consistency with the
rest of the SpiderMonkey code base.

Depends on D160506

Depends on D160507

This matches how we handle this case for Promise handlers and it's much
faster.

Depends on D160508

NewBuiltinClassInstance automatically selects the correct prototype.

Depends on D160509

The previous code assumed that the "name" property is always a string, but
user-code can redefine it to any value, which could easily lead to an exploit.

Furthermore the spec requires that the computed string matches the NativeFunction
production, so even if we only append the "name" when it's a string value, we'd
first have to parse the string as an identifier to determine if the string has to
be escaped or not. This is non-trivial, so instead follow the toString output
of bound functions and return an unnamed function source code string.

Depends on D160510

Move the AutoRealm in WrappedFunctionCreate into a separate block to ensure
we're back in the original realm when we call CopyNameAndLength. Without this
change an error thrown CopyNameAndLength can escape the wrong realm to resp.
from the ShadowRealm.

Also add some cx->check() calls to ensure arguments are in the correct compartment
in GetWrappedValue and WrappedFunctionCreate. And add cx->compartment()->wrap()
in some places which we're using the wrong compartment.

Depends on D160512

ValidateShadowRealmObject didn't correctly handle non-CCW wrappers and was
missing error reporting when CheckedUnwrapDynamic() returned nullptr. Switch
to UnwrapAndTypeCheckValue to fix both issues.

Depends on D160513

Pushed by andre.bargull@gmail.com: https://hg.mozilla.org/integration/autoland/rev/4e673df3058a Part 1: Add trailing comma for nicer clang-format output. r=mgaudet https://hg.mozilla.org/integration/autoland/rev/3a6e1feb1ebc Part 2: Remove unnecessary rooting. r=mgaudet https://hg.mozilla.org/integration/autoland/rev/bec9fda2f8ce Part 3: Prefer internal API instead of the public JS API. r=mgaudet https://hg.mozilla.org/integration/autoland/rev/947432f9bd0d Part 4: Clean-up CopyNameAndLength. r=mgaudet https://hg.mozilla.org/integration/autoland/rev/544c58f111c2 Part 5: Fix a typo. r=mgaudet https://hg.mozilla.org/integration/autoland/rev/9c02299f0d56 Part 6: Use an array to store extra handler values. r=mgaudet https://hg.mozilla.org/integration/autoland/rev/8787060a075a Part 7: Use NewBuiltinClassInstance instead of explicitly passing the prototype. r=mgaudet https://hg.mozilla.org/integration/autoland/rev/55b2f301b941 Part 8: Don't attempt to use the current name property for the toString output. r=mgaudet https://hg.mozilla.org/integration/autoland/rev/4e551959f63a Part 9: Implement OrdinaryWrappedFunctionCall. r=mgaudet https://hg.mozilla.org/integration/autoland/rev/c93b57fe1b66 Part 10: Correct compartment/realm handling in WrappedFunctionCreate. r=mgaudet https://hg.mozilla.org/integration/autoland/rev/6d7bcd4a08bb Part 11: Use UnwrapAndTypeCheckValue to correctly unwrap non-CCW wrappers. r=mgaudet
Backout by sstanca@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/c3b27791729f Backed out 11 changesets for causing spidermonkey failures on WrappedFunctionObject.cpp. CLOSED TREE

Backed out for causing spidermonkey failures on WrappedFunctionObject.cpp.

Flags: needinfo?(andrebargull)
Flags: needinfo?(andrebargull)
Pushed by andre.bargull@gmail.com: https://hg.mozilla.org/integration/autoland/rev/81e98a6cf3cd Part 1: Add trailing comma for nicer clang-format output. r=mgaudet https://hg.mozilla.org/integration/autoland/rev/b396bff2aad9 Part 2: Remove unnecessary rooting. r=mgaudet https://hg.mozilla.org/integration/autoland/rev/356aa1ad2aa7 Part 3: Prefer internal API instead of the public JS API. r=mgaudet https://hg.mozilla.org/integration/autoland/rev/f448d1f36f8c Part 4: Clean-up CopyNameAndLength. r=mgaudet https://hg.mozilla.org/integration/autoland/rev/de23ee558eac Part 5: Fix a typo. r=mgaudet https://hg.mozilla.org/integration/autoland/rev/b4a1d3a92cd2 Part 6: Use an array to store extra handler values. r=mgaudet https://hg.mozilla.org/integration/autoland/rev/e375bc432e37 Part 7: Use NewBuiltinClassInstance instead of explicitly passing the prototype. r=mgaudet https://hg.mozilla.org/integration/autoland/rev/a900fb44f606 Part 8: Don't attempt to use the current name property for the toString output. r=mgaudet https://hg.mozilla.org/integration/autoland/rev/8083ae8e8e43 Part 9: Implement OrdinaryWrappedFunctionCall. r=mgaudet https://hg.mozilla.org/integration/autoland/rev/a581812a6162 Part 10: Correct compartment/realm handling in WrappedFunctionCreate. r=mgaudet https://hg.mozilla.org/integration/autoland/rev/9fc0e520ba91 Part 11: Use UnwrapAndTypeCheckValue to correctly unwrap non-CCW wrappers. r=mgaudet
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: