Summary: | Rationalized 'this' value conversion | ||||||||
---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Geoffrey Garen <ggaren> | ||||||
Component: | New Bugs | Assignee: | Geoffrey Garen <ggaren> | ||||||
Status: | RESOLVED FIXED | ||||||||
Severity: | Normal | CC: | buildbot, commit-queue, eflews.bot, fpizlo, gtk-ews, gyuyoung.kim, rego+ews, rniwa, webkit-ews, xan.lopez | ||||||
Priority: | P2 | ||||||||
Version: | 528+ (Nightly build) | ||||||||
Hardware: | Unspecified | ||||||||
OS: | Unspecified | ||||||||
Bug Depends on: | 120781 | ||||||||
Bug Blocks: | |||||||||
Attachments: |
|
Description
Geoffrey Garen
2013-05-02 21:30:15 PDT
Created attachment 200385 [details]
Patch
Comment on attachment 200385 [details] Patch Attachment 200385 [details] did not pass qt-ews (qt): Output: http://webkit-queues.appspot.com/results/391318 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 on attachment 200385 [details] Patch Attachment 200385 [details] did not pass efl-ews (efl): Output: http://webkit-queues.appspot.com/results/343309 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 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 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 on attachment 200385 [details] Patch Attachment 200385 [details] did not pass win-ews (win): Output: http://webkit-queues.appspot.com/results/391324 Comment on attachment 200385 [details] Patch Attachment 200385 [details] did not pass mac-ews (mac): Output: http://webkit-queues.appspot.com/results/392286 > 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 on attachment 200385 [details] Patch Attachment 200385 [details] did not pass gtk-ews (gtk): Output: http://webkit-queues.appspot.com/results/360367 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.
Committed r149527: <http://trac.webkit.org/changeset/149527> Reopening to attach new patch. Created attachment 200551 [details]
Patch
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.
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 on attachment 200551 [details] Patch Attachment 200551 [details] did not pass win-ews (win): Output: http://webkit-queues.appspot.com/results/406447 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? > 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). Committed <http://trac.webkit.org/changeset/149636>. This broke a regression test, filed bug 119271. This totally breaks the DFG's handling of this. 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 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. 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. Correctness fixed in http://trac.webkit.org/changeset/155149. Filed https://bugs.webkit.org/show_bug.cgi?id=120796 for the performance bug. |