Summary: | Check the existence of the frame in Document::hasFrameSpecificStorageAccess() and Document::setHasFrameSpecificStorageAccess() | ||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | John Wilander <wilander> | ||||||||
Component: | WebCore Misc. | Assignee: | John Wilander <wilander> | ||||||||
Status: | RESOLVED FIXED | ||||||||||
Severity: | Normal | CC: | achristensen, cdumez, commit-queue, dbates, esprehn+autocc, ews-watchlist, ggaren, kangil.han, webkit-bug-importer | ||||||||
Priority: | P2 | Keywords: | InRadar | ||||||||
Version: | WebKit Nightly Build | ||||||||||
Hardware: | Unspecified | ||||||||||
OS: | Unspecified | ||||||||||
Attachments: |
|
Description
John Wilander
2019-02-18 08:53:53 PST
Created attachment 362301 [details]
Patch
Comment on attachment 362301 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=362301&action=review > Source/WebCore/ChangeLog:9 > + No new tests. I think we should try and write a test as explained offline. Created attachment 362321 [details]
Patch
Comment on attachment 362321 [details] 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? (In reply to Geoffrey Garen from comment #5) > Comment on attachment 362321 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=362321&action=review > > r=me Thanks! > > 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? I'm actually of the same opinion but Chris wants us to find other bad call sites (if there are any). Chris, care to weigh in? (In reply to John Wilander from comment #6) > (In reply to Geoffrey Garen from comment #5) > > Comment on attachment 362321 [details] > > Patch > > > > View in context: > > https://bugs.webkit.org/attachment.cgi?id=362321&action=review > > > > r=me > > Thanks! > > > > 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? > > I'm actually of the same opinion but Chris wants us to find other bad call > sites (if there are any). Chris, care to weigh in? I am OK with ASSERT() with early return. I actually thought this is what we eventually agreed on offline but I probably wasn't clear I changed my mind. (In reply to Chris Dumez from comment #7) > (In reply to John Wilander from comment #6) > > (In reply to Geoffrey Garen from comment #5) > > > Comment on attachment 362321 [details] > > > Patch > > > > > > View in context: > > > https://bugs.webkit.org/attachment.cgi?id=362321&action=review > > > > > > r=me > > > > Thanks! > > > > > > 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? > > > > I'm actually of the same opinion but Chris wants us to find other bad call > > sites (if there are any). Chris, care to weigh in? > > I am OK with ASSERT() with early return. I actually thought this is what we > eventually agreed on offline but I probably wasn't clear I changed my mind. OK, I will fix this and land. Thanks, both of you. Created attachment 362335 [details]
Patch for landing
Comment on attachment 362335 [details] Patch for landing Clearing flags on attachment: 362335 Committed r241743: <https://trac.webkit.org/changeset/241743> All reviewed patches have been landed. Closing bug. |