Bug 202599 - InternalFunction::createSubclassStructure should use newTarget's globalObject
Summary: InternalFunction::createSubclassStructure should use newTarget's globalObject
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: WebKit Nightly Build
Hardware: All All
: P2 Minor
Assignee: Alexey Shvayka
URL:
Keywords: InRadar
: 205669 (view as bug list)
Depends on:
Blocks:
 
Reported: 2019-10-04 13:15 PDT by Alexey Shvayka
Modified: 2020-04-26 14:31 PDT (History)
11 users (show)

See Also:


Attachments
Patch (48.90 KB, patch)
2019-10-04 16:11 PDT, Alexey Shvayka
no flags Details | Formatted Diff | Diff
Patch (48.92 KB, patch)
2019-10-04 16:42 PDT, Alexey Shvayka
no flags Details | Formatted Diff | Diff
Patch (48.92 KB, patch)
2019-10-07 06:24 PDT, Alexey Shvayka
no flags Details | Formatted Diff | Diff
Patch (39.77 KB, patch)
2019-11-07 08:12 PST, Alexey Shvayka
no flags Details | Formatted Diff | Diff
Patch (39.69 KB, patch)
2019-11-07 08:19 PST, Alexey Shvayka
no flags Details | Formatted Diff | Diff
Patch (41.97 KB, patch)
2019-11-08 07:00 PST, Alexey Shvayka
no flags Details | Formatted Diff | Diff
Patch (45.09 KB, patch)
2019-11-08 17:23 PST, Alexey Shvayka
no flags Details | Formatted Diff | Diff
Patch (44.16 KB, patch)
2019-11-08 19:19 PST, Alexey Shvayka
no flags Details | Formatted Diff | Diff
Patch (43.26 KB, patch)
2019-11-11 04:15 PST, Alexey Shvayka
no flags Details | Formatted Diff | Diff
Patch (41.49 KB, patch)
2019-11-11 07:20 PST, Alexey Shvayka
no flags Details | Formatted Diff | Diff
Patch (46.09 KB, patch)
2020-01-10 16:35 PST, Alexey Shvayka
no flags Details | Formatted Diff | Diff
Patch (72.75 KB, patch)
2020-04-13 09:02 PDT, Alexey Shvayka
no flags Details | Formatted Diff | Diff
Patch (72.66 KB, patch)
2020-04-13 13:19 PDT, Alexey Shvayka
no flags Details | Formatted Diff | Diff
Patch (72.37 KB, patch)
2020-04-16 08:48 PDT, Alexey Shvayka
no flags Details | Formatted Diff | Diff
Patch (91.77 KB, patch)
2020-04-17 13:00 PDT, Alexey Shvayka
no flags Details | Formatted Diff | Diff
Patch (117.75 KB, patch)
2020-04-26 05:09 PDT, Alexey Shvayka
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Alexey Shvayka 2019-10-04 13:15:57 PDT
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.
Comment 1 Alexey Shvayka 2019-10-04 16:11:37 PDT Comment hidden (obsolete)
Comment 2 Alexey Shvayka 2019-10-04 16:42:55 PDT Comment hidden (obsolete)
Comment 3 Alexey Shvayka 2019-10-07 06:24:00 PDT Comment hidden (obsolete)
Comment 4 Alexey Shvayka 2019-11-07 08:12:32 PST Comment hidden (obsolete)
Comment 5 Alexey Shvayka 2019-11-07 08:19:04 PST Comment hidden (obsolete)
Comment 6 EWS Watchlist 2019-11-07 10:37:09 PST Comment hidden (obsolete)
Comment 7 Alexey Shvayka 2019-11-08 07:00:59 PST Comment hidden (obsolete)
Comment 8 EWS Watchlist 2019-11-08 09:27:28 PST Comment hidden (obsolete)
Comment 9 Keith Miller 2019-11-08 10:09:11 PST
Looks like this broke API tests. Can you look into it?
Comment 10 Alexey Shvayka 2019-11-08 17:23:19 PST Comment hidden (obsolete)
Comment 11 Alexey Shvayka 2019-11-08 19:19:27 PST Comment hidden (obsolete)
Comment 12 EWS Watchlist 2019-11-08 22:41:20 PST Comment hidden (obsolete)
Comment 13 Alexey Shvayka 2019-11-11 04:15:41 PST Comment hidden (obsolete)
Comment 14 EWS Watchlist 2019-11-11 06:36:16 PST Comment hidden (obsolete)
Comment 15 Alexey Shvayka 2019-11-11 07:20:10 PST Comment hidden (obsolete)
Comment 16 EWS Watchlist 2019-11-11 10:57:08 PST Comment hidden (obsolete)
Comment 17 Alexey Shvayka 2020-01-10 16:35:29 PST
Created attachment 387394 [details]
Patch

Fix API tests and add ASSERTs.
Comment 18 Yusuke Suzuki 2020-01-10 17:28:45 PST
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?
Comment 19 Alexey Shvayka 2020-04-13 09:02:36 PDT
Created attachment 396291 [details]
Patch

Cover operationCreatePromise (+ add test), WASM & Intl constructors, fix .prototype realm.
Comment 20 Yusuke Suzuki 2020-04-13 09:05:49 PDT
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?
Comment 21 Alexey Shvayka 2020-04-13 09:12:18 PDT
(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.
Comment 22 Alexey Shvayka 2020-04-13 09:18:32 PDT
(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.
Comment 23 Yusuke Suzuki 2020-04-13 09:38:25 PDT
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?
Comment 24 Yusuke Suzuki 2020-04-13 09:40:42 PDT
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?
Comment 25 Alexey Shvayka 2020-04-13 09:52:41 PDT
(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()?
Comment 26 Alexey Shvayka 2020-04-13 13:19:57 PDT
Created attachment 396323 [details]
Patch

Drop CallFrame::newTarget() usage in DFG operation.
Comment 27 Yusuke Suzuki 2020-04-13 13:22:50 PDT
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!
Comment 28 Alexey Shvayka 2020-04-16 08:48:42 PDT
Created attachment 396653 [details]
Patch

Rebase patch on updated test262/expectations.yaml.
Comment 29 Alexey Shvayka 2020-04-17 13:00:17 PDT
Created attachment 396784 [details]
Patch

Introduce getFunctionRealm().
Comment 30 Alexey Shvayka 2020-04-17 13:42:21 PDT
*** Bug 205669 has been marked as a duplicate of this bug. ***
Comment 31 Yusuke Suzuki 2020-04-17 18:21:23 PDT
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.
Comment 32 Alexey Shvayka 2020-04-26 05:09:32 PDT
Created attachment 397617 [details]
Patch

Drop jsDynamicCast, handle Intl.RelativeTimeFormat and mark more tests as passing.
Comment 33 Alexey Shvayka 2020-04-26 13:45:00 PDT
(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.
Comment 34 EWS 2020-04-26 14:30:43 PDT
Committed r260732: <https://trac.webkit.org/changeset/260732>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 397617 [details].
Comment 35 Radar WebKit Bug Importer 2020-04-26 14:31:19 PDT
<rdar://problem/62408975>