Bug 194777 – Check the existence of the frame in Document::hasFrameSpecificStorageAccess() and Document::setHasFrameSpecificStorageAccess()

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore Misc.

[John Wilander]

We should check that the document has a frame in Document::hasFrameSpecificStorageAccess() and Document::setHasFrameSpecificStorageAccess().

[John Wilander]

rdar://problem/47731945

[John Wilander]

Created attachment 362301 [details]
Patch

[Chris Dumez]

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.

[John Wilander]

Created attachment 362321 [details]
Patch

[Geoffrey Garen]

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?

[John Wilander]

(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?

[Chris Dumez]

(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.

[John Wilander]

(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.

[John Wilander]

Created attachment 362335 [details]
Patch for landing

[WebKit Commit Bot]

Comment on attachment 362335 [details]
Patch for landing

Clearing flags on attachment: 362335

Committed r241743: <https://trac.webkit.org/changeset/241743>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.