Bug 23551 – Crash on page load with profiler enabled and running

• Status: RESOLVED
• Resolution: FIXED
• Component: JavaScriptCore

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

[Mark Rowe (bdash)]

<rdar://problem/6529521>

[Cameron Zwarich (cpst)]

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.

[Cameron Zwarich (cpst)]

*** Bug 23204 has been marked as a duplicate of this bug. ***

[Cameron Zwarich (cpst)]

*** Bug 23447 has been marked as a duplicate of this bug. ***

[Cameron Zwarich (cpst)]

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.

[Cameron Zwarich (cpst)]

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.

[Cameron Zwarich (cpst)]

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?

[Geoffrey Garen]

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.

[Geoffrey Garen]

Afterwards, you should probably check whether the other crashes are truly dups.

[Cameron Zwarich (cpst)]

> 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.

[Cameron Zwarich (cpst)]

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?

[Geoffrey Garen]

> > 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.

[Cameron Zwarich (cpst)]

Okay. I'll put a patch up for review.

[Cameron Zwarich (cpst)]

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?

[Cameron Zwarich (cpst)]

Comment on attachment 27156 [details]
Proposed patch

Oops. Wrong profiler.diff on my machine. This already landed. ;-)

[Cameron Zwarich (cpst)]

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?

[Cameron Zwarich (cpst)]

Created attachment 27158 [details]
With a better ChangeLog

I didn't hit command-S on the ChangeLog. ;-)

[Cameron Zwarich (cpst)]

Landed in r40396.