Bug 84387 – [chromium] DomStorage events handling needs TLC (1)

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

[Michael Nordman]

Events are handled inconsistently. The dispatch of some events are initiated from within webkit/webcore, the dispatch of other events are initiated from the outside via the WebKit::WebStorageEventDispatcher interface. The existing WebStorageEventDispatcher is not expressive enough to handle initiated all events from the outside. There's a chunk of nearly replicated code in there that shouldn't be. The existing code has several FIXMEs related to making this better.

It could use some TLC for those reasons alone, but the current state of things in webkit/webcore is also blocking development of some overall performance improvements to chromium's implemention of this feature (getting rid of sync ipcs for each access and adding a renderer-side caching layer).

To facilitate the perf improvements, event dispatching needs to be initiated from the outside because there may be an async latency between setting an item and receiving the 'oldValue' from the main browser process which is needed to raise the mutation event.

[Michael Nordman]

Created attachment 137993 [details]
events

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

[WebKit Review Bot]

Attachment 137993 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1
Source/WebKit/chromium/src/StorageAreaProxy.cpp:202:  One line control clauses should not use braces.  [whitespace/braces] [4]
Source/WebKit/chromium/src/StorageAreaProxy.h:72:  The parameter name "storage" adds no information, so it should be removed.  [readability/parameter_name] [5]
Total errors found: 2 in 13 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Michael Nordman]

Created attachment 137994 [details]
events

style fixes

[Michael Nordman]

ping somebody with a (r) next to your name

[Michael Nordman]

Comment on attachment 137994 [details]
events

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

> Source/WebKit/chromium/public/WebStorageEventDispatcher.h:49
> +            bool originatedInProcess);

I should probably rename the 'sourceArea' param to 'sourceAreaInstance' to be more clear. The intent of that param is to identify the Document that caused the mutation.

[Michael Nordman]

Comment on attachment 137994 [details]
events

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

> Source/WebKit/chromium/src/StorageAreaProxy.cpp:200
> +            if (frame->document()->securityOrigin()->equal(securityOrigin) && !IsEventSource(frame->domWindow()->optionalLocalStorage(), sourceArea))

I suspect this SecurityOrigin::equal() test could give us some testing grief, some tests use file urls whose origins are considered unique (and things just behave differently). I'm working on the chrome-side changes to use the new dispatch API and will find out soon enough.

[Eric Seidel (no email)]

Adam is on vacation for another week, but he's a great person to have review this patch.  If you can wait until May (and likely ping him again) that would probably be best.

[Michael Nordman]

(In reply to comment #8)
> Adam is on vacation for another week, but he's a great person to have review this patch.  If you can wait until May (and likely ping him again) that would probably be best.

I'll be out that first week of May and I was kind of hoping to be wrapping up this project by the end of next week.

Maybe [e-mail], [e-mail], [e-mail] or [e-mail] could take a peek.

[Dimitri Glazkov (Google)]

Comment on attachment 137994 [details]
events

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

Here are a few general comments.

> Source/WebCore/ChangeLog:18
> +        To facilitate the perf improvements, this patch adds WebKit API too allow all

too -> to

> Source/WebCore/ChangeLog:28
> +        xxxxxxx 4/5: cleanup, remove extra params from new API whose only purpose was to

was to ... what? :)

> Source/WebCore/page/DOMWindow.h:358
> +        Storage* optionalSessionStorage() const { return m_sessionStorage.get(); }
> +        Storage* optionalLocalStorage() const { return m_localStorage.get(); }

"optional" seems unclear in meaning. Does this goe around the RAII in the original accessor? In WebKit, we usually do the other way around, with "ensure" function to do the initialization. I guess we could name it something like sessionStorageOrNull?

> Source/WebCore/storage/Storage.h:54
> +        StorageArea* area() const { return m_storageArea.get(); }

I am not familiar with this code -- why was area kept private before?

> Source/WebKit/chromium/public/WebStorageEventDispatcher.h:58
> +    // DEPRECATED - The instance methods are going away soon in favor

Perhaps link to a bug that aims to remove this?

> Source/WebKit/chromium/src/StorageAreaProxy.cpp:186
> +    // FIXME: Multi-sided patch engineering alert !

Perhaps link to a meta bug that explains the effort?

> Source/WebKit/chromium/src/StorageAreaProxy.cpp:203
> +    const HashSet<Page*>& pages = PageGroup::pageGroup(pageGroupName)->pages();
> +    for (HashSet<Page*>::const_iterator it = pages.begin(); it != pages.end(); ++it) {
> +        for (Frame* frame = (*it)->mainFrame(); frame; frame = frame->tree()->traverseNext()) {
> +            if (frame->document()->securityOrigin()->equal(securityOrigin) && !IsEventSource(frame->domWindow()->optionalLocalStorage(), sourceArea))
> +                frames.append(frame);
> +        }
> +    }

This looks like an isolated helper function -- and eric@ is right that abarth@ is a good person to look at this.

> Source/WebKit/chromium/src/StorageAreaProxy.cpp:210
> +            frames[i]->document()->enqueueWindowEvent(StorageEvent::create(eventNames().storageEvent, key, oldValue, newValue, pageURL, storage));

Interesting -- since we just enqueue events, do we need to worry about guarding the frames?

> Source/WebKit/chromium/src/StorageAreaProxy.cpp:234
> +    Vector<RefPtr<Frame> > frames;
> +    for (Frame* frame = page->mainFrame(); frame; frame = frame->tree()->traverseNext()) {
> +        if (frame->document()->securityOrigin()->equal(securityOrigin) && !IsEventSource(frame->domWindow()->optionalSessionStorage(), sourceArea))
> +            frames.append(frame);
> +    }

Ditto about the helper function.

> Source/WebKit/chromium/src/StorageAreaProxy.cpp:241
> +            frames[i]->document()->enqueueWindowEvent(StorageEvent::create(eventNames().storageEvent, key, oldValue, newValue, pageURL, storage));

Ditto about async event dispatch.

> Source/WebKit/chromium/src/StorageAreaProxy.h:73
> +    static bool IsEventSource(Storage*, WebKit::WebStorageArea* sourceArea);
> +    static Page* FindPageWithSessionStorageNamespace(const String& pageGroupName, const WebKit::WebStorageNamespace& sessionNamespace);

lowercase names.

> Source/WebKit/chromium/src/StorageEventDispatcherImpl.cpp:44
> +// FIXME: delete this almost obsolete file soon

Link to a bug?

> Source/WebKit/chromium/src/WebStorageEventDispatcherImpl.cpp:45
> +// static

No need for this comment in WebKit-land.

> Source/WebKit/chromium/src/WebStorageEventDispatcherImpl.cpp:52
> +    RefPtr<WebCore::SecurityOrigin> securityOrigin = WebCore::SecurityOrigin::create(origin);

Creating a security origin? Must check with @abarth :)

> Source/WebKit/chromium/src/WebStorageEventDispatcherImpl.cpp:58
> +// static

Ditto.

[Michael Nordman]

Comment on attachment 137994 [details]
events

>> Source/WebKit/chromium/src/StorageAreaProxy.cpp:186
>> +    // FIXME: Multi-sided patch engineering alert !
> 
> Perhaps link to a meta bug that explains the effort?

The bug is this bug, this comment just describes the multi-sided sequence of patches to alter the interfaces as described in this bug. This bug mentions 'overall performance improvements' as a goal, with the exception of these api changes, that effort is in the chrome codebase so it doesn't make sense to have a meta bug in webkit for that effort. Here's some related work in progress over there.

http://codereview.chromium.org/10160003/
http://codereview.chromium.org/10162015/  

The first defines a set of more async ipcs. The second is very preliminary yet, but if you squint, you can see where a caching layer is to be inserted in the renderer.


>> Source/WebKit/chromium/src/StorageAreaProxy.cpp:203
>> +    }
> 
> This looks like an isolated helper function -- and eric@ is right that abarth@ is a good person to look at this.

Its defined as a static class method for two reasons, it accesses private members of this class to do its work, and the function it performs is very much related to this class which is all about proxying between the embedder and webcore internals. Do you think it would be better to have these methods elsewhere, where, and why?

>> Source/WebKit/chromium/src/StorageAreaProxy.cpp:210
>> +            frames[i]->document()->enqueueWindowEvent(StorageEvent::create(eventNames().storageEvent, key, oldValue, newValue, pageURL, storage));
> 
> Interesting -- since we just enqueue events, do we need to worry about guarding the frames?

I think you're right. I'll pull this up into the loop above on my next upload of this patch. Thnx.

>> Source/WebKit/chromium/src/StorageAreaProxy.h:73
>> +    static Page* FindPageWithSessionStorageNamespace(const String& pageGroupName, const WebKit::WebStorageNamespace& sessionNamespace);
> 
> lowercase names.

doh!

[Dimitri Glazkov (Google)]

Comment on attachment 137994 [details]
events

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

>>> Source/WebKit/chromium/src/StorageAreaProxy.cpp:186
>>> +    // FIXME: Multi-sided patch engineering alert !
>> 
>> Perhaps link to a meta bug that explains the effort?
> 
> The bug is this bug, this comment just describes the multi-sided sequence of patches to alter the interfaces as described in this bug. This bug mentions 'overall performance improvements' as a goal, with the exception of these api changes, that effort is in the chrome codebase so it doesn't make sense to have a meta bug in webkit for that effort. Here's some related work in progress over there.
> 
> http://codereview.chromium.org/10160003/
> http://codereview.chromium.org/10162015/  
> 
> The first defines a set of more async ipcs. The second is very preliminary yet, but if you squint, you can see where a caching layer is to be inserted in the renderer.

I was more thinking along the lines of linking to a bug that tracks changes in WebKit. Here's an example of what I was describing: http://code.google.com/searchframe#OAMlx_jo-ck/src/third_party/WebKit/Source/WebCore/dom/ShadowRoot.cpp&exact_package=chromium&q=authorshadow&type=cs&l=75

Here, we aim to eventually get rid of this whole function, but can't at the moment. Each condition is thus tracked by a bug, so that those reading or refactoring the code could understand the wider context. It is totally uncritical for this patch, since you probably will have this done quickly.

>>> Source/WebKit/chromium/src/StorageAreaProxy.cpp:203
>>> +    }
>> 
>> This looks like an isolated helper function -- and eric@ is right that abarth@ is a good person to look at this.
> 
> Its defined as a static class method for two reasons, it accesses private members of this class to do its work, and the function it performs is very much related to this class which is all about proxying between the embedder and webcore internals. Do you think it would be better to have these methods elsewhere, where, and why?

The function of populating vector of frame ref ptrs seems like a  nicely isolatable free-standing function (same code is also present in dispatchSessionStorageEvent), but I think you may not need it at all, since the event dispatch is async.

[Michael Nordman]

Created attachment 138410 [details]
events

[Michael Nordman]

Created attachment 138411 [details]
events

[Michael Nordman]

New patch
- lowered uppercase method names
- removed // static code comments
- removed second loop in each of the dispatch[Type]StorageEvents methods
- fixed ChangeLog'isms
- removed a couple more param names from method declarations
- renamed sourceArea params to sourceAreaInstance (not sure it helps that much, but a little)

Some responses to earlier comments...

> "optional" seems unclear in meaning
For consistency, these names are being patterned off of an existing method named in this same way. Widen the diff context a little and see optionalApplicationCache().

> I am not familiar with this code -- why was area kept private before?
There simply was no need for it to be exposed before. The webcore ChangeLog comment has been modified to explain the reason for these changes.

> The function of populating vector of frame ref ptrs seems like...
I see, I misunderstood your meaning and thought you were referring to the dispatch[]StorageEvents() methods as a whole. In any case, that loop populating the separate vector of frames has been removed in the latest snapshot.

[Dimitri Glazkov (Google)]

Comment on attachment 138411 [details]
events

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

ok with a nit.

> Source/WebKit/chromium/src/StorageAreaProxy.cpp:243
> +Page* StorageAreaProxy::findPageWithSessionStorageNamespace(const String& pageGroupName, const WebKit::WebStorageNamespace& sessionNamespace)

This looks like it can be just a freestanding static function.

[Michael Nordman]

Comment on attachment 138411 [details]
events

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

>> Source/WebKit/chromium/src/StorageAreaProxy.cpp:243
>> +Page* StorageAreaProxy::findPageWithSessionStorageNamespace(const String& pageGroupName, const WebKit::WebStorageNamespace& sessionNamespace)
> 
> This looks like it can be just a freestanding static function.

yup, thnx!

[Michael Nordman]

Created attachment 138460 [details]
events - patch for landing

made findPageWithSessionStorageNamespace() a free standing static helper function instead of a static class method

[WebKit Review Bot]

Comment on attachment 138460 [details]
events - patch for landing

Clearing flags on attachment: 138460

Committed r114993: <http://trac.webkit.org/changeset/114993>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Michael Nordman]

reopening for the next in the sequence

[Michael Nordman]

Created attachment 139320 [details]
events3

Now that http://codereview.chromium.org/10201010/ has landed, some cleanup and next steps can be taken back here in webkit land.

[Dimitri Glazkov (Google)]

(In reply to comment #21)
> reopening for the next in the sequence

Don't do that. In WebKit, a bug is a patch. Umbrella bugs serve the purpose of grouping bugs. See this for example: https://bugs.webkit.org/showdependencytree.cgi?id=52962&hide_resolved=1

[WebKit Review Bot]

Attachment 139320 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1
Source/WebCore/storage/StorageArea.h:52:  The parameter name "ec" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/WebKit/chromium/src/StorageAreaProxy.h:54:  The parameter name "ec" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/WebKit/chromium/src/StorageAreaProxy.h:62:  The parameter name "pageGroup" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/WebKit/chromium/src/StorageAreaProxy.h:65:  The parameter name "pageGroup" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/WebCore/storage/StorageAreaImpl.h:49:  The parameter name "ec" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/WebKit/chromium/public/WebStorageArea.h:66:  Should have only a single space after a punctuation in a comment.  [whitespace/comments] [5]
Source/WebKit/chromium/public/WebStorageArea.h:75:  Should have only a single space after a punctuation in a comment.  [whitespace/comments] [5]
Source/WebKit/chromium/public/WebStorageArea.h:83:  Should have only a single space after a punctuation in a comment.  [whitespace/comments] [5]
Source/WebKit/chromium/public/WebStorageArea.h:92:  The parameter name "url" adds no information, so it should be removed.  [readability/parameter_name] [5]
Total errors found: 9 in 12 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[WebKit Review Bot]

Comment on attachment 139320 [details]
events3

Attachment 139320 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/12555598

[James Robinson]

One patch per bug.

[Michael Nordman]

new sacrificial bug offerings created: 85215, 85221