Bug 116253

Summary: Replace WebFrameLoaderClient static_casts with a function that might return null
Product: WebKit Reporter: Brady Eidson <beidson>
Component: WebKit2Assignee: Brady Eidson <beidson>
Status: RESOLVED FIXED    
Severity: Normal CC: ap, benjamin, commit-queue, sam
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: All   
OS: All   
Attachments:
Description Flags
Patch v1 ap: review+

Description Brady Eidson 2013-05-16 17:10:14 PDT
Replace WebFrameLoaderClient static_casts with a function that might return null

Spun off from https://bugs.webkit.org/show_bug.cgi?id=115917

This bug is about WebKit2.  We should handle WebKit1 separately.
Comment 1 Brady Eidson 2013-05-16 17:15:03 PDT
Created attachment 202006 [details]
Patch v1
Comment 2 Brady Eidson 2013-05-17 12:12:07 PDT
Somethings clearly off in the weeds on the EWS bot.  Landing soon.
Comment 3 Brady Eidson 2013-05-17 12:15:23 PDT
http://trac.webkit.org/changeset/150282
Comment 4 Andreas Kling 2013-05-17 12:44:59 PDT
Comment on attachment 202006 [details]
Patch v1 

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

> Source/WebKit2/WebProcess/WebCoreSupport/WebChromeClient.cpp:182
>  void WebChromeClient::focusedFrameChanged(Frame* frame)
>  {
> -    WebFrame* webFrame = frame ? static_cast<WebFrameLoaderClient*>(frame->loader()->client())->webFrame() : 0;
> +    WebFrameLoaderClient* webFrameLoaderClient = toWebFrameLoaderClient(frame->loader()->client());
> +    WebFrame* webFrame = webFrameLoaderClient ? webFrameLoaderClient->webFrame() : 0;

This function no longer null-checks the 'frame' argument.

Just got this crash on ToT:
0   com.apple.WebKit2             	0x000000010e7e1e0e WebKit::WebChromeClient::focusedFrameChanged(WebCore::Frame*) + 18 (FrameLoader.h:197)
1   com.apple.WebCore             	0x000000010f5d4dce WebCore::FocusController::setFocusedFrame(WTF::PassRefPtr<WebCore::Frame>) + 590 (FocusController.cpp:201)
2   com.apple.WebCore             	0x000000010f5f6b8d WebCore::Frame::willDetachPage() + 221 (PassRefPtr.h:67)
3   com.apple.WebCore             	0x000000010f6032c7 WebCore::FrameLoader::detachFromParent() + 471 (FrameLoader.cpp:2402)
4   com.apple.WebKit2             	0x000000010e82ada3 WebKit::WebPage::close() + 393 (WebPage.cpp:855)
Comment 5 Brady Eidson 2013-05-17 12:49:03 PDT
(In reply to comment #4)
> (From update of attachment 202006 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=202006&action=review
> 
> > Source/WebKit2/WebProcess/WebCoreSupport/WebChromeClient.cpp:182
> >  void WebChromeClient::focusedFrameChanged(Frame* frame)
> >  {
> > -    WebFrame* webFrame = frame ? static_cast<WebFrameLoaderClient*>(frame->loader()->client())->webFrame() : 0;
> > +    WebFrameLoaderClient* webFrameLoaderClient = toWebFrameLoaderClient(frame->loader()->client());
> > +    WebFrame* webFrame = webFrameLoaderClient ? webFrameLoaderClient->webFrame() : 0;
> 
> This function no longer null-checks the 'frame' argument.
> 
> Just got this crash on ToT:
> 0   com.apple.WebKit2                 0x000000010e7e1e0e WebKit::WebChromeClient::focusedFrameChanged(WebCore::Frame*) + 18 (FrameLoader.h:197)
> 1   com.apple.WebCore                 0x000000010f5d4dce WebCore::FocusController::setFocusedFrame(WTF::PassRefPtr<WebCore::Frame>) + 590 (FocusController.cpp:201)
> 2   com.apple.WebCore                 0x000000010f5f6b8d WebCore::Frame::willDetachPage() + 221 (PassRefPtr.h:67)
> 3   com.apple.WebCore                 0x000000010f6032c7 WebCore::FrameLoader::detachFromParent() + 471 (FrameLoader.cpp:2402)
> 4   com.apple.WebKit2                 0x000000010e82ada3 WebKit::WebPage::close() + 393 (WebPage.cpp:855)

Yikes.

Fixed in http://trac.webkit.org/changeset/150288