Bug 88412 – StorageAreaProxy::canAccessStorage should cache the permissions.

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

[Marja Hölttä]

StorageAreaProxy::canAccessStorage should cache the permissions.

[Marja Hölttä]

Created attachment 146015 [details]
Patch

Comment on attachment 146015 [details]
Patch

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

> Source/WebKit/chromium/ChangeLog:3
> +        StorageAreaProxy::canAccessStorage should cache the permissions.

prefix with [chromium] please

> Source/WebKit/chromium/src/StorageAreaProxy.h:31
> +#include <map>

don't use stl map but <wtf/HashMap.h>

> Source/WebKit/chromium/src/StorageAreaProxy.h:78
> +    mutable std::map<Frame*, bool> m_canAccessStorageCache;

I wonder whether we should instead add a boolean to WebFrameImpl canAccessStorage

[Marja Hölttä]

Created attachment 146032 [details]
Patch

[Marja Hölttä]

Comment on attachment 146015 [details]
Patch

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

>> Source/WebKit/chromium/ChangeLog:3
>> +        StorageAreaProxy::canAccessStorage should cache the permissions.
> 
> prefix with [chromium] please

Done

>> Source/WebKit/chromium/src/StorageAreaProxy.h:31
>> +#include <map>
> 
> don't use stl map but <wtf/HashMap.h>

Removed this

>> Source/WebKit/chromium/src/StorageAreaProxy.h:78
>> +    mutable std::map<Frame*, bool> m_canAccessStorageCache;
> 
> I wonder whether we should instead add a boolean to WebFrameImpl canAccessStorage

Added.

this patch looks good to me, thx

[Michael Nordman]

Comment on attachment 146032 [details]
Patch

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

> Source/WebKit/chromium/src/WebFrameImpl.cpp:1408
> +        m_canAccessStorage = !permissionClient || permissionClient->allowStorage(this, localStorage);

is the answer ever different for local vs session storage?

Comment on attachment 146032 [details]
Patch

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

>> Source/WebKit/chromium/src/WebFrameImpl.cpp:1408
>> +        m_canAccessStorage = !permissionClient || permissionClient->allowStorage(this, localStorage);
> 
> is the answer ever different for local vs session storage?

hum, no, but we show different notifications. Also, it's an implementation detail outside of webkit, so we should probably cache both results

Comment on attachment 146032 [details]
Patch

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

>> Source/WebKit/chromium/src/WebFrameImpl.cpp:1408
>> +        m_canAccessStorage = !permissionClient || permissionClient->allowStorage(this, localStorage);
> 
> is the answer ever different for local vs session storage?

hum, no, but we show different notifications. Also, it's an implementation detail outside of webkit, so we should probably cache both results

[Marja Hölttä]

Comment on attachment 146032 [details]
Patch

Ahh, this was wrong, I'll fix the patch.

[Michael Nordman]

Comment on attachment 146032 [details]
Patch

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

>>>> Source/WebKit/chromium/src/WebFrameImpl.cpp:1408
>>>> +        m_canAccessStorage = !permissionClient || permissionClient->allowStorage(this, localStorage);
>>> 
>>> is the answer ever different for local vs session storage?
>> 
>> hum, no, but we show different notifications. Also, it's an implementation detail outside of webkit, so we should probably cache both results
> 
> hum, no, but we show different notifications. Also, it's an implementation detail outside of webkit, so we should probably cache both results

hold on, if this setting can be different per-origin, webframeimpl is probably not a good place to cache this value since the frame can navigate from document to document in different origins. the storageareaproxy class is probably the best place for this cached value. instances of that class are per-document. while the methods of that class take a Frame* as input, in practice, the Frame* param value presented to a StorageAreaProxy instance will be the same value on each call. maybe something like this...

class StorageAreaProxy {

  bool canAccess(frame) {
    if (frame != m_cachedCanAccessSettingForFrame) {
      m_cachedCanAccessSetting = get value out of frame and such;
      m_cachedCanAccessSettingForFrame = frame;
    }
    return m_cachedCanAccessSetting;
  }

  void* m_cachedCanAccessSettingForFrame;  // the frame ptr for which we've cached the value (null if not cached)
  bool m_cachedCanAccessSetting;
};

Also, if there's no use case for having the setting be different for local vs session, maybe we should alter the webkit api to remove that param from the permissionClient->allowStorage() method (eventually), and in this patch just pass a hard coded value of 'true' and take it as a TODO or FIXME to clean up that api.

(In reply to comment #10)
> (From update of attachment 146032 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=146032&action=review
> 
> >>>> Source/WebKit/chromium/src/WebFrameImpl.cpp:1408
> >>>> +        m_canAccessStorage = !permissionClient || permissionClient->allowStorage(this, localStorage);
> >>> 
> >>> is the answer ever different for local vs session storage?
> >> 
> >> hum, no, but we show different notifications. Also, it's an implementation detail outside of webkit, so we should probably cache both results
> > 
> > hum, no, but we show different notifications. Also, it's an implementation detail outside of webkit, so we should probably cache both results
> 
> hold on, if this setting can be different per-origin, webframeimpl is probably not a good place to cache this value since the frame can navigate from document to document in different origins. the storageareaproxy class is probably the best place for this cached value. instances of that class are per-document. while the methods of that class take a Frame* as input, in practice, the Frame* param value presented to a StorageAreaProxy instance will be the same value on each call. maybe something like this...

What if a page has multiple frames from the same origin, all using local storage?

> 
> class StorageAreaProxy {
> 
>   bool canAccess(frame) {
>     if (frame != m_cachedCanAccessSettingForFrame) {
>       m_cachedCanAccessSetting = get value out of frame and such;
>       m_cachedCanAccessSettingForFrame = frame;
>     }
>     return m_cachedCanAccessSetting;
>   }
> 
>   void* m_cachedCanAccessSettingForFrame;  // the frame ptr for which we've cached the value (null if not cached)
>   bool m_cachedCanAccessSetting;
> };
> 
> Also, if there's no use case for having the setting be different for local vs session, maybe we should alter the webkit api to remove that param from the permissionClient->allowStorage() method (eventually), and in this patch just pass a hard coded value of 'true' and take it as a TODO or FIXME to clean up that api.

The use case is displaying different UI based on whether it's sessionStorage or localStorage

[Michael Nordman]

> > hold on, if this setting can be different per-origin, webframeimpl is probably not a good place to cache this value since the frame can navigate from document to document in different origins. the storageareaproxy class is probably the best place for this cached value. instances of that class are per-document. while the methods of that class take a Frame* as input, in practice, the Frame* param value presented to a StorageAreaProxy instance will be the same value on each call. maybe something like this...
> 
> What if a page has multiple frames from the same origin, all using local storage?

Each document has a distinct StorageAreaProxy, actually potentially two of them, one for session storage and another for local storage.

> > 
> > class StorageAreaProxy {
> > 
> >   bool canAccess(frame) {
> >     if (frame != m_cachedCanAccessSettingForFrame) {
> >       m_cachedCanAccessSetting = get value out of frame and such;
> >       m_cachedCanAccessSettingForFrame = frame;
> >     }
> >     return m_cachedCanAccessSetting;
> >   }
> > 
> >   void* m_cachedCanAccessSettingForFrame;  // the frame ptr for which we've cached the value (null if not cached)
> >   bool m_cachedCanAccessSetting;
> > };
> > 
> > Also, if there's no use case for having the setting be different for local vs session, maybe we should alter the webkit api to remove that param from the permissionClient->allowStorage() method (eventually), and in this patch just pass a hard coded value of 'true' and take it as a TODO or FIXME to clean up that api.
> 
> The use case is displaying different UI based on whether it's sessionStorage or localStorage

Ok, so we keep the param then.

[Michael Nordman]

(In reply to comment #12)
> > > class StorageAreaProxy {
> > > 
> > >   bool canAccess(frame) {
> > >     if (frame != m_cachedCanAccessSettingForFrame) {
> > >       m_cachedCanAccessSetting = get value out of frame and such;
> > >       m_cachedCanAccessSettingForFrame = frame;
> > >     }
> > >     return m_cachedCanAccessSetting;
> > >   }
> > > 
> > >   void* m_cachedCanAccessSettingForFrame;  // the frame ptr for which we've cached the value (null if not cached)
> > >   bool m_cachedCanAccessSetting;
> > > };

If you like, I can work this into another small performance related change to this class thats in the works... https://chromiumcodereview.appspot.com/10539097/

[Michael Nordman]

Created attachment 147699 [details]
tlc

[Michael Nordman]

Created attachment 147701 [details]
tlc

added missing platform/ChangeLog diffs to the patch

[Michael Nordman]

Created attachment 147899 [details]
tlc

not sure why the patch didn't apply, reupping to run it by the ews bots again

[Michael Nordman]

> not sure why the patch didn't apply, reupping to run it by the ews bots again

i think that may have to do with line endings

fyi, the chrome-side can be seen here, https://chromiumcodereview.appspot.com/10533093/

[Michael Nordman]

Created attachment 148945 [details]
tlc

[WebKit Review Bot]

Please wait for approval from [e-mail], [e-mail], [e-mail], [e-mail] or [e-mail] before submitting, as this patch contains changes to the Chromium public API. See also https://trac.webkit.org/wiki/ChromiumWebKitAPI.

[Adam Barth]

Comment on attachment 148945 [details]
tlc

API change LGTM

[Michael Nordman]

(In reply to comment #20)
> (From update of attachment 148945 [details])
> API change LGTM

jochen, can you take a look at the .cpp parts of this?

(In reply to comment #21)
> (In reply to comment #20)
> > (From update of attachment 148945 [details] [details])
> > API change LGTM
> 
> jochen, can you take a look at the .cpp parts of this?

I'm surprised this doesn't break LayoutTests/platform/chromium/permissionclient/storage-permission.html ?

[Michael Nordman]

> I'm surprised this doesn't break LayoutTests/platform/chromium/permissionclient/storage-permission.html ?

It sure should break that test! Probably 'passing' because of this test expectation...

BUGCR85292 : platform/chromium/permissionclient/storage-permission.html = PASS TEXT

(In reply to comment #23)
> > I'm surprised this doesn't break LayoutTests/platform/chromium/permissionclient/storage-permission.html ?
> 
> It sure should break that test! Probably 'passing' because of this test expectation...
> 
> BUGCR85292 : platform/chromium/permissionclient/storage-permission.html = PASS TEXT

ouch

Anyway, the question is, should the cache be invalidated by the embedder? Alternatively, we could update the test to always block storage.

[Michael Nordman]

(In reply to comment #24)
> (In reply to comment #23)
> > > I'm surprised this doesn't break LayoutTests/platform/chromium/permissionclient/storage-permission.html ?
> > 
> > It sure should break that test! Probably 'passing' because of this test expectation...
> > 
> > BUGCR85292 : platform/chromium/permissionclient/storage-permission.html = PASS TEXT
> 
> ouch
> 
> Anyway, the question is, should the cache be invalidated by the embedder? Alternatively, we could update the test to always block storage.

I'll poke at the test to setup to block access prior to checking and have that test just check the blocked condition.

I've unskipped the test meanwhile. Can you update the test so it still works with this patch in?

[Levi Weintraub]

Comment on attachment 148945 [details]
tlc

Clearing flags.