Bug 38535 - WebKit crashes at DebuggerCallFrame::functionName() if m_callFrame is the top global callframe.
Summary: WebKit crashes at DebuggerCallFrame::functionName() if m_callFrame is the top...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit Misc. (show other bugs)
Version: 528+ (Nightly build)
Hardware: Mac OS X 10.5
: P2 Normal
Assignee: Yongjun Zhang
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2010-05-04 11:29 PDT by Yongjun Zhang
Modified: 2010-12-23 15:11 PST (History)
9 users (show)

See Also:


Attachments
Bailout if callframe's callee is null. (1.63 KB, patch)
2010-05-04 11:40 PDT, Yongjun Zhang
no flags Details | Formatted Diff | Diff
Check if this CallFrame is top call frame in [WebScriptCallFrame functionName]. (1.49 KB, patch)
2010-12-22 20:37 PST, Yongjun Zhang
no flags Details | Formatted Diff | Diff
Maintain top callframe in willExecuteProgram and didExecuteProgram to keep WebScriptDebugger's callframe stack in sync with JavaScriptCore's stack. (3.47 KB, patch)
2010-12-23 12:48 PST, Yongjun Zhang
no flags Details | Formatted Diff | Diff
Maintain top callframe in willExecuteProgram and didExecuteProgram to keep WebScriptDebugger's callframe stack in sync with JavaScriptCore's stack. (2.01 KB, patch)
2010-12-23 12:56 PST, Yongjun Zhang
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Yongjun Zhang 2010-05-04 11:29:24 PDT
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().
Comment 1 Yongjun Zhang 2010-05-04 11:40:58 PDT
Created attachment 55032 [details]
Bailout if callframe's callee is null.
Comment 2 Greg Bolsinga 2010-05-04 12:03:22 PDT
<rdar://problem/7928746>
Comment 3 Darin Adler 2010-05-04 12:36:03 PDT
Comment on attachment 55032 [details]
Bailout if callframe's callee is null.

Looks OK. r=me
Comment 4 WebKit Commit Bot 2010-05-04 15:01:50 PDT
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>
Comment 5 WebKit Commit Bot 2010-05-04 15:01:55 PDT
All reviewed patches have been landed.  Closing bug.
Comment 6 WebKit Review Bot 2010-05-04 15:33:51 PDT
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
Comment 7 Yongjun Zhang 2010-12-22 20:15:11 PST
Reopen this since we still see crashes on the same spot when global top frame is called with functionName.
Comment 8 Yongjun Zhang 2010-12-22 20:30:30 PST
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].
Comment 9 Yongjun Zhang 2010-12-22 20:37:41 PST
Created attachment 77301 [details]
Check if this CallFrame is top call frame in [WebScriptCallFrame functionName].
Comment 10 Darin Adler 2010-12-23 08:44:25 PST
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?
Comment 11 Darin Adler 2010-12-23 08:46:28 PST
There must be a way to fix this in JavaScriptCore. I’m not satisfied with the current patch. Geoff, Ollie, ideas?
Comment 12 Yongjun Zhang 2010-12-23 09:54:19 PST
(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?
Comment 13 Yongjun Zhang 2010-12-23 12:40:36 PST
 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.
Comment 14 Yongjun Zhang 2010-12-23 12:48:07 PST
Created attachment 77362 [details]
Maintain top callframe in willExecuteProgram and didExecuteProgram to keep WebScriptDebugger's callframe stack in sync with JavaScriptCore's stack.
Comment 15 Yongjun Zhang 2010-12-23 12:56:47 PST
Created attachment 77363 [details]
Maintain top callframe in willExecuteProgram and didExecuteProgram to keep WebScriptDebugger's callframe stack in sync with JavaScriptCore's stack.
Comment 16 Darin Adler 2010-12-23 13:02:10 PST
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?
Comment 17 Yongjun Zhang 2010-12-23 13:33:06 PST
(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.
Comment 18 WebKit Commit Bot 2010-12-23 15:10:57 PST
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>
Comment 19 WebKit Commit Bot 2010-12-23 15:11:05 PST
All reviewed patches have been landed.  Closing bug.