Bug 38535 – WebKit crashes at DebuggerCallFrame::functionName() if m_callFrame is the top global callframe.

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit Misc.

[Yongjun Zhang]

When an app is using WebKit and it also implements WebScriptDebuggerDelegate::exceptionWasRaised to sniff exceptions thrown from JS engine, and then uses [WebScriptCallFrame functionName] to get the function which throws the exception.  If the call frame is the top global call frame, [WebScriptCallFrame functionName] crashes.

The reason is JSC::DebuggerCallFrame::functionName() doesn't check if the call frame is a Program or or Function before calling m_callFrame->callee().  For global call frame, m_callFrame->callee() return 0 and it crashes later on in asFunction().

[Yongjun Zhang]

Created attachment 55032 [details]
Bailout if callframe's callee is null.

[Greg Bolsinga]

<rdar://problem/7928746>

[Darin Adler]

Comment on attachment 55032 [details]
Bailout if callframe's callee is null.

Looks OK. r=me

[WebKit Commit Bot]

Comment on attachment 55032 [details]
Bailout if callframe's callee is null.

Clearing flags on attachment: 55032

Committed r58779: <http://trac.webkit.org/changeset/58779>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[WebKit Review Bot]

http://trac.webkit.org/changeset/58779 might have broken Qt Linux Release
The following changes are on the blame list:
http://trac.webkit.org/changeset/58779
http://trac.webkit.org/changeset/58780

[Yongjun Zhang]

Reopen this since we still see crashes on the same spot when global top frame is called with functionName.

[Yongjun Zhang]

It crashes only in the case when the exception is throw from the second most top frame, aka. its caller frame is the top most frame, AND the top most frame is an anonymous function (like an eventHandler).  In this case, m_callFrame->callee() in JSC::debuggerCallFrame::functionName() returns 0x2.  We need to check if this callFrame is top callFrame in [WebScriptCallFrame functionName].

[Yongjun Zhang]

Created attachment 77301 [details]
Check if this CallFrame is top call frame in [WebScriptCallFrame functionName].

[Darin Adler]

Comment on attachment 77301 [details]
Check if this CallFrame is top call frame in [WebScriptCallFrame functionName].

View in context: https://bugs.webkit.org/attachment.cgi?id=77301&action=review

> WebKit/mac/WebView/WebScriptDebugDelegate.mm:207
> +    if (!_private->caller)
> +        return nil;

Checking this seems OK, but random; not clearly connected to the rest of this function. Could you give more detail on what fails when caller is 0? Why is adding this check the best fix? Should we be making a fix inside the JavaScriptcore functionName function?

[Darin Adler]

There must be a way to fix this in JavaScriptCore. I’m not satisfied with the current patch. Geoff, Ollie, ideas?

[Yongjun Zhang]

(In reply to comment #10)
> (From update of attachment 77301 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=77301&action=review
> 
> > WebKit/mac/WebView/WebScriptDebugDelegate.mm:207
> > +    if (!_private->caller)
> > +        return nil;
> 
> Checking this seems OK, but random; not clearly connected to the rest of this function. Could you give more detail on what fails when caller is 0? Why is adding this check the best fix? Should we be making a fix inside the JavaScriptcore functionName function?


(In reply to comment #11)
> There must be a way to fix this in JavaScriptCore. I’m not satisfied with the current patch. Geoff, Ollie, ideas?

I agree we should fix it in JavaScriptCore.  Here is what I saw.  For top call frames, the caller is 0 and functionName of a top callframe is nil.  As far as I understand, top call frame's callee (in JSC::DebuggerCallFrame::functionName()) should be 0, that way, we can bail out in DebuggerCallFrame::functionName() quickly (which is http://trac.webkit.org/changeset/58779 did).  The odd thing is for this particular top call frame, the callee in DebuggerCallFrame is 0x2, seems like it was read from a wrong offset in the RegisterFile, and it crashes later on in asFunction(m_callFrame->callee()).  
Geoff, Ollie, any idea why the callee is 0x2 in this case?

[Yongjun Zhang]

 It turns out WebScriptDebugger in WebKit has empty implementations for willExecuteProgram and didExecuteProgram.  As a result, if the top call frame is from a program, WebKitScriptDebugger doesn't record that callframe as the top frame, and WebScriptDebugger's callframe stack is wrong from this point.  That could cause crash if we trying to access the top call from from this stack when an exception throws because the saved top frame could be invalid.

[Yongjun Zhang]

Created attachment 77362 [details]
Maintain top callframe in willExecuteProgram and didExecuteProgram to keep WebScriptDebugger's callframe stack in sync with JavaScriptCore's stack.

[Yongjun Zhang]

Created attachment 77363 [details]
Maintain top callframe in willExecuteProgram and didExecuteProgram to keep WebScriptDebugger's callframe stack in sync with JavaScriptCore's stack.

[Darin Adler]

Comment on attachment 77363 [details]
Maintain top callframe in willExecuteProgram and didExecuteProgram to keep WebScriptDebugger's callframe stack in sync with JavaScriptCore's stack.

Looks good. How did you test?

[Yongjun Zhang]

(In reply to comment #16)
> (From update of attachment 77363 [details])
> Looks good. How did you test?

thanks.  I tested with the app in <rdar://problem/7928746>, and the crash doesn't happen after the fix.  It would be nice to have an unit test for this, but it involves a WebKit based app which using WebScriptDebugger API directly.

[WebKit Commit Bot]

Comment on attachment 77363 [details]
Maintain top callframe in willExecuteProgram and didExecuteProgram to keep WebScriptDebugger's callframe stack in sync with JavaScriptCore's stack.

Clearing flags on attachment: 77363

Committed r74586: <http://trac.webkit.org/changeset/74586>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.