| Summary: | Crash in PageBanner::detachFromPage() when called from ~WebPage() | ||||||
|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Ada Chan <adachan> | ||||
| Component: | Layout and Rendering | Assignee: | Ada Chan <adachan> | ||||
| Status: | RESOLVED FIXED | ||||||
| Severity: | Normal | CC: | bdakin, sam | ||||
| Priority: | P2 | ||||||
| Version: | 528+ (Nightly build) | ||||||
| Hardware: | Unspecified | ||||||
| OS: | Unspecified | ||||||
| Attachments: |
|
||||||
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;
}
(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; > } Created attachment 205857 [details]
Patch
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. 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? (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. Thanks Beth and Darin for your comments! Committed: https://trac.webkit.org/changeset/152316 |
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?