Bug 225397 – [JSC] Don't use JSScope instances as `this` value when calling functions

• Status: REOPENED
• Component: JavaScriptCore

[Alexey Shvayka]

[JSC] Abolish using resolved scope as |this| value when calling functions

[Alexey Shvayka]

Created attachment 427777 [details]
Patch

[Alexey Shvayka]

Created attachment 427784 [details]
Patch

Add missing prediction for GetThisValueForCall and call operationToObject with JSValueRegs.

[Alexey Shvayka]

Created attachment 427844 [details]
Patch

Call getters / setters with JSProxy as |this| value, fix SpeculativeJIT::compileGetThisValueForCall(), fix EvalFunctionCallNode, fix 32-bit builds, remove JSObject::isEnvironment().

[Radar WebKit Bug Importer]

<rdar://problem/77915872>

[Alexey Shvayka]

Created attachment 429549 [details]
Patch

Remove loadVariable() from 32-bit to_this LLInt op to fix segfaults, rebase on top of r277830, and adjust some LayoutTests.

[Alexey Shvayka]

Created attachment 429630 [details]
Patch

Remove incorrect exception check after toGlobalThis(), fix JSErrorHandler to pass JSProxy as |this| value, and adjust more LayoutTests.

[Alexey Shvayka]

Created attachment 429869 [details]
Patch

Adjust flaky test, tweak JS API avoid any behavior change (+ add coverage), and fix multiple cases of WebCore using raw GlobalObject as |this| value for [[Call]] (+ add ASSERTs for that).

[Alexey Shvayka]

Created attachment 429873 [details]
Patch

Fix build and rename JSScope::receiverForScope() to JSObject::toPublicThisValue().

[Alexey Shvayka]

Created attachment 429991 [details]
Patch

Test [PutForwards] WebIDL attribute on global interface, turn JSObject::toPublicThisValue() into a function to fix --debug buil, and utilize it to fix plugins & SparseArrayEntry::put() (+ improve coverage).

[Alexey Shvayka]

Created attachment 429993 [details]
Patch

Rebase patch and use std::nullopt to fix the build.

[Filip Pizlo]

Comment on attachment 429993 [details]
Patch

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

> Source/JavaScriptCore/ChangeLog:8
> +        Reviewed by NOBODY (OOPS!).
> +
> +        * API/APICallbackFunction.h:

Can you write some stuff about what you did?

> Source/JavaScriptCore/dfg/DFGSpeculativeJIT.cpp:13497
> +    addSlowPathGenerator(slowPathCall(slowCases, this, operationToObject, valueGPR, TrustedImmPtr::weakPointer(m_graph, globalObject), valueRegs, TrustedImmPtr(nullptr)));

What are some example programs where the slow path would be taken?

> Source/JavaScriptCore/jit/JITOpcodes.cpp:2097
> +void JIT::emit_op_get_this_value_for_call(const Instruction* currentInstruction)
> +{
> +    auto bytecode = currentInstruction->as<OpGetThisValueForCall>();
> +#if USE(JSVALUE64)
> +    JSValueRegs valueRegs(regT0);
> +#else
> +    JSValueRegs valueRegs(regT1, regT0);
> +#endif
> +    emitGetVirtualRegister(bytecode.m_scope, valueRegs);
> +
> +    Jump isWithScope = branchIfNotType(valueRegs.payloadGPR(), JSTypeRange { FirstJSScopeType, LastJSScopeType });
> +    moveTrustedValue(jsUndefined(), valueRegs);
> +    isWithScope.link(this);
> +
> +    emitPutVirtualRegister(bytecode.m_dst, valueRegs);
> +}

I really like this.

[Alexey Shvayka]

(In reply to Filip Pizlo from comment #11)
> Comment on attachment 429993 [details]
> Patch

Thank you for reviewing so quickly, Filip! And sorry for missing ChangeLog.

> > Source/JavaScriptCore/dfg/DFGSpeculativeJIT.cpp:13497
> > +    addSlowPathGenerator(slowPathCall(slowCases, this, operationToObject, valueGPR, TrustedImmPtr::weakPointer(m_graph, globalObject), valueRegs, TrustedImmPtr(nullptr)));
> 
> What are some example programs where the slow path would be taken?

Sloppy mode functions called with |this| value of a primitive string/number/boolean/bigint, which requires allocation: `(function() { return this; }.call(42);`.

[Ahmad Saleem]

I checked and it seems this r+ patch didn't landed yet and also this FIXME is still present:

https://github.com/WebKit/WebKit/blob/6bb75cf119f4cf3c077ec234af476fb575b28509/Source/WebCore/bindings/js/JSDOMOperation.h#L38

Is something pending or this is no longer required? Thanks!

[Alexey Shvayka]

Pull request: https://github.com/WebKit/WebKit/pull/31572

[EWS]

Committed 284931@main (ee167b8fe4fd): <https://commits.webkit.org/284931@main>

Reviewed commits have been landed. Closing PR #31572 and removing active labels.

[Fujii Hironori]

Re-opening for pull request https://github.com/WebKit/WebKit/pull/34946

[Fujii Hironori]

The test is failing.
https://results.webkit.org/?suite=layout-tests&test=imported%2Fw3c%2Fweb-platform-tests%2FWebIDL%2Fecmascript-binding%2Fglobal-object-implicit-this-value.any.serviceworker.html&flavor=wk2

[EWS]

Test gardening commit 284940@main (b692c5aeee32): <https://commits.webkit.org/284940@main>

Reviewed commits have been landed. Closing PR #34946 and removing active labels.

[WebKit Commit Bot]

Re-opened since this is blocked by bug 281214

[Fady Farag]

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

[Alexey Shvayka]

Pull request: https://github.com/WebKit/WebKit/pull/35527