[webkit-reviews] review denied: [Bug 67941] occasional crash in Chromium in dispatching keyEvent : [Attachment 107064] more informative assert

[bugzilla-daemon at webkit.org]

Darin Fisher (:fishd, Google) <fishd at chromium.org> has denied Scott Graham
<scottmg at chromium.org>'s request for review:
Bug 67941: occasional crash in Chromium in dispatching keyEvent
https://bugs.webkit.org/show_bug.cgi?id=67941

Attachment 107064: more informative assert
https://bugs.webkit.org/attachment.cgi?id=107064&action=review

------- Additional Comments from Darin Fisher (:fishd, Google)
<fishd at chromium.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=107064&action=review


> Source/WebKit/chromium/src/WebViewImpl.cpp:657
> +	       ASSERT_WITH_MESSAGE(frame->document(),

Are you trying to catch this in production builds?  Have you considered
writing these values to the stack and then crashing (even in release
builds), so that we can get more informative crash reports?

Often you are not the one who will see this assertion, and when other
developers see the assertion, they'll most likely ignore it unless
they suspect their local changes led to the assertion.

This is why you don't see ASSERT_WITH_MESSAGE used much.  Getting the
info into a crash dump tends to be more valuable.

> Source/WebKit/chromium/src/WebViewImpl.cpp:661
>	       if (!node || !node->renderer() ||
!node->renderer()->isEmbeddedObject())

because of this null check, we will now stop crashing in production.  won't
that
mean that we lose information about this failure?

it seems really bad for the document of the focused node to not exist.	we
shouldn't
have to be prepared for it to be null.	this tells me that this cannot be the
right
fix.  it is just a band-aid fix.  the real fix is probably to somehow resolve
why
document is null and make it not be null.