Bug 115542 – Rationalized 'this' value conversion

• Component: New Bugs
• Status: RESOLVED FIXED

[Geoffrey Garen]

Rationalized 'this' value conversion

[Geoffrey Garen]

Created attachment 200385 [details]
Patch

[Early Warning System Bot]

Comment on attachment 200385 [details]
Patch

Attachment 200385 [details] did not pass qt-ews (qt):
Output: http://webkit-queues.appspot.com/results/391318

[EFL EWS Bot]

Comment on attachment 200385 [details]
Patch

Attachment 200385 [details] did not pass efl-wk2-ews (efl-wk2):
Output: http://webkit-queues.appspot.com/results/290326

[EFL EWS Bot]

Comment on attachment 200385 [details]
Patch

Attachment 200385 [details] did not pass efl-ews (efl):
Output: http://webkit-queues.appspot.com/results/343309

[Early Warning System Bot]

Comment on attachment 200385 [details]
Patch

Attachment 200385 [details] did not pass qt-wk2-ews (qt-wk2):
Output: http://webkit-queues.appspot.com/results/343310

[Filip Pizlo]

Comment on attachment 200385 [details]
Patch

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

Do you want to land on trunk?  Or on the branch?  R=me either way, but I sort of prefer the branch.

> Source/JavaScriptCore/dfg/DFGFixupPhase.cpp:639
> +            ECMAMode ecmaMode = m_graph.executableFor(node->codeOrigin)->isStrictMode() ? StrictMode : NotStrictMode;

Oh thank god for the ECMAMode enum.

> Source/JavaScriptCore/llint/LLIntData.cpp:107
> +    ASSERT(FinalObjectType == 18);

Is this just to have a fast "am I already suitable for this" check?

I buy that it's probably a sensible way to do it, but I do wonder why not have a type flag instead.

(I'm not objecting.  I'm just curious what your thinking was.)

[Build Bot]

Comment on attachment 200385 [details]
Patch

Attachment 200385 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/290325

[Build Bot]

Comment on attachment 200385 [details]
Patch

Attachment 200385 [details] did not pass win-ews (win):
Output: http://webkit-queues.appspot.com/results/391324

[Build Bot]

Comment on attachment 200385 [details]
Patch

Attachment 200385 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/392286

[Geoffrey Garen]

> Do you want to land on trunk?  Or on the branch?  R=me either way, but I sort of prefer the branch.

Yeah, branch.

> > Source/JavaScriptCore/llint/LLIntData.cpp:107
> > +    ASSERT(FinalObjectType == 18);
> 
> Is this just to have a fast "am I already suitable for this" check?

Yes.

> I buy that it's probably a sensible way to do it, but I do wonder why not have a type flag instead.
> 
> (I'm not objecting.  I'm just curious what your thinking was.)

Thinking about this now, I think a flag would be fine too, and a bit broader -- checking FinalObject was just easier.

[kov's GTK+ EWS bot]

Comment on attachment 200385 [details]
Patch

Attachment 200385 [details] did not pass gtk-ews (gtk):
Output: http://webkit-queues.appspot.com/results/360367

[Oliver Hunt]

Comment on attachment 200385 [details]
Patch

I would make replace isStrictMode() with ecmaMode() everywhere and use the new ECMAMode enum -- also maybe use the enum class thingy?  I think that may mitigate accidental enum->bool conversion, though i'm not 100% sure.

[Geoffrey Garen]

Committed r149527: <http://trac.webkit.org/changeset/149527>

[Geoffrey Garen]

Reopening to attach new patch.

[Geoffrey Garen]

Created attachment 200551 [details]
Patch

[WebKit Commit Bot]

Attachment 200551 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/fast/js/Object-defineProperty-expected.txt', u'LayoutTests/fast/js/script-tests/primitive-property-access-edge-cases.js', u'LayoutTests/sputnik/Conformance/11_Expressions/11.1_Primary_Expressions/11.1.1_The_this_Keyword/S11.1.1_A2-expected.txt', u'LayoutTests/sputnik/Conformance/15_Native_Objects/15.5_String/15.5.4/15.5.4.10_String.prototype.match/S15.5.4.10_A1_T3-expected.txt', u'LayoutTests/sputnik/Conformance/15_Native_Objects/15.5_String/15.5.4/15.5.4.11_String.prototype.replace/S15.5.4.11_A1_T3-expected.txt', u'LayoutTests/sputnik/Conformance/15_Native_Objects/15.5_String/15.5.4/15.5.4.12_String.prototype.search/S15.5.4.12_A1_T3-expected.txt', u'LayoutTests/sputnik/Conformance/15_Native_Objects/15.5_String/15.5.4/15.5.4.13_String.prototype.slice/S15.5.4.13_A1_T3-expected.txt', u'LayoutTests/sputnik/Conformance/15_Native_Objects/15.5_String/15.5.4/15.5.4.14_String.prototype.split/S15.5.4.14_A1_T3-expected.txt', u'LayoutTests/sputnik/Conformance/15_Native_Objects/15.5_String/15.5.4/15.5.4.15_String.prototype.substring/S15.5.4.15_A1_T3-expected.txt', u'LayoutTests/sputnik/Conformance/15_Native_Objects/15.5_String/15.5.4/15.5.4.6_String.prototype.concat/S15.5.4.6_A1_T3-expected.txt', u'LayoutTests/sputnik/Conformance/15_Native_Objects/15.5_String/15.5.4/15.5.4.7_String.prototype.indexOf/S15.5.4.7_A1_T3-expected.txt', u'LayoutTests/sputnik/Conformance/15_Native_Objects/15.5_String/15.5.4/15.5.4.8_String.prototype.lastIndexOf/S15.5.4.8_A1_T3-expected.txt', u'Source/JavaScriptCore/API/JSCallbackFunction.cpp', u'Source/JavaScriptCore/API/JSCallbackObjectFunctions.h', u'Source/JavaScriptCore/API/JSContextRef.cpp', u'Source/JavaScriptCore/API/JSObjectRef.cpp', u'Source/JavaScriptCore/ChangeLog', u'Source/JavaScriptCore/JavaScriptCore.order', u'Source/JavaScriptCore/JavaScriptCore.vcproj/JavaScriptCore/JavaScriptCoreExports.def', u'Source/JavaScriptCore/JavaScriptCore.vcxproj/JavaScriptCoreExportGenerator/JavaScriptCoreExports.def.in', u'Source/JavaScriptCore/bytecode/CodeBlock.cpp', u'Source/JavaScriptCore/bytecode/CodeOrigin.h', u'Source/JavaScriptCore/bytecode/Opcode.h', u'Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp', u'Source/JavaScriptCore/dfg/DFGAbstractState.cpp', u'Source/JavaScriptCore/dfg/DFGByteCodeParser.cpp', u'Source/JavaScriptCore/dfg/DFGCapabilities.h', u'Source/JavaScriptCore/dfg/DFGFixupPhase.cpp', u'Source/JavaScriptCore/dfg/DFGGraph.h', u'Source/JavaScriptCore/dfg/DFGNodeType.h', u'Source/JavaScriptCore/dfg/DFGOperations.cpp', u'Source/JavaScriptCore/dfg/DFGOperations.h', u'Source/JavaScriptCore/dfg/DFGPredictionPropagationPhase.cpp', u'Source/JavaScriptCore/dfg/DFGSpeculativeJIT32_64.cpp', u'Source/JavaScriptCore/dfg/DFGSpeculativeJIT64.cpp', u'Source/JavaScriptCore/interpreter/Interpreter.cpp', u'Source/JavaScriptCore/interpreter/Interpreter.h', u'Source/JavaScriptCore/jit/JIT.cpp', u'Source/JavaScriptCore/jit/JIT.h', u'Source/JavaScriptCore/jit/JITOpcodes.cpp', u'Source/JavaScriptCore/jit/JITOpcodes32_64.cpp', u'Source/JavaScriptCore/jit/JITStubs.cpp', u'Source/JavaScriptCore/jit/JITStubs.h', u'Source/JavaScriptCore/llint/LLIntData.cpp', u'Source/JavaScriptCore/llint/LLIntSlowPaths.cpp', u'Source/JavaScriptCore/llint/LLIntSlowPaths.h', u'Source/JavaScriptCore/llint/LowLevelInterpreter.asm', u'Source/JavaScriptCore/llint/LowLevelInterpreter32_64.asm', u'Source/JavaScriptCore/llint/LowLevelInterpreter64.asm', u'Source/JavaScriptCore/profiler/ProfileGenerator.cpp', u'Source/JavaScriptCore/runtime/CallData.cpp', u'Source/JavaScriptCore/runtime/ClassInfo.h', u'Source/JavaScriptCore/runtime/Completion.cpp', u'Source/JavaScriptCore/runtime/DatePrototype.cpp', u'Source/JavaScriptCore/runtime/GetterSetter.cpp', u'Source/JavaScriptCore/runtime/GetterSetter.h', u'Source/JavaScriptCore/runtime/JSActivation.cpp', u'Source/JavaScriptCore/runtime/JSActivation.h', u'Source/JavaScriptCore/runtime/JSCJSValue.cpp', u'Source/JavaScriptCore/runtime/JSCJSValue.h', u'Source/JavaScriptCore/runtime/JSCJSValueInlines.h', u'Source/JavaScriptCore/runtime/JSCell.cpp', u'Source/JavaScriptCore/runtime/JSCell.h', u'Source/JavaScriptCore/runtime/JSGlobalObject.cpp', u'Source/JavaScriptCore/runtime/JSGlobalObject.h', u'Source/JavaScriptCore/runtime/JSGlobalObjectFunctions.cpp', u'Source/JavaScriptCore/runtime/JSNameScope.cpp', u'Source/JavaScriptCore/runtime/JSNameScope.h', u'Source/JavaScriptCore/runtime/JSObject.cpp', u'Source/JavaScriptCore/runtime/JSObject.h', u'Source/JavaScriptCore/runtime/JSScope.cpp', u'Source/JavaScriptCore/runtime/JSString.cpp', u'Source/JavaScriptCore/runtime/JSString.h', u'Source/JavaScriptCore/runtime/PropertySlot.cpp', u'Source/JavaScriptCore/runtime/PropertySlot.h', u'Source/JavaScriptCore/runtime/SparseArrayValueMap.cpp', u'Source/JavaScriptCore/runtime/StrictEvalActivation.cpp', u'Source/JavaScriptCore/runtime/StrictEvalActivation.h', u'Source/WebCore/ChangeLog', u'Source/WebCore/WebCore.order', u'Source/WebCore/bindings/js/JSErrorHandler.cpp', u'Source/WebCore/bindings/js/JSInjectedScriptHostCustom.cpp', u'Source/WebCore/bindings/js/JSMainThreadExecState.h', u'Source/WebCore/bindings/scripts/CodeGeneratorJS.pm', u'Source/WebCore/bridge/NP_jsobject.cpp', u'Source/WebKit/mac/ChangeLog', u'Source/WebKit/mac/Plugins/Hosted/NetscapePluginInstanceProxy.mm', u'Source/WebKit2/ChangeLog', u'Source/WebKit2/WebProcess/Plugins/Netscape/NPJSObject.cpp']" exit_code: 1
Source/JavaScriptCore/runtime/GetterSetter.h:83:  The parameter name "value" adds no information, so it should be removed.  [readability/parameter_name] [5]
Total errors found: 1 in 82 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Geoffrey Garen]

This patch fixes a crash seen in a cyclic prototype regression test. It adds a non-trivial amount of changes to getters and setters, so it probably warrants a re-review.

[Build Bot]

Comment on attachment 200551 [details]
Patch

Attachment 200551 [details] did not pass win-ews (win):
Output: http://webkit-queues.appspot.com/results/406447

[Oliver Hunt]

Comment on attachment 200551 [details]
Patch

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

> Source/JavaScriptCore/runtime/GetterSetter.cpp:61
> +
> +    // Unlike other function calls, getter calls are specified to ToObject their base
> +    // at the call site.
> +    JSValue thisValue = base.toThis(exec, NotStrictMode);

Really?  Seems fairly dumb behavior to me :-/

> Source/JavaScriptCore/runtime/JSActivation.cpp:235
> +JSValue JSActivation::toThis(JSCell*, ExecState* exec, ECMAMode ecmaMode)
>  {
> +    if (ecmaMode == StrictMode)
> +        return jsUndefined();
>      return exec->globalThisValue();

May be faster to simply have this use this->globalObject() ?

> Source/JavaScriptCore/runtime/JSCJSValue.cpp:88
> +    if (ecmaMode == StrictMode)
> +        return *this;

What if we're the global object?  we should be returning the proxy - how is this handled?

[Geoffrey Garen]

> Really?  Seems fairly dumb behavior to me :-/

As you pointed out in my office, I read the spec wrong -- the spec mentions ToObject(base), but then says that base, and not ToObject(base), is passed to the getter/setter.

It appears that Firefox's behavior here is a bug.

> > Source/JavaScriptCore/runtime/JSActivation.cpp:235
> > +JSValue JSActivation::toThis(JSCell*, ExecState* exec, ECMAMode ecmaMode)
> >  {
> > +    if (ecmaMode == StrictMode)
> > +        return jsUndefined();
> >      return exec->globalThisValue();
> 
> May be faster to simply have this use this->globalObject() ?

"this->globalObject()" is no good because it's not the window shell. You probably meant "this->globalThis()".

But I don't think that would be correct, either. If you have...

Window A:
    function f() { return this; }

Window B:
    (function(f) {
        return (function() {
            return f();
        })();
    })(A.f);

... you should see A.window and not B.window, even though "f" was resolved to an activation in B. In other words, 'this' conversion is always performed in the lexical scope of the callee.

> > Source/JavaScriptCore/runtime/JSCJSValue.cpp:88
> > +    if (ecmaMode == StrictMode)
> > +        return *this;
> 
> What if we're the global object?  we should be returning the proxy - how is this handled?

It may not be clear from diff context, but this is JSValue::toThisSlowCase(), which only handles non-cells (and it ASSERTs as much).

[Geoffrey Garen]

Committed <http://trac.webkit.org/changeset/149636>.

[Alexey Proskuryakov]

This broke a regression test, filed bug 119271.

[Filip Pizlo]

This totally breaks the DFG's handling of this.

[Filip Pizlo]

Simple test program:

var myFunction = function (arg1) { return [this, "myFunction", arg1] };

for (var i = 0; i < 1000; ++i) {
    var array = myFunction("arg");
    describe(array[0]);
}

Notice that after DFG tier-up we start returning something different than what we returned before.

[Filip Pizlo]

Comment on attachment 200551 [details]
Patch

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

> Source/JavaScriptCore/dfg/DFGFixupPhase.cpp:657
> -            if (isObjectSpeculation(node->child1()->prediction())) {
> +            if (isFinalObjectSpeculation(node->child1()->prediction())) {
>                  setUseKindAndUnboxIfProfitable<ObjectUse>(node->child1());

This is really bad.  This changes the prediction required to use this optimization, but then the check still accepts any object.  So if you had a program that passed FinalObject as this for  while and this kicked in, but then you started passing some weird proxied thing, then all of a sudden you'd start getting bizarre behavior.

[Filip Pizlo]

There are two really hard bugs introduced by this change:

- The DFG's optimization to turn ToThis into a structure check just doesn't work anymore.  Well, the DFG keeps doing it, and then you get totally wrong code.  The DFG can't know based on looking at the Structure whether toThis() will behave normally nor weirdly.  I mean, it could try to compare the toThis entry in the methodTable to something - but then it would still not be able to do any better than a full-blown virtual call in the case that you used 'this' inside of a function that is called in global this context!  Anyway, that's just speculation about the performance regression we'd see if we made the behavior correct - right now the behavior is just wrong.

- The DFG's optimization to use object checks to see if ToThis passes its argument through still does a generic object check rather than a "are you an object and do you not have weird toThis behavior" check, which would be required for correctness.  Even worse, this optimization is guarded by a prediction check that checks if 'this' happened to be a final object according to profiling.  So this is a bug that manifests only when programs flip-flop their behavior.

[Filip Pizlo]

Correctness fixed in http://trac.webkit.org/changeset/155149.  Filed https://bugs.webkit.org/show_bug.cgi?id=120796 for the performance bug.