70535 2011-10-20 12:19:54 -0700 WK2 - Crash deref'ing a null context menu 2011-10-20 12:33:18 -0700 1 1 1 Unclassified WebKit WebKit2 528+ (Nightly build) All All RESOLVED FIXED InRadar P2 Normal --- 1 beidson beidson aroben oldest_to_newest 487770 0 beidson 2011-10-20 12:19:54 -0700 WK2 - Crash deref'ing a null context menu In rare cases WebPage::didSelectItemFromActiveContextMenu in the WebProcess can be called without an active context menu, leading to a null deref. We have an ASSERT in this function but no one has noticed (or reported...) this in debug builds. Replacing the ASSERT with an early return will safely plug the crash. In radar as <rdar://problem/9412849> 487777 1 111826 beidson 2011-10-20 12:26:50 -0700 Created attachment 111826 Patch v1 - s/assert/early return/ 487781 2 111826 aroben 2011-10-20 12:30:11 -0700 Comment on attachment 111826 Patch v1 - s/assert/early return/ Maybe we should keep the assert so we have a hope of catching this case in Debug builds? 487782 3 111826 bweinstein 2011-10-20 12:30:30 -0700 Comment on attachment 111826 Patch v1 - s/assert/early return/ Do we still want the assert to help us catch this in the future? 487784 4 111826 darin 2011-10-20 12:31:39 -0700 Comment on attachment 111826 Patch v1 - s/assert/early return/ View in context: https://bugs.webkit.org/attachment.cgi?id=111826&action=review > Source/WebKit2/WebProcess/WebPage/WebPage.cpp:2076 > - ASSERT(m_contextMenu); > + if (!m_contextMenu) > + return; If we don’t know why it happens and hope to some day track it down, then we could augment the assert with a return rather than replacing it with a return. But if we think it’s not surprising since the back and forth is cross-process without a strong guarantee the processes are in-sync, then perhaps we should land this patch as-is. Also, if there is some kind of race her then maybe each context menu needs an ID so choosing an item from an old context menu doesn’t get mixed up with a new one. 487785 5 beidson 2011-10-20 12:32:07 -0700 In radar Darin had mentioned to me the following: "A principle: ...It is not safe to assert something if it’s dependent on the state of the other process." I don't disagree with him. 487787 6 beidson 2011-10-20 12:33:18 -0700 Sending WebKit2/ChangeLog Sending WebKit2/WebProcess/WebPage/WebPage.cpp Transmitting file data .. Committed revision 98013. 111826 2011-10-20 12:26:50 -0700 2011-10-20 12:31:39 -0700 Patch v1 - s/assert/early return/ patch.txt text/plain 1519 beidson SW5kZXg6IFNvdXJjZS9XZWJLaXQyL0NoYW5nZUxvZwo9PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09Ci0tLSBTb3VyY2UvV2Vi S2l0Mi9DaGFuZ2VMb2cJKHJldmlzaW9uIDk4MDEwKQorKysgU291cmNlL1dlYktpdDIvQ2hhbmdl TG9nCSh3b3JraW5nIGNvcHkpCkBAIC0xLDMgKzEsMTYgQEAKKzIwMTEtMTAtMjAgIEJyYWR5IEVp ZHNvbiAgPGJlaWRzb25AYXBwbGUuY29tPgorCisgICAgICAgIDxyZGFyOi8vcHJvYmxlbS85NDEy ODQ5PiBhbmQgaHR0cHM6Ly9idWdzLndlYmtpdC5vcmcvc2hvd19idWcuY2dpP2lkPTcwNTM1Cisg ICAgICAgIFdLMiAtIENyYXNoIGRlcmVmJ2luZyBhIG51bGwgY29udGV4dCBtZW51CisKKyAgICAg ICAgUmV2aWV3ZWQgYnkgTk9CT0RZIChPT1BTISkuCisKKyAgICAgICAgKiBXZWJQcm9jZXNzL1dl YlBhZ2UvV2ViUGFnZS5jcHA6CisgICAgICAgIChXZWJLaXQ6OldlYlBhZ2U6OmRpZFNlbGVjdEl0 ZW1Gcm9tQWN0aXZlQ29udGV4dE1lbnUpOiBJbiBzb21lIGNhc2VzIHRoYXQgd2Ugc3RpbGwgY2Fu J3QgcmVwcm9kdWNlLAorICAgICAgICAgIHRoaXMgbWVzc2FnZSBjYW4gYmUgcmVjZWl2ZWQgaW4g dGhlIFdlYlByb2Nlc3MgYWZ0ZXIgdGhlIGNvbnRleHQgbWVudSBoYXMgYmVlbiBjbGVhcmVkLCBs ZWFkaW5nCisgICAgICAgICAgdG8gYSBjcmFzaC4gVHVybmluZyB0aGUgQVNTRVJUIGluIHRvIGFu IGVhcmx5IHJldHVybiB3aWxsIHByZXZlbnQgdGhlIGNyYXNoIHdoaWxlIHdlIHRyeSB0byBsZWFy biBtb3JlCisgICAgICAgICAgYWJvdXQgaG93IHRoaXMgY291bGQgaGFwcGVuLgorCiAyMDExLTEw LTIwICBHdXN0YXZvIE5vcm9uaGEgU2lsdmEgIDxnbnNAZ25vbWUub3JnPgogCiAgICAgICAgIEdU SysgYnVpbGQgZml4LiBXazIgZG9jdW1lbnRhdGlvbiB3aWxsIGJlIGRlYWx0IHdpdGggaW4gZnV0 dXJlCkluZGV4OiBTb3VyY2UvV2ViS2l0Mi9XZWJQcm9jZXNzL1dlYlBhZ2UvV2ViUGFnZS5jcHAK PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PQotLS0gU291cmNlL1dlYktpdDIvV2ViUHJvY2Vzcy9XZWJQYWdlL1dlYlBhZ2Uu Y3BwCShyZXZpc2lvbiA5ODAxMCkKKysrIFNvdXJjZS9XZWJLaXQyL1dlYlByb2Nlc3MvV2ViUGFn ZS9XZWJQYWdlLmNwcAkod29ya2luZyBjb3B5KQpAQCAtMjA3Miw3ICsyMDcyLDkgQEAgdm9pZCBX ZWJQYWdlOjpzZXRUZXh0Rm9yQWN0aXZlUG9wdXBNZW51KAogCiB2b2lkIFdlYlBhZ2U6OmRpZFNl bGVjdEl0ZW1Gcm9tQWN0aXZlQ29udGV4dE1lbnUoY29uc3QgV2ViQ29udGV4dE1lbnVJdGVtRGF0 YSYgaXRlbSkKIHsKLSAgICBBU1NFUlQobV9jb250ZXh0TWVudSk7CisgICAgaWYgKCFtX2NvbnRl eHRNZW51KQorICAgICAgICByZXR1cm47CisKICAgICBtX2NvbnRleHRNZW51LT5pdGVtU2VsZWN0 ZWQoaXRlbSk7CiAgICAgbV9jb250ZXh0TWVudSA9IDA7CiB9Cg==