Bug 115542 - Rationalized 'this' value conversion
Summary: Rationalized 'this' value conversion
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Geoffrey Garen
URL:
Keywords:
Depends on: 120781
Blocks:
  Show dependency treegraph
 
Reported: 2013-05-02 21:30 PDT by Geoffrey Garen
Modified: 2013-09-05 14:20 PDT (History)
10 users (show)

See Also:


Attachments
Patch (82.45 KB, patch)
2013-05-02 21:50 PDT, Geoffrey Garen
no flags Details | Formatted Diff | Diff
Patch (104.98 KB, patch)
2013-05-04 17:16 PDT, Geoffrey Garen
oliver: review+
buildbot: commit-queue-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Geoffrey Garen 2013-05-02 21:30:15 PDT
Rationalized 'this' value conversion
Comment 1 Geoffrey Garen 2013-05-02 21:50:39 PDT
Created attachment 200385 [details]
Patch
Comment 2 Early Warning System Bot 2013-05-02 21:59:31 PDT
Comment on attachment 200385 [details]
Patch

Attachment 200385 [details] did not pass qt-ews (qt):
Output: http://webkit-queues.appspot.com/results/391318
Comment 3 EFL EWS Bot 2013-05-02 22:01:56 PDT
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
Comment 4 EFL EWS Bot 2013-05-02 22:02:27 PDT
Comment on attachment 200385 [details]
Patch

Attachment 200385 [details] did not pass efl-ews (efl):
Output: http://webkit-queues.appspot.com/results/343309
Comment 5 Early Warning System Bot 2013-05-02 22:02:44 PDT
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
Comment 6 Filip Pizlo 2013-05-02 22:04:00 PDT
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.)
Comment 7 Build Bot 2013-05-02 22:12:32 PDT
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
Comment 8 Build Bot 2013-05-02 22:22:33 PDT
Comment on attachment 200385 [details]
Patch

Attachment 200385 [details] did not pass win-ews (win):
Output: http://webkit-queues.appspot.com/results/391324
Comment 9 Build Bot 2013-05-02 22:30:57 PDT
Comment on attachment 200385 [details]
Patch

Attachment 200385 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/392286
Comment 10 Geoffrey Garen 2013-05-02 23:03:34 PDT
> 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.
Comment 11 kov's GTK+ EWS bot 2013-05-02 23:17:37 PDT
Comment on attachment 200385 [details]
Patch

Attachment 200385 [details] did not pass gtk-ews (gtk):
Output: http://webkit-queues.appspot.com/results/360367
Comment 12 Oliver Hunt 2013-05-03 09:48:09 PDT
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.
Comment 13 Geoffrey Garen 2013-05-03 11:45:48 PDT
Committed r149527: <http://trac.webkit.org/changeset/149527>
Comment 14 Geoffrey Garen 2013-05-04 17:16:02 PDT
Reopening to attach new patch.
Comment 15 Geoffrey Garen 2013-05-04 17:16:05 PDT
Created attachment 200551 [details]
Patch
Comment 16 WebKit Commit Bot 2013-05-04 17:17:03 PDT
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.
Comment 17 Geoffrey Garen 2013-05-04 17:17:26 PDT
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.
Comment 18 Build Bot 2013-05-06 02:18:12 PDT
Comment on attachment 200551 [details]
Patch

Attachment 200551 [details] did not pass win-ews (win):
Output: http://webkit-queues.appspot.com/results/406447
Comment 19 Oliver Hunt 2013-05-06 10:18:18 PDT
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?
Comment 20 Geoffrey Garen 2013-05-06 11:27:54 PDT
> 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).
Comment 21 Geoffrey Garen 2013-05-06 13:47:37 PDT
Committed <http://trac.webkit.org/changeset/149636>.
Comment 22 Alexey Proskuryakov 2013-07-30 13:19:36 PDT
This broke a regression test, filed bug 119271.
Comment 23 Filip Pizlo 2013-09-05 10:12:24 PDT
This totally breaks the DFG's handling of this.
Comment 24 Filip Pizlo 2013-09-05 10:12:53 PDT
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.
Comment 25 Filip Pizlo 2013-09-05 10:21:18 PDT
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.
Comment 26 Filip Pizlo 2013-09-05 10:23:58 PDT
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.
Comment 27 Filip Pizlo 2013-09-05 14:20:10 PDT
Correctness fixed in http://trac.webkit.org/changeset/155149.  Filed https://bugs.webkit.org/show_bug.cgi?id=120796 for the performance bug.