[webkit-reviews] review granted: [Bug 194777] Check the existence of the frame in Document::hasFrameSpecificStorageAccess() and Document::setHasFrameSpecificStorageAccess() : [Attachment 362321] Patch

[bugzilla-daemon at webkit.org]

Geoffrey Garen <ggaren at apple.com> has granted John Wilander
<wilander at apple.com>'s request for review:
Bug 194777: Check the existence of the frame in
Document::hasFrameSpecificStorageAccess() and
Document::setHasFrameSpecificStorageAccess()
https://bugs.webkit.org/show_bug.cgi?id=194777

Attachment 362321: Patch

https://bugs.webkit.org/attachment.cgi?id=362321&action=review




--- Comment #5 from Geoffrey Garen <ggaren at apple.com> ---
Comment on attachment 362321
  --> https://bugs.webkit.org/attachment.cgi?id=362321
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=362321&action=review

r=me

> Source/WebKit/NetworkProcess/NetworkConnectionToWebProcess.cpp:590
> +    RELEASE_ASSERT(sessionID.isValid());

I'm not sure it's good practice to include a RELEASE_ASSERT in a fix for a null
pointer dereference. That increases the chances that the crash rate will go up
rather than down after the fix. For example, if we ever wanted to merge this
fix onto a release branch, instead of saying "it's just a null check" we would
say "it's a null check and possibly a new way to crash". That's a much more
muddled risk/reward discussion.

How about an ASSERT with early return?