InternalFunction::createSubclassStructure should use newTarget's globalObject

• Component: JavaScriptCore
• Status: RESOLVED FIXED

[Alexey Shvayka]

Test case:
  var iframe = document.createElement("iframe")
  document.body.appendChild(iframe)
  var other = iframe.contentWindow
  var newTarget = new other.Function()
  newTarget.prototype = null
  var map = Reflect.construct(Map, [], newTarget)
  Object.getPrototypeOf(map) // ?

Expected:
  other.Map.prototype

Actual:
  Map.prototype

ECMA262:
  Step 2 of https://tc39.es/ecma262/#sec-map-iterable
    Step 2 of https://tc39.es/ecma262/#sec-ordinarycreatefromconstructor (constructor is NewTarget)
      Step 4.b of https://tc39.es/ecma262/#sec-getprototypefromconstructor

Test262:
  Tests named "proto-from-ctor-realm*.js", for example: https://test262.report/browse/built-ins/Map/proto-from-ctor-realm.js

Both V8 and SpiderMonkey create subclasses with prototypes from correct realms.

[Alexey Shvayka]

Created attachment 380263 [details]
Patch

[Alexey Shvayka]

Created attachment 380266 [details]
Patch

Fix Function constructor invoked with [[Call]].

[Alexey Shvayka]

Created attachment 380319 [details]
Patch

Tweak IntlPluralRulesConstructor.

[Alexey Shvayka]

Created attachment 383050 [details]
Patch

Drop Intl* changes and rebase patch.

[Alexey Shvayka]

Created attachment 383052 [details]
Patch

Reword ChangeLog.

[EWS Watchlist]

Comment on attachment 383052 [details]
Patch

Attachment 383052 [details] did not pass jsc-ews (mac):
Output: https://webkit-queues.webkit.org/results/13223159

New failing tests:
apiTests

[Alexey Shvayka]

Created attachment 383123 [details]
Patch

Rebase patch and fix async functions constructors.

[EWS Watchlist]

Comment on attachment 383123 [details]
Patch

Attachment 383123 [details] did not pass jsc-ews (mac):
Output: https://webkit-queues.webkit.org/results/13226742

New failing tests:
apiTests

[Keith Miller]

Looks like this broke API tests. Can you look into it?

[Alexey Shvayka]

Created attachment 383186 [details]
Patch

Mark TypedArrayConstructors tests as passing and revert Promise changes.

[Alexey Shvayka]

Created attachment 383197 [details]
Patch

Apologies for all the emails, API tests fail for me even on trunk.

[EWS Watchlist]

Comment on attachment 383197 [details]
Patch

Attachment 383197 [details] did not pass jsc-ews (mac):
Output: https://webkit-queues.webkit.org/results/13228852

New failing tests:
apiTests

[Alexey Shvayka]

Created attachment 383262 [details]
Patch

Bring back Promise changes and revert Array changes.

[EWS Watchlist]

Comment on attachment 383262 [details]
Patch

Attachment 383262 [details] did not pass jsc-ews (mac):
Output: https://webkit-queues.webkit.org/results/13238831

New failing tests:
apiTests

[Alexey Shvayka]

Created attachment 383268 [details]
Patch

Revert both Array and Promise changes.

[EWS Watchlist]

Comment on attachment 383268 [details]
Patch

Attachment 383268 [details] did not pass jsc-ews (mac):
Output: https://webkit-queues.webkit.org/results/13239508

New failing tests:
apiTests

[Alexey Shvayka]

Created attachment 387394 [details]
Patch

Fix API tests and add ASSERTs.

[Yusuke Suzuki]

Comment on attachment 387394 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=387394&action=review

Nice. But I think this patch is not covering DFG and FTL e.g. operationCreatePromise in DFGOperations invoked by DFG::CreatePromise.

1. Add DFG / FTL handlings.
2. Can you clarify why JSGenerator / JSAsyncGenerator etc. are OK?
3. Why are Wasm constructors OK?
4. Why are Intl constructors OK?

> Source/JavaScriptCore/runtime/CommonSlowPaths.cpp:292
>      }

InternalFunction::createSubclassStructure is used for `slow_path_create_generator`, `slow_path_create_async_generator` etc. Why are they OK? Can you add clarification in ChangeLog?

[Alexey Shvayka]

Created attachment 396291 [details]
Patch

Cover operationCreatePromise (+ add test), WASM & Intl constructors, fix .prototype realm.

[Yusuke Suzuki]

I think CreateGenerator / CreateAsyncGenerator need to be supported too, is it correct?
Can you grep InternalFunction::createSubclassStructure to ensure that all the cases are supported?

[Alexey Shvayka]

(In reply to Yusuke Suzuki from comment #18)
> 1. Add DFG / FTL handlings.

Done (with a test).

> 2. Can you clarify why JSGenerator / JSAsyncGenerator etc. are OK?
> InternalFunction::createSubclassStructure is used for `slow_path_create_generator`, `slow_path_create_async_generator` etc. Why are they OK?

Done, please see ChangeLog.

> 3. Why are Wasm constructors OK?
> 4. Why are Intl constructors OK?

I was planning to cover those in a follow-up patch to simplify review process, but it seems like it would be better to fix all existing InternalFunction::createSubclassStructure() call sites with a single change.

5 of 8 WASM constructors are currently subclassable, all of them are fixed by updated patch. The remaining 3 would require a follow-up patch that adds InternalFunction::createSubclassStructure() with regard to correct realm.

Tests for all WASM constructors are coming in https://github.com/web-platform-tests/wpt/pull/22833. Those tests depend on "wasm-module-builder.js" harness which seems to be broken/outdated in WebKit trunk.

[Alexey Shvayka]

(In reply to Yusuke Suzuki from comment #20)
> I think CreateGenerator / CreateAsyncGenerator need to be supported too, is
> it correct?

They use active function's (GeneratorFunctionConstructor / AsyncGeneratorFunctionConstructor) realm according to steps 23-24 of https://tc39.es/ecma262/#sec-createdynamicfunction.

> Can you grep InternalFunction::createSubclassStructure to ensure that all
> the cases are supported?

We support all cases in Source/JavaScriptCore except for generators and internal promises. DOM constructors were fixed in https://trac.webkit.org/r256716, HTML custom elements upgrades are also correct.

[Yusuke Suzuki]

Comment on attachment 396291 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=396291&action=review

> Source/JavaScriptCore/dfg/DFGOperations.cpp:396
> +    JSValue newTarget = callFrame->newTarget();

This is not correct. CallFrame is per-DFG-CodeBlock. So if your inlined function has different realm to the root DFG CodeBlock, this returns wrong result. And if the root CodeBlock is not constructor call, accessing newTarget is not OK.
I think that this modification is not necessary: globalObject is pointing proper lexical global object to `op_create_promise` bytecode.
Can you ensure this?

[Yusuke Suzuki]

Comment on attachment 396291 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=396291&action=review

>> Source/JavaScriptCore/dfg/DFGOperations.cpp:396
>> +    JSValue newTarget = callFrame->newTarget();
> 
> This is not correct. CallFrame is per-DFG-CodeBlock. So if your inlined function has different realm to the root DFG CodeBlock, this returns wrong result. And if the root CodeBlock is not constructor call, accessing newTarget is not OK.
> I think that this modification is not necessary: globalObject is pointing proper lexical global object to `op_create_promise` bytecode.
> Can you ensure this?

Ah, maybe, some modification is required, but you cannot use `CallFrame::newTarget` here. I think you need to pass newTarget value to CreatePromise DFG node, and you need to modify DFG / FTL code too. Is my understanding correct?

[Alexey Shvayka]

(In reply to Yusuke Suzuki from comment #24)
> Ah, maybe, some modification is required, but you cannot use
> `CallFrame::newTarget` here. I think you need to pass newTarget value to
> CreatePromise DFG node, and you need to modify DFG / FTL code too. Is my
> understanding correct?

Yes, the stress test this change is adding was failing before DFGOperations.cpp modification.

NewTarget from another realm for Promise is an extremely rare case, maybe we should just fall back to slow path in compileCreatePromise()?

[Alexey Shvayka]

Created attachment 396323 [details]
Patch

Drop CallFrame::newTarget() usage in DFG operation.

[Yusuke Suzuki]

Comment on attachment 396323 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=396323&action=review

> Source/JavaScriptCore/dfg/DFGOperations.cpp:397
> +    Structure* baseStructure = constructor->globalObject(vm)->promiseStructure();
> +    Structure* structure = InternalFunction::createSubclassStructure(globalObject, globalObject->promiseConstructor(), constructor, baseStructure);

Oh, yeah, looks correct!

[Alexey Shvayka]

Created attachment 396653 [details]
Patch

Rebase patch on updated test262/expectations.yaml.

[Alexey Shvayka]

Created attachment 396784 [details]
Patch

Introduce getFunctionRealm().

[Alexey Shvayka]

*** Bug 205669 has been marked as a duplicate of this bug. ***

[Yusuke Suzuki]

Comment on attachment 396784 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=396784&action=review

r=me

> Source/JavaScriptCore/runtime/InternalFunction.cpp:157
> +    if (auto* boundFunction = jsDynamicCast<JSBoundFunction*>(vm, object))

Use `inherit<JSBoundFunction>` instead of jsDynamicCast. jsDynamicCast uses `LIKELY()` internally, which means that we are saying that this is likely a JSBoundFunction. But it is not.

> Source/JavaScriptCore/runtime/InternalFunction.cpp:160
> +    if (auto* proxy = jsDynamicCast<ProxyObject*>(vm, object)) {

Ditto.

[Alexey Shvayka]

Created attachment 397617 [details]
Patch

Drop jsDynamicCast, handle Intl.RelativeTimeFormat and mark more tests as passing.

[Alexey Shvayka]

(In reply to Yusuke Suzuki from comment #31)
> Use `inherit<JSBoundFunction>` instead of jsDynamicCast. jsDynamicCast uses
> `LIKELY()` internally, which means that we are saying that this is likely a
> JSBoundFunction. But it is not.

Thank you for taking time to review this: it is quite massive.
I've used type() == ProxyObjectType as it seems like a common way and feels a bit faster.

[EWS]

Committed r260732: <https://trac.webkit.org/changeset/260732>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 397617 [details].

[Radar WebKit Bug Importer]

<rdar://problem/62408975>