Bug 238158

Created attachment 455287 [details]
Patch

Created attachment 455288 [details]
Patch

Comment on attachment 455288 [details]
Patch

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

> Source/WebCore/Modules/storage/StorageManager.cpp:73
> +    if (!origin->canAccessStorage())

The spec merely says to check if the origin is opaque (which we call unique in WebKit). Why not simply check `origin->isUnique()` ?

> Source/WebCore/page/SecurityOrigin.h:154
> +    bool canAccessStorage() const { return canAccessStorage(nullptr); }

There is a comment inside canAccessStorage() saying that we should stop passing nullptr so this may not be a change in the right direction.

(In reply to Chris Dumez from comment #3)
> Comment on attachment 455288 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=455288&action=review
> 
> > Source/WebCore/Modules/storage/StorageManager.cpp:73
> > +    if (!origin->canAccessStorage())
> 
> The spec merely says to check if the origin is opaque (which we call unique
> in WebKit). Why not simply check `origin->isUnique()` ?

I see the other APIs using canAccessStorage to check so I use it for consistency.
I guess we can only check if origin is unique, though I don't know why we can ignore storage blocking policy for Storage API.

> 
> > Source/WebCore/page/SecurityOrigin.h:154
> > +    bool canAccessStorage() const { return canAccessStorage(nullptr); }
> 
> There is a comment inside canAccessStorage() saying that we should stop
> passing nullptr so this may not be a change in the right direction.

Comment on attachment 455288 [details]
Patch

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

>>> Source/WebCore/Modules/storage/StorageManager.cpp:73
>>> +    if (!origin->canAccessStorage())
>> 
>> The spec merely says to check if the origin is opaque (which we call unique in WebKit). Why not simply check `origin->isUnique()` ?
> 
> I see the other APIs using canAccessStorage to check so I use it for consistency.
> I guess we can only check if origin is unique, though I don't know why we can ignore storage blocking policy for Storage API.

Ok, so you want to do more checks that simply opaque origin, is that right? The change log doesn't say anything about that (and neither does the spec).

The change may well be correct if so.

(In reply to Chris Dumez from comment #5)
> Comment on attachment 455288 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=455288&action=review
> 
> >>> Source/WebCore/Modules/storage/StorageManager.cpp:73
> >>> +    if (!origin->canAccessStorage())
> >> 
> >> The spec merely says to check if the origin is opaque (which we call unique in WebKit). Why not simply check `origin->isUnique()` ?
> > 
> > I see the other APIs using canAccessStorage to check so I use it for consistency.
> > I guess we can only check if origin is unique, though I don't know why we can ignore storage blocking policy for Storage API.
> 
> Ok, so you want to do more checks that simply opaque origin, is that right?
> The change log doesn't say anything about that (and neither does the spec).
> 
> The change may well be correct if so.

Yes, I would like to make this consistent with IndexedDB, whose spec only says about opaque origin(https://www.w3.org/TR/IndexedDB/#dom-idbfactory-open), but we also check storage blocking policy of the origin (we don't check its top origin, so we pass nullptr). I will update changelog to include this.

Comment on attachment 455288 [details]
Patch

Ok.

Created attachment 455429 [details]
Patch for landing

Committed r291726 (248758@main): <https://commits.webkit.org/248758@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 455429 [details].

<rdar://problem/90667430>