WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
RESOLVED FIXED
118268
Crash in PageBanner::detachFromPage() when called from ~WebPage()
https://bugs.webkit.org/show_bug.cgi?id=118268
Summary
Crash in PageBanner::detachFromPage() when called from ~WebPage()
Ada Chan
Reported
2013-07-01 17:39:13 PDT
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?
Attachments
Patch
(2.58 KB, patch)
2013-07-01 19:59 PDT
,
Ada Chan
bdakin
: review+
Details
Formatted Diff
Diff
View All
Add attachment
proposed patch, testcase, etc.
Beth Dakin
Comment 1
2013-07-01 19:11:00 PDT
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
Comment 2
2013-07-01 19:51:58 PDT
(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
Comment 3
2013-07-01 19:59:41 PDT
Created
attachment 205857
[details]
Patch
Beth Dakin
Comment 4
2013-07-02 11:04:32 PDT
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 5
2013-07-02 12:13:54 PDT
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
Comment 6
2013-07-02 12:16:10 PDT
(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
Comment 7
2013-07-02 12:54:27 PDT
Thanks Beth and Darin for your comments! Committed:
https://trac.webkit.org/changeset/152316
Note
You need to
log in
before you can comment on or make changes to this bug.
Top of Page
Format For Printing
XML
Clone This Bug