118268 2013-07-01 17:39:13 -0700 Crash in PageBanner::detachFromPage() when called from ~WebPage() 2013-07-02 12:54:27 -0700 1 1 1 Unclassified WebKit Layout and Rendering 528+ (Nightly build) Unspecified Unspecified RESOLVED FIXED P2 Normal --- 1 adachan adachan bdakin sam oldest_to_newest 905182 0 adachan 2013-07-01 17:39:13 -0700 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? 905213 1 bdakin 2013-07-01 19:11:00 -0700 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; } 905221 2 adachan 2013-07-01 19:51:58 -0700 (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; > } 905223 3 205857 adachan 2013-07-01 19:59:41 -0700 Created attachment 205857 Patch 905496 4 205857 bdakin 2013-07-02 11:04:32 -0700 Comment on attachment 205857 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. 905534 5 205857 darin 2013-07-02 12:13:54 -0700 Comment on attachment 205857 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? 905537 6 bdakin 2013-07-02 12:16:10 -0700 (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. 905554 7 adachan 2013-07-02 12:54:27 -0700 Thanks Beth and Darin for your comments! Committed: https://trac.webkit.org/changeset/152316 205857 2013-07-01 19:59:41 -0700 2013-07-02 12:16:37 -0700 Patch 0001-Fix-crash-when-PageBanner-detachFromPage-is-called-f.patch text/plain 2643 adachan RnJvbSAzMDYzMWNjNTRiN2UzODk3NWQ4YjBkMWE5YTE2YjUzNDNmZDgwYWUwIE1vbiBTZXAgMTcg MDA6MDA6MDAgMjAwMQpGcm9tOiBBZGEgQ2hhbiA8YWRhY2hhbkBhcHBsZS5jb20+CkRhdGU6IE1v biwgMSBKdWwgMjAxMyAxOTo1Njo0NyAtMDcwMApTdWJqZWN0OiBbUEFUQ0hdIEZpeCBjcmFzaCB3 aGVuIFBhZ2VCYW5uZXI6OmRldGFjaEZyb21QYWdlKCkgaXMgY2FsbGVkIGZyb20KIFdlYlBhZ2U6 On5XZWJQYWdlKCkuIGh0dHBzOi8vYnVncy53ZWJraXQub3JnL3Nob3dfYnVnLmNnaT9pZD0xMTgy NjgKClJldmlld2VkIGJ5IE5PQk9EWSAoT09QUyEpLgoKKiBXZWJQcm9jZXNzL1dlYlBhZ2UvbWFj L1BhZ2VCYW5uZXJNYWMubW06CihXZWJLaXQ6OlBhZ2VCYW5uZXI6OmRldGFjaEZyb21QYWdlKTog QWRkIHNvbWUgbnVsbCBjaGVja3MuIG1fd2ViUGFnZS0+Y29yZVBhZ2UoKQpjYW4gYmUgbnVsbCBp ZiB0aGlzIG1ldGhvZCBpcyBjYWxsZWQgYWZ0ZXIgdGhlIHdlYiBwYWdlIGlzIGNsb3NlZC4KLS0t CiBTb3VyY2UvV2ViS2l0Mi9DaGFuZ2VMb2cgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAg fCAxMSArKysrKysrKysrKwogU291cmNlL1dlYktpdDIvV2ViUHJvY2Vzcy9XZWJQYWdlL21hYy9Q YWdlQmFubmVyTWFjLm1tIHwgMTcgKysrKysrKysrKysrLS0tLS0KIDIgZmlsZXMgY2hhbmdlZCwg MjMgaW5zZXJ0aW9ucygrKSwgNSBkZWxldGlvbnMoLSkKCmRpZmYgLS1naXQgYS9Tb3VyY2UvV2Vi S2l0Mi9DaGFuZ2VMb2cgYi9Tb3VyY2UvV2ViS2l0Mi9DaGFuZ2VMb2cKaW5kZXggNTM5YTI5Ni4u OTNhM2IzOSAxMDA2NDQKLS0tIGEvU291cmNlL1dlYktpdDIvQ2hhbmdlTG9nCisrKyBiL1NvdXJj ZS9XZWJLaXQyL0NoYW5nZUxvZwpAQCAtMSwzICsxLDE0IEBACisyMDEzLTA3LTAxICBBZGEgQ2hh biAgPGFkYWNoYW5AYXBwbGUuY29tPgorCisgICAgICAgIEZpeCBjcmFzaCB3aGVuIFBhZ2VCYW5u ZXI6OmRldGFjaEZyb21QYWdlKCkgaXMgY2FsbGVkIGZyb20gV2ViUGFnZTo6fldlYlBhZ2UoKS4K KyAgICAgICAgaHR0cHM6Ly9idWdzLndlYmtpdC5vcmcvc2hvd19idWcuY2dpP2lkPTExODI2OAor CisgICAgICAgIFJldmlld2VkIGJ5IE5PQk9EWSAoT09QUyEpLgorCisgICAgICAgICogV2ViUHJv Y2Vzcy9XZWJQYWdlL21hYy9QYWdlQmFubmVyTWFjLm1tOgorICAgICAgICAoV2ViS2l0OjpQYWdl QmFubmVyOjpkZXRhY2hGcm9tUGFnZSk6IEFkZCBzb21lIG51bGwgY2hlY2tzLiBtX3dlYlBhZ2Ut PmNvcmVQYWdlKCkKKyAgICAgICAgY2FuIGJlIG51bGwgaWYgdGhpcyBtZXRob2QgaXMgY2FsbGVk IGFmdGVyIHRoZSB3ZWIgcGFnZSBpcyBjbG9zZWQuCisKIDIwMTMtMDctMDEgIEFsZXhleSBQcm9z a3VyeWFrb3YgIDxhcEBhcHBsZS5jb20+CiAKICAgICAgICAgQ2xlYW4gdXAgcHJpdmF0ZSBicm93 c2luZyBzZXNzaW9uIHRyYWNraW5nCmRpZmYgLS1naXQgYS9Tb3VyY2UvV2ViS2l0Mi9XZWJQcm9j ZXNzL1dlYlBhZ2UvbWFjL1BhZ2VCYW5uZXJNYWMubW0gYi9Tb3VyY2UvV2ViS2l0Mi9XZWJQcm9j ZXNzL1dlYlBhZ2UvbWFjL1BhZ2VCYW5uZXJNYWMubW0KaW5kZXggMWU0MmIzNS4uNjk4YTFmMiAx MDA2NDQKLS0tIGEvU291cmNlL1dlYktpdDIvV2ViUHJvY2Vzcy9XZWJQYWdlL21hYy9QYWdlQmFu bmVyTWFjLm1tCisrKyBiL1NvdXJjZS9XZWJLaXQyL1dlYlByb2Nlc3MvV2ViUGFnZS9tYWMvUGFn ZUJhbm5lck1hYy5tbQpAQCAtODEsMTEgKzgxLDE4IEBAIHZvaWQgUGFnZUJhbm5lcjo6ZGlkQWRk UGFyZW50TGF5ZXIoR3JhcGhpY3NMYXllciogcGFyZW50TGF5ZXIpCiAKIHZvaWQgUGFnZUJhbm5l cjo6ZGV0YWNoRnJvbVBhZ2UoKQogewotICAgIC8vIFdlIGNhbiBoaWRlIHRoZSBiYW5uZXIgYnkg cmVtb3ZpbmcgdGhlIHBhcmVudCBsYXllciB0aGF0IGhvc3RzIGl0LgotICAgIGlmIChtX3R5cGUg PT0gSGVhZGVyKQotICAgICAgICBtX3dlYlBhZ2UtPmNvcmVQYWdlKCktPmFkZEhlYWRlcldpdGhI ZWlnaHQoMCk7Ci0gICAgZWxzZSBpZiAobV90eXBlID09IEZvb3RlcikKLSAgICAgICAgbV93ZWJQ YWdlLT5jb3JlUGFnZSgpLT5hZGRGb290ZXJXaXRoSGVpZ2h0KDApOworICAgIGlmICghbV93ZWJQ YWdlKQorICAgICAgICByZXR1cm47CisKKyAgICAvLyBtX3dlYlBhZ2UtPmNvcmVQYWdlKCkgY2Fu IGJlIG51bGwgd2hlbiB0aGlzIGlzIGNhbGxlZCBmcm9tIFdlYlBhZ2U6On5XZWJQYWdlKCkgYWZ0 ZXIKKyAgICAvLyB0aGUgd2ViIHBhZ2UgaGFzIGJlZW4gY2xvc2VkLgorICAgIGlmIChtX3dlYlBh Z2UtPmNvcmVQYWdlKCkpIHsKKyAgICAgICAgLy8gV2UgY2FuIGhpZGUgdGhlIGJhbm5lciBieSBy ZW1vdmluZyB0aGUgcGFyZW50IGxheWVyIHRoYXQgaG9zdHMgaXQuCisgICAgICAgIGlmIChtX3R5 cGUgPT0gSGVhZGVyKQorICAgICAgICAgICAgbV93ZWJQYWdlLT5jb3JlUGFnZSgpLT5hZGRIZWFk ZXJXaXRoSGVpZ2h0KDApOworICAgICAgICBlbHNlIGlmIChtX3R5cGUgPT0gRm9vdGVyKQorICAg ICAgICAgICAgbV93ZWJQYWdlLT5jb3JlUGFnZSgpLT5hZGRGb290ZXJXaXRoSGVpZ2h0KDApOwor ICAgIH0KIAogICAgIG1fdHlwZSA9IE5vdFNldDsKICAgICBtX3dlYlBhZ2UgPSAwOwotLSAKMS44 LjIuMSAoQXBwbGUgR2l0LTQ1KQoK