Bug 123642

Created attachment 215778 [details] Patch The changes to jit/JITStubsARM.h, jit/JITStubsARM64.h, jit/JITStubsMIPS.h, jit/JITStubsMSVC64.asm and jit/JITStubsSH4.h are untested.

Attachment 215778 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/JavaScriptCore/ChangeLog', u'Source/JavaScriptCore/bytecode/CodeBlock.cpp', u'Source/JavaScriptCore/debugger/DebuggerCallFrame.cpp', u'Source/JavaScriptCore/dfg/DFGJITCompiler.cpp', u'Source/JavaScriptCore/interpreter/CallFrame.h', u'Source/JavaScriptCore/interpreter/Interpreter.cpp', u'Source/JavaScriptCore/interpreter/Interpreter.h', u'Source/JavaScriptCore/interpreter/JSStackInlines.h', u'Source/JavaScriptCore/interpreter/Register.h', u'Source/JavaScriptCore/interpreter/StackVisitor.cpp', u'Source/JavaScriptCore/interpreter/VMInspector.cpp', u'Source/JavaScriptCore/jit/JIT.cpp', u'Source/JavaScriptCore/jit/JITOperations.cpp', u'Source/JavaScriptCore/jit/JITStubsARM.h', u'Source/JavaScriptCore/jit/JITStubsARM64.h', u'Source/JavaScriptCore/jit/JITStubsARMv7.h', u'Source/JavaScriptCore/jit/JITStubsMIPS.h', u'Source/JavaScriptCore/jit/JITStubsMSVC64.asm', u'Source/JavaScriptCore/jit/JITStubsSH4.h', u'Source/JavaScriptCore/jit/JITStubsX86.h', u'Source/JavaScriptCore/jit/JITStubsX86_64.h', u'Source/JavaScriptCore/jsc.cpp', u'Source/JavaScriptCore/llint/LowLevelInterpreter.cpp', u'Source/JavaScriptCore/runtime/VM.cpp', u'Source/WebCore/ChangeLog', u'Source/WebCore/bindings/js/ScriptController.cpp', u'Source/WebCore/bindings/js/ScriptDebugServer.cpp']" exit_code: 1 Source/JavaScriptCore/jit/JITStubsX86.h:262: Extra space before [ [whitespace/braces] [5] Total errors found: 1 in 25 files If any of these errors are false positives, please file a bug against check-webkit-style.

Comment on attachment 215778 [details] Patch Attachment 215778 [details] did not pass mac-ews (mac): Output: http://webkit-queues.appspot.com/results/20328032 New failing tests: plugins/refcount-leaks.html

Created attachment 215792 [details] Archive of layout-test-results from webkit-ews-05 for mac-mountainlion The attached test failures were seen while running run-webkit-tests on the mac-ews. Bot: webkit-ews-05 Port: mac-mountainlion Platform: Mac OS X 10.8.5

Comment on attachment 215778 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=215778&action=review r=me. Please consider my nit-picking suggestion on "savedTopCallFrame()", but I won't be heart broken if you choose not to adopt it. > Source/JavaScriptCore/ChangeLog:19 > + Replace the HostCallFrame bit anded to the CallerFrame value in a CallFrame with > + a dummy host CallFrame. Logically, the host call frame is pushed on the stack > + before the callee frame when calling from native to JavaScript code. The > + callee frame's CallerFrame points at the host call frame and the host call frame's > + CallerFrame points at the real caller. The host call frame has a sentinal (1) in > + the CodeBlock to indicate it is a host call frame. It's ScopeChain has > + vm.topCallFrame at the time of the call. This allows for a complete stack walk > + as well as walking just the contiguous JSC frames. > + > + The host call frame and callee frame are currently allocated and initialized in > + ExecState::init(), but this initialization will be moved to ctiTrampoline when > + we actually move onto the native stack. I wonder if it'd be better if we start calling this the "sentinel frame" instead of the "host call frame". That's because "host call frame" sounds too much like the native C frame that preceeds the JS frame (especially when we start mingling the frames on the native C stack later). Just a suggestion. > Source/JavaScriptCore/interpreter/CallFrame.h:286 > + void setHostCallFrame(CallFrame* callFrame) > + { > + setCallerFrame(noCaller()); > + setReturnPC(0); > + setCodeBlock(hostCodeBlock()); > + static_cast<Register*>(this)[JSStack::ScopeChain] = callFrame; Nit: I'd prefer that the callFrame argument be named topCallFrame since that is what it is expected to be. The reason this came to mind was because the first time I read this code, it made me wonder why you didn't just store the callerFrame in the callerFrame field. And then I realized that once we move to using the native C stack, the set value will not be the true caller frame, but is the saved value of the JS topCallFrame instead. This made me feel that it might be better to spell that out by renaming: 1. setHostCallFrame() ==> setSavedTopCallFrame() 2. the callFrame arg ==> topCallFrame Similarly, rename the hostCallerFrame() method above to savedTopCallFrame(). > Source/JavaScriptCore/interpreter/Interpreter.cpp:430 > + callFrame->vm().topCallFrame = callerFrame->hostCallerFrame(); Nit: I'm a bit uncomfortable with calling this "hostCallerFrame()". Why don't we just make it more explicit and call it "cachedTopCallFrame()" or "savedTopCallFrame()" instead? I think it'll read better and communicate the intent more clearly that way. > Source/JavaScriptCore/interpreter/JSStackInlines.h:95 > + newHostLinkCallframe->setHostCallFrame(callerFrame); Ditto nit: setHostCallFrame(callerFrame) ==> setSavedTopCallFrame(m_topCallFrame); > Source/JavaScriptCore/interpreter/JSStackInlines.h:126 > + // Pop off the callee frame and the host link frame. > + CallFrame* callerFrame = frame->callerFrame()->hostCallerFrame(); > > // Pop to the caller: > m_topCallFrame = callerFrame; Ditto nit. > Source/JavaScriptCore/runtime/VM.cpp:656 > CallFrame* callFrame; > - for (callFrame = exec; callFrame && !callFrame->codeBlock(); callFrame = callFrame->callerFrame()->removeHostCallFrameFlag()) > + for (callFrame = exec; callFrame && !callFrame->codeBlock(); ) { > stackIndex++; > + callFrame = callFrame->callerFrameOrHostCallerFrame(); > + } why not just change this into a while loop?

Comment on attachment 215778 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=215778&action=review >> Source/JavaScriptCore/interpreter/CallFrame.h:286 >> + void setHostCallFrame(CallFrame* callFrame) >> + { >> + setCallerFrame(noCaller()); >> + setReturnPC(0); >> + setCodeBlock(hostCodeBlock()); >> + static_cast<Register*>(this)[JSStack::ScopeChain] = callFrame; > > Nit: I'd prefer that the callFrame argument be named topCallFrame since that is what it is expected to be. > > The reason this came to mind was because the first time I read this code, it made me wonder why you didn't just store the callerFrame in the callerFrame field. And then I realized that once we move to using the native C stack, the set value will not be the true caller frame, but is the saved value of the JS topCallFrame instead. This made me feel that it might be better to spell that out by renaming: > > 1. setHostCallFrame() ==> setSavedTopCallFrame() > 2. the callFrame arg ==> topCallFrame > > Similarly, rename the hostCallerFrame() method above to savedTopCallFrame(). Sorry, I was unclear there and also wrote some of my feedback wrongly. What I meant to say is: The hostCallerFrame() method tells me that the callFrame arg in setHostCallFrame() is really a caller frame pointer. This made me wonder why you didn't just store it in the CallFrameHeader's callerFrame field. And then I remembered that the callerFrame field is reserved for the previous native C frame which is not necessarily the previous JS frame. But since what you really want to store there is the previous JS frame (which is the VM.topCallFrame at the moment setHostCallFrame() is called), I suggest calling it the savedTopCallFrame pointer instead. I had also suggested calling the "host call frame" a "sentinel frame" instead. I'll apply this change also below to illustrate what it looks like for your consideration. So, to be precise, this is what I'm suggesting: 1. Change hostCallerFrame() to savedTopCalledFrame(): CallFrame* savedTopCallFrame() const { ASSERT(isSentinelFrame()); return this[JSStack::ScopeChain].callFrame(); } 2. Change setHostCallFrame() to initializeSentinelFrame() as follows: void initializeSentinelFrame(CallFrame* topCallFrame) { ... static_cast<Register*>(this)[JSStack::ScopeChain] = topCallFrame; ... } 3. Change isHostCallFrame() to isSentinelFrame(). 4. Change the code at line 430 which restores topCallFrame as follows: if (callerFrame->isSentinelFrame()) { callFrame->vm().topCallFrame = callerFrame->savedTopCallFrame(); return false; } 5. In JSStack::pushFrame(), change newHostLinkFrame to sentinelFrame: CallFrame* sentinelframe = CallFrame::create(newCallFrameSlot + paddedArgsCount + JSStack::CallFrameHeaderSize); ... sentinelFrame->initSentinelFrame(m_topCallFrame); ... newCallFrame->init(codeBlock, 0, scope, sentinelFrame, argsCount, callee); 6. In JSStack::popFrame(), change the restoration of topCallFrame to: m_topCallFrame = frame->callerFrame()->savedTopCallFrame(); CallFrame* callerFrame = m_topCallFrame; Again, my reasoning for the above suggestion is: 1. I find the term "host call frame" confusing as it suggests the native C stack frame that calls into JS (which is how we have always used that term before). This new sentinel frame is a new construct. I suggest we make it explicit and call it a SentinelFrame so as to not confuse it with the host call frame which is the native C frame that called into JS. 2. I find calling the value stored in the sentinel frame's ScopeChain field the "hostCallerFrame" confusing because: a. It is not a native / host caller frame. It is a JS caller frame of the current JS method. b. It is not store in the callerFrame field of the sentinel frame, but in an overloaded special location i.e. the ScopeChain slot. Since that value is also the VM.topCallFrame at that moment the sentinel frame is initialized, why not call it the savedTopCallFrame pointer. I think framing it this way makes the code clearer. After thinking this through, I feel more strongly that the code could be made clearer and improved. So, I'll take back my r+ for now. What do you think?

> 1. I find the term "host call frame" confusing as it suggests the native C stack frame that calls into JS (which is how we have always used that term before). I agree. Good catch. Michael and I discussed this in person, and came up with these name proposals: isVMEntrySentinel() vmEntrySentinelCallerFrame() callerFrameSkippingVMEntrySentinel() I also think we should remove setHostCallFrame(), and replace it with CallFrame::createVMEntrySentinel(). I also think that all accessor functions other than vmEntrySentinelCallerFrame() should ASSERT(!isVMEntrySentinel()). Does that sounds reasonable to you, Mark?

(In reply to comment #7) > I also think we should remove setHostCallFrame(), and replace it with CallFrame::createVMEntrySentinel(). This is only used in JSStack::pushFrame() where the memory for the sentinel was allocated before hand. This function is supposed to initialize that memory, which is why I suggested naming it "initialize...()". The name "create...()" often suggests to me that it allocates memory as well. That said, I don't think the difference is such a big deal. I can see how calling it "create...()" can work. > I also think that all accessor functions other than vmEntrySentinelCallerFrame() should ASSERT(!isVMEntrySentinel()). Good idea. > Does that sounds reasonable to you, Mark? Sounds good to me so far.

Created attachment 215942 [details] Updated patch (In reply to comment #7) > I also think that all accessor functions other than vmEntrySentinelCallerFrame() should ASSERT(!isVMEntrySentinel()). I tried adding the ASSERT() to the various accessors and it resulted in many crashes. I appears that we don't have an order that we initialize fields in the header. Before the CodeBlock is set up for a normal frame, we could have a random 1 value. All other suggested changes were made.

Attachment 215942 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/JavaScriptCore/ChangeLog', u'Source/JavaScriptCore/bytecode/CodeBlock.cpp', u'Source/JavaScriptCore/debugger/DebuggerCallFrame.cpp', u'Source/JavaScriptCore/dfg/DFGJITCompiler.cpp', u'Source/JavaScriptCore/interpreter/CallFrame.h', u'Source/JavaScriptCore/interpreter/Interpreter.cpp', u'Source/JavaScriptCore/interpreter/Interpreter.h', u'Source/JavaScriptCore/interpreter/JSStack.cpp', u'Source/JavaScriptCore/interpreter/JSStackInlines.h', u'Source/JavaScriptCore/interpreter/Register.h', u'Source/JavaScriptCore/interpreter/StackVisitor.cpp', u'Source/JavaScriptCore/interpreter/VMInspector.cpp', u'Source/JavaScriptCore/jit/JIT.cpp', u'Source/JavaScriptCore/jit/JITOperations.cpp', u'Source/JavaScriptCore/jit/JITStubsARM.h', u'Source/JavaScriptCore/jit/JITStubsARM64.h', u'Source/JavaScriptCore/jit/JITStubsARMv7.h', u'Source/JavaScriptCore/jit/JITStubsMIPS.h', u'Source/JavaScriptCore/jit/JITStubsMSVC64.asm', u'Source/JavaScriptCore/jit/JITStubsSH4.h', u'Source/JavaScriptCore/jit/JITStubsX86.h', u'Source/JavaScriptCore/jit/JITStubsX86_64.h', u'Source/JavaScriptCore/jsc.cpp', u'Source/JavaScriptCore/llint/LowLevelInterpreter.cpp', u'Source/JavaScriptCore/runtime/VM.cpp', u'Source/WebCore/ChangeLog', u'Source/WebCore/bindings/js/ScriptController.cpp', u'Source/WebCore/bindings/js/ScriptDebugServer.cpp']" exit_code: 1 Source/JavaScriptCore/jit/JITStubsX86.h:262: Extra space before [ [whitespace/braces] [5] Total errors found: 1 in 26 files If any of these errors are false positives, please file a bug against check-webkit-style.

Comment on attachment 215942 [details] Updated patch View in context: https://bugs.webkit.org/attachment.cgi?id=215942&action=review r=me > Source/JavaScriptCore/ChangeLog:14 > + it is a VM entry sentinal call frame. It's ScopeChain has vm.topCallFrame at the Should be "Its".

Comment on attachment 215942 [details] Updated patch View in context: https://bugs.webkit.org/attachment.cgi?id=215942&action=review I'd prefer if you make these edits before landing. Thanks. > Source/JavaScriptCore/ChangeLog:8 > + Replace the HostCallFrame bit anded to the CallerFrame value in a CallFrame with "anded to" ==> "or'ed to". > Source/JavaScriptCore/ChangeLog:9 > + a VM entry sentinal CallFrame. Logically, the VM entry sentinal call frame is "sentinal" ==> "sentinel". See http://dictionary.reference.com/browse/sentinel. > Source/JavaScriptCore/ChangeLog:12 > + code. The callee frame's CallerFrame points at the VM entry sentinal call frame > + and the VM entry sentinal call frame's CallerFrame points at the real caller. nit: "points at" ==> "points to"? > Source/JavaScriptCore/ChangeLog:16 > + the contiguous JSC frames. nit: "JSC frames" suggests all frames used by the VM. I think "JS frames" is what you really mean here? > Source/JavaScriptCore/interpreter/CallFrame.h:282 > + void initializeVMEntrySentinelCallerFrame(CallFrame* callFrame) Sorry to be picky about this, but would you mind renaming "initializeVMEntrySentinelCallerFrame" to "initializeVMEntrySentinel"? My reasoning is that everywhere else you use the term "vmEntrySentinelCallerFrame", you are referring to the frame that precedes the vmEntrySentinel frame. In this case, you are initializing the vmEntrySentinel which happen to take the caller frame as an argument. > Source/JavaScriptCore/interpreter/JSStackInlines.h:83 > + CallFrame* newVMEntrySentinalCallerframe = CallFrame::create(newCallFrameSlot + paddedArgsCount + JSStack::CallFrameHeaderSize); > + ASSERT(!!newVMEntrySentinalCallerframe); "newVMEntrySentinalCallerframe" ==> "newVMEntrySentinel"? > Source/JavaScriptCore/interpreter/JSStackInlines.h:95 > + newVMEntrySentinalCallerframe->initializeVMEntrySentinelCallerFrame(callerFrame); ==> newVMEntrySentinel->initializeVMEntrySentinel(callerFrame); > Source/JavaScriptCore/interpreter/JSStackInlines.h:98 > + newCallFrame->init(codeBlock, 0, scope, newVMEntrySentinalCallerframe, argsCount, callee); ditto.

(In reply to comment #12) I made the edit Geoff suggested. > (From update of attachment 215942 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=215942&action=review > > I'd prefer if you make these edits before landing. Thanks. > > > Source/JavaScriptCore/ChangeLog:8 > > + Replace the HostCallFrame bit anded to the CallerFrame value in a CallFrame with > > "anded to" ==> "or'ed to". > > > Source/JavaScriptCore/ChangeLog:9 > > + a VM entry sentinal CallFrame. Logically, the VM entry sentinal call frame is > > "sentinal" ==> "sentinel". See http://dictionary.reference.com/browse/sentinel. > > > Source/JavaScriptCore/ChangeLog:12 > > + code. The callee frame's CallerFrame points at the VM entry sentinal call frame > > + and the VM entry sentinal call frame's CallerFrame points at the real caller. > > nit: "points at" ==> "points to"? > > > Source/JavaScriptCore/ChangeLog:16 > > + the contiguous JSC frames. > > nit: "JSC frames" suggests all frames used by the VM. I think "JS frames" is what you really mean here? > > > Source/JavaScriptCore/interpreter/CallFrame.h:282 > > + void initializeVMEntrySentinelCallerFrame(CallFrame* callFrame) > > Sorry to be picky about this, but would you mind renaming "initializeVMEntrySentinelCallerFrame" to "initializeVMEntrySentinel"? My reasoning is that everywhere else you use the term "vmEntrySentinelCallerFrame", you are referring to the frame that precedes the vmEntrySentinel frame. In this case, you are initializing the vmEntrySentinel which happen to take the caller frame as an argument. > > > Source/JavaScriptCore/interpreter/JSStackInlines.h:83 > > + CallFrame* newVMEntrySentinalCallerframe = CallFrame::create(newCallFrameSlot + paddedArgsCount + JSStack::CallFrameHeaderSize); > > + ASSERT(!!newVMEntrySentinalCallerframe); > > "newVMEntrySentinalCallerframe" ==> "newVMEntrySentinel"? > > > Source/JavaScriptCore/interpreter/JSStackInlines.h:95 > > + newVMEntrySentinalCallerframe->initializeVMEntrySentinelCallerFrame(callerFrame); > > ==> newVMEntrySentinel->initializeVMEntrySentinel(callerFrame); > > > Source/JavaScriptCore/interpreter/JSStackInlines.h:98 > > + newCallFrame->init(codeBlock, 0, scope, newVMEntrySentinalCallerframe, argsCount, callee); > > ditto. I made the suggested edits, except that I changed initializeVMEntrySentinelCallerFrame() to initializeVMEntrySentinelFrame() and newVMEntrySentinalCallerframe to newVMEntrySentinalFrame.

Committed r158586: <http://trac.webkit.org/changeset/158586>

It appears that plugins/refcount-leaks.html started failing after this patch got landed: http://trac.webkit.org/log/?verbose=on&rev=158586&stop_rev=158585

(In reply to comment #15) > It appears that plugins/refcount-leaks.html started failing after this patch got landed: > > http://trac.webkit.org/log/?verbose=on&rev=158586&stop_rev=158585 The Debug version of the test works for me. I'll check release Release after the build finishes.

(In reply to comment #16) > (In reply to comment #15) > > It appears that plugins/refcount-leaks.html started failing after this patch got landed: > > > > http://trac.webkit.org/log/?verbose=on&rev=158586&stop_rev=158585 > > The Debug version of the test works for me. I'll check release Release after the build finishes. I have reproduced this running Release version of the tests. This doesn't happen with WebKitTestRunner, only DumpRenderTree. Investigating further...

Filed https://bugs.webkit.org/show_bug.cgi?id=123765 to track the failure.