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>
Created attachment 111826 [details] Patch v1 - s/assert/early return/
Comment on attachment 111826 [details] Patch v1 - s/assert/early return/ Maybe we should keep the assert so we have a hope of catching this case in Debug builds?
Comment on attachment 111826 [details] Patch v1 - s/assert/early return/ Do we still want the assert to help us catch this in the future?
Comment on attachment 111826 [details] 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.
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.
Sending WebKit2/ChangeLog Sending WebKit2/WebProcess/WebPage/WebPage.cpp Transmitting file data .. Committed revision 98013.