Bug 118268 – Crash in PageBanner::detachFromPage() when called from ~WebPage()

• Component: Layout and Rendering
• Status: RESOLVED FIXED

[Ada Chan]

The crash log looks like this:


Thread 0 Crashed:: Dispatch queue: com.apple.main-thread
0   com.apple.WebCore             	0x0000000102e1443d WebCore::Page::addHeaderWithHeight(int) + 13 (Page.cpp:1309)
1   com.apple.WebKit2             	0x000000010195ee08 WebKit::PageBanner::detachFromPage() + 74 (PageBannerMac.mm:87)
2   com.apple.WebKit2             	0x00000001019eedc4 WebKit::WebPage::~WebPage() + 188 (RefPtr.h:76)
3   com.apple.WebKit2             	0x00000001019eeca9 WebKit::WebPage::~WebPage() + 17 (ThreadSafeRefCounted.h:72)
4   com.apple.WebKit2             	0x0000000101a455f9 WTF::HashTable<unsigned long long, WTF::KeyValuePair<unsigned long long, WTF::RefPtr<WebKit::WebPage> >, WTF::KeyValuePairKeyExtractor<WTF::KeyValuePair<unsigned long long, WTF::RefPtr<WebKit::WebPage> > >, WTF::IntHash<unsigned long long>, WTF::HashMapValueTraits<WTF::HashTraits<unsigned long long>, WTF::HashTraits<WTF::RefPtr<WebKit::WebPage> > >, WTF::HashTraits<unsigned long long> >::remove(WTF::KeyValuePair<unsigned long long, WTF::RefPtr<WebKit::WebPage> >*) + 31 (HashTraits.h:62)
5   com.apple.WebKit2             	0x0000000101a41d89 WebKit::WebProcess::removeWebPage(unsigned long long) + 37 (WebProcess.cpp:593)
6   com.apple.WebKit2             	0x00000001019efd18 WebKit::WebPage::close() + 466 (WebPage.cpp:867)
7   com.apple.WebKit2             	0x00000001019372c1 CoreIPC::MessageReceiverMap::dispatchMessage(CoreIPC::Connection*, CoreIPC::MessageDecoder&) + 137 (MessageReceiverMap.cpp:86)
8   com.apple.WebKit2             	0x0000000101a41eb2 WebKit::WebProcess::didReceiveMessage(CoreIPC::Connection*, CoreIPC::MessageDecoder&) + 34 (WebProcess.cpp:637)
9   com.apple.WebKit2             	0x000000010190ac37 CoreIPC::Connection::dispatchMessage(WTF::PassOwnPtr<CoreIPC::MessageDecoder>) + 105 (ArgumentDecoder.h:47)
10  com.apple.WebKit2             	0x000000010190c694 CoreIPC::Connection::dispatchOneMessage() + 96 (PassOwnPtr.h:59)
11  com.apple.WebCore             	0x0000000102fd64ae WebCore::RunLoop::performWork() + 254 (Functional.h:704)
12  com.apple.WebCore             	0x0000000102fd69b2 WebCore::RunLoop::performWork(void*) + 34 (RunLoopCF.cpp:39)
13  com.apple.CoreFoundation      	0x00007fff92a41b31 __CFRUNLOOP_IS_CALLING_OUT_TO_A_SOURCE0_PERFORM_FUNCTION__ + 17
14  com.apple.CoreFoundation      	0x00007fff92a41455 __CFRunLoopDoSources0 + 245
15  com.apple.CoreFoundation      	0x00007fff92a647f5 __CFRunLoopRun + 789
16  com.apple.CoreFoundation      	0x00007fff92a640e2 CFRunLoopRunSpecific + 290
17  com.apple.HIToolbox           	0x00007fff97aa7eb4 RunCurrentEventLoopInMode + 209
18  com.apple.HIToolbox           	0x00007fff97aa7c52 ReceiveNextEventCommon + 356
19  com.apple.HIToolbox           	0x00007fff97aa7ae3 BlockUntilNextEventMatchingListInMode + 62
20  com.apple.AppKit              	0x00007fff966be533 _DPSNextEvent + 685
21  com.apple.AppKit              	0x00007fff966bddf2 -[NSApplication nextEventMatchingMask:untilDate:inMode:dequeue:] + 128
22  com.apple.AppKit              	0x00007fff966b51a3 -[NSApplication run] + 517
23  com.apple.WebCore             	0x0000000102fd7032 WebCore::RunLoop::run() + 82 (RunLoopMac.mm:43)
24  com.apple.WebKit2             	0x00000001019af97b int WebKit::ChildProcessMain<WebKit::WebProcess, WebKit::WebContentProcessMainDelegate>(int, char**) + 579 (ChildProcessEntryPoint.h:100)
25  com.apple.WebProcess          	0x00000001018c0e23 main + 337 (ChildProcessMain.mm:83)
26  libdyld.dylib                 	0x00007fff8c8a27e1 start + 1

Looks like m_webPage->corePage() is null.

Proposed fix is to add a null check:

void PageBanner::detachFromPage()
{
    // We can hide the banner by removing the parent layer that hosts it.
    if (m_webPage && m_webPage->corePage()) {
        if (m_type == Header)
            m_webPage->corePage()->addHeaderWithHeight(0);
        else if (m_type == Footer)
            m_webPage->corePage()->addFooterWithHeight(0);
    }

    m_type = NotSet;
    m_webPage = 0;
}

Beth, Sam: what do you think?

[Beth Dakin]

I think it would be just slightly better to return early in this function if there is no WebPage. Something like:

void PageBanner::detachFromPage()
{
    // Comment about how it is possible that the banner has already detached from the page by explicitly being set to null.
    if (!m_webPage)
        return;

    // We can hide the banner by removing the parent layer that hosts it.
    if ( m_webPage->corePage()) {
        if (m_type == Header)
            m_webPage->corePage()->addHeaderWithHeight(0);
        else if (m_type == Footer)
            m_webPage->corePage()->addFooterWithHeight(0);
    }

    m_type = NotSet;
    m_webPage = 0;
}

[Ada Chan]

(In reply to comment #1)
> I think it would be just slightly better to return early in this function if there is no WebPage. Something like:
> 
> void PageBanner::detachFromPage()
> {
>     // Comment about how it is possible that the banner has already detached from the page by explicitly being set to null.

I'm actually not sure how it is possible that we get into the situation where m_webPage is 0. In the case of this crash, m_webPage is non-null, but m_webPage->corePage() is because by the time ~WebPage() is called, WebPage::close() has been called and WebPage::m_page has been set to null.

>     if (!m_webPage)
>         return;
> 
>     // We can hide the banner by removing the parent layer that hosts it.
>     if ( m_webPage->corePage()) {
>         if (m_type == Header)
>             m_webPage->corePage()->addHeaderWithHeight(0);
>         else if (m_type == Footer)
>             m_webPage->corePage()->addFooterWithHeight(0);
>     }
> 
>     m_type = NotSet;
>     m_webPage = 0;
> }

[Ada Chan]

Created attachment 205857 [details]
Patch

[Beth Dakin]

Comment on attachment 205857 [details]
Patch

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

> Source/WebKit2/WebProcess/WebPage/mac/PageBannerMac.mm:84
> +    if (!m_webPage)

I think we should make this:

if (!m_webPage || !m_webPage->corePage())
    return;

And you can put your comment up there too.

[Darin Adler]

Comment on attachment 205857 [details]
Patch

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

>> Source/WebKit2/WebProcess/WebPage/mac/PageBannerMac.mm:84
>> +    if (!m_webPage)
> 
> I think we should make this:
> 
> if (!m_webPage || !m_webPage->corePage())
>     return;
> 
> And you can put your comment up there too.

But don’t we want to set m_type to NotSet and m_webPage to 0 even if m_webPage->corePage() is 0?

[Beth Dakin]

(In reply to comment #5)
> (From update of attachment 205857 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=205857&action=review
> 
> >> Source/WebKit2/WebProcess/WebPage/mac/PageBannerMac.mm:84
> >> +    if (!m_webPage)
> > 
> > I think we should make this:
> > 
> > if (!m_webPage || !m_webPage->corePage())
> >     return;
> > 
> > And you can put your comment up there too.
> 
> But don’t we want to set m_type to NotSet and m_webPage to 0 even if m_webPage->corePage() is 0?

Oh right. Yes. I forgot that my theory that this was happening because PageBanner::detachFromPage() had already been called was false. In this case, I think your patch is fine as-is, Ada.

[Ada Chan]

Thanks Beth and Darin for your comments!

Committed:
https://trac.webkit.org/changeset/152316