When I try opening this page (or any other page on this website), with profiler enabled and running, safari just crashes. It seems to me that it's javascript-related (judging by crash log made using drwtsn32). It has been confirmed by other person on different platform.
Created attachment 27046 [details] Crash log generated by dr. watson 32
Created attachment 27048 [details] dump file generated by dr. watson 32
<rdar://problem/6529521>
This can be triggered by loading about:blank, starting a profile, and reloading about:blank again and again (with the inspector window open) until it crashes. I don't think it has much to do with the page. All of these "profiler crashes during navigation" bugs seem to be the same bug. I'll mark them all duplicates of this one, because this one has an associated Radar.
*** Bug 23204 has been marked as a duplicate of this bug. ***
*** Bug 23447 has been marked as a duplicate of this bug. ***
The problematic profiler call is in Interpreter::execute(FunctionBodyNode*, ...): if (*profiler) (*profiler)->didExecute(newCallFrame, function); Profiler::didExecute() calls a method on newCallFrame->lexicalGlobalObject(), which is garbage. The cause seems to be that part of the ScopeChain is deleted. Enabling COLLECT_ON_EVERY_ALLOCATION doesn't seem to make it more reproducible.
I am having a lot of trouble tracking this one down. I'll unassign it to myself so someone else who has a bit more luck can fix it.
So, this appears to be a difference in ScopeChain handling between Debug and Release. It's not a random compiler bug, because I still get it with NDEBUG and -O0. Printing out the ScopeChain right before the offending profiler hook along with ref counts shows this difference. I have no clue what is causing this. Oliver suspects it is a difference in conservative GC marking that is saving the day. Since ScopeChainNodes aren't allocated on the JS heap, it would have to be some other object, like a closure, keeping the ScopeChainNode alive. Geoff, you seem to be the only one who really understands the ScopeChainNode management scheme. Any thoughts?
I don't understand how you could trigger this problem just by loading about:blank, since you shouldn't execute any JavaScript, let alone Interpreter::execute(FunctionBodyNode*,...), which only gets called for special direct invocations of functions, like event handlers. That said, a function derefs its scope chain before returning, so calling "didExecute(newCallFrame...)" might provide a garbage scope chain to the profiler. That would explain why lexicalGlobalObject() could crash. The best solution is probably to pass "callFrame" rather than "newCallFrame" to the profiler.
Afterwards, you should probably check whether the other crashes are truly dups.
> I don't understand how you could trigger this problem just by loading > about:blank, since you shouldn't execute any JavaScript, let alone > Interpreter::execute(FunctionBodyNode*,...), which only gets called for special > direct invocations of functions, like event handlers. I assumed it was due to the code that the profiler executes when a page loads. > The best solution is probably to pass "callFrame" rather than "newCallFrame" to > the profiler. Won't that give the profiler the wrong lexical global object in some cases, giving the profiler the wrong profile group? Is that a problem? All fast/profiler tests pass and the crashes goes away. I still don't understand the difference between Debug and Release.
I added an assertion that the profile groups are the same, and just from browsing the web they seem to be. Is this always going to be the case, Geoff?
> > The best solution is probably to pass "callFrame" rather than "newCallFrame" to > > the profiler. > > Won't that give the profiler the wrong lexical global object in some cases, > giving the profiler the wrong profile group? Is that a problem? All > fast/profiler tests pass and the crashes goes away. I would pass "callFrame" both to willExecute and to didExecute. That will give the profiler the lexical global object of the caller, which is what we want. (We don't want to profile the function call if the caller is not being profiled.) It *would* be a slight error to pass newCallFrame to willExecute and callFrame to didExecute. I believe Interpreter::execute for EvalNode and ProgramNode make this mistake, and should probably be fixed.
Okay. I'll put a patch up for review.
Created attachment 27156 [details] Proposed patch Here's the fix. Is there a way to test the difference in behaviour with the didExecute() call in DRT, even if I can't get the crash?
Comment on attachment 27156 [details] Proposed patch Oops. Wrong profiler.diff on my machine. This already landed. ;-)
Created attachment 27157 [details] Real proposed patch Here's the fix. Is there a way to test the difference in behaviour with the didExecute() call in DRT, even if I can't get the crash?
Created attachment 27158 [details] With a better ChangeLog I didn't hit command-S on the ChangeLog. ;-)
Landed in r40396.