Bug 87031 - DomStorage events handling needs TLC (3)
Summary: DomStorage events handling needs TLC (3)
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Michael Nordman
URL:
Keywords:
Depends on:
Blocks: 85215
  Show dependency treegraph
 
Reported: 2012-05-21 11:01 PDT by Michael Nordman
Modified: 2012-05-23 15:04 PDT (History)
6 users (show)

See Also:


Attachments
lessChatty (5.83 KB, patch)
2012-05-21 12:09 PDT, Michael Nordman
webkit-ews: commit-queue-
Details | Formatted Diff | Diff
lessChatty (6.04 KB, patch)
2012-05-21 12:33 PDT, Michael Nordman
no flags Details | Formatted Diff | Diff
lessChatty (6.16 KB, patch)
2012-05-21 15:55 PDT, Michael Nordman
abarth: review-
Details | Formatted Diff | Diff
lessChatty (6.50 KB, patch)
2012-05-23 13:57 PDT, Michael Nordman
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Michael Nordman 2012-05-21 11:01:46 PDT
Changes made in one renderer are propagated to all renderes associated with the profile of that originating renderer. That's overkill. Only those renderers containing pages that are using the domstorage area for the origin or are listening for storage events via window.onstorage need to receive these mutation events. As it stands right now, all processes 'light up' a little on any domstorage mutation (which is pretty ugly if you have a lot of tabs open on unrelated sites).

See http://code.google.com/p/chromium/issues/detail?id=128482 for more info.
Comment 1 Michael Nordman 2012-05-21 12:09:16 PDT
Created attachment 143073 [details]
lessChatty
Comment 2 Early Warning System Bot 2012-05-21 12:15:02 PDT
Comment on attachment 143073 [details]
lessChatty

Attachment 143073 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/12731715
Comment 3 Early Warning System Bot 2012-05-21 12:15:59 PDT
Comment on attachment 143073 [details]
lessChatty

Attachment 143073 [details] did not pass qt-wk2-ews (qt):
Output: http://queues.webkit.org/results/12731716
Comment 4 WebKit Review Bot 2012-05-21 12:16:28 PDT
Comment on attachment 143073 [details]
lessChatty

Attachment 143073 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/12738532
Comment 5 Build Bot 2012-05-21 12:19:59 PDT
Comment on attachment 143073 [details]
lessChatty

Attachment 143073 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/12741353
Comment 6 Build Bot 2012-05-21 12:31:11 PDT
Comment on attachment 143073 [details]
lessChatty

Attachment 143073 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/12744293
Comment 7 Michael Nordman 2012-05-21 12:33:44 PDT
Created attachment 143076 [details]
lessChatty
Comment 8 Michael Nordman 2012-05-21 15:55:07 PDT
Created attachment 143122 [details]
lessChatty
Comment 9 Michael Nordman 2012-05-21 17:47:24 PDT
cc'ing adam too
Comment 10 Adam Barth 2012-05-21 17:57:54 PDT
Comment on attachment 143122 [details]
lessChatty

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

> Source/WebCore/page/DOMWindow.cpp:1550
> +#if PLATFORM(CHROMIUM)

We shouldn't be adding PLATFORM(CHROMIUM) ifdefs to DOMWindow.cpp.

> Source/WebKit/chromium/src/StorageAreaProxy.cpp:128
> +            Storage* storage = frame->domWindow()->optionalLocalStorage();

What is optionalLocalStorage ?
Comment 11 Adam Barth 2012-05-21 18:02:14 PDT
Comment on attachment 143122 [details]
lessChatty

The general idea of this patch is fine.  The only wonky part is didAddStorageEventListener being conditional on PLATFORM(CHROMIUM).  It's also a pretty subtle change that's likely to be broken if we don't add a test upstream that checks that we continue to do this correctly.
Comment 12 Michael Nordman 2012-05-22 12:09:56 PDT
Comment on attachment 143122 [details]
lessChatty

Good question about a test. 5 of the existing test fail if the Storage instances are not created when a handler are attached, so a new test would add no value.
  storage/domstorage/events/basic-body-attribute.html -> unexpected test timed out
  storage/domstorage/events/basic-setattribute.html -> unexpected test timed out
  storage/domstorage/events/basic.html -> unexpected text diff mismatch
  storage/domstorage/events/case-sensitive.html -> unexpected text diff mismatch
  storage/domstorage/events/documentURI.html -> unexpected test timed out

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

>> Source/WebCore/page/DOMWindow.cpp:1550
>> +#if PLATFORM(CHROMIUM)
> 
> We shouldn't be adding PLATFORM(CHROMIUM) ifdefs to DOMWindow.cpp.

I'm not super happy about that either. I'm not sure any of the alternatives are better. I'm pretty certain some of them are not. Some of the options as i see them...

0) Like it is, conditional on PLATFORM.
Pros: most minimal and simple and obvious and constrained change that can be made
Cons: risks gripes about PLATFORM conditionals

1) Do this unconditionally regardless of PLATFORM.
Pros: no gripes about conditional complication
Cons: doesn't really need to be there for other PLATFORMs (but it "shouldn't" hurt)

2) Hoist the inline conditional logic into either Storage.h or StorageArea.h as a static class method
on one of those two classes.
Pros:
* removes the conditional from the  monster DOMWindow.cpp file and puts it closer to the source of platform specific'ness.
* expresses in the dom storage related .h file that something special happens at didAddStorageEventListener(window) time
Cons: Storage|StorageArea.h would have to #include DOMWindow.h (that inclusion could also be conditional)

3) Other more invasive forms of indirection involving new source files and/or new virtual methods on a 'client' object somewhere in the webcore object graph.
Pros: none
Cons: obscurity, and unecessary runtime and congnitive complexity

I'm open to suggestions. I put the patch up with #0 because it's the simplest possible to read/write/and review. #2 seems like a reasonable option to me.

>> Source/WebKit/chromium/src/StorageAreaProxy.cpp:128
>> +            Storage* storage = frame->domWindow()->optionalLocalStorage();
> 
> What is optionalLocalStorage ?

Can you make a more constructive comment? It's simply a getter that doesn't force the creation of
the underlying instance whereas the typically getter does. The name was patterned off of the
pre-existing optionalApplicationCache getter.
        Storage* optionalSessionStorage() const { return m_sessionStorage.get(); }
        Storage* optionalLocalStorage() const { return m_localStorage.get(); }
        DOMApplicationCache* optionalApplicationCache() const { return m_applicationCache.get(); }
Comment 13 Adam Barth 2012-05-22 12:14:55 PDT
> >> Source/WebKit/chromium/src/StorageAreaProxy.cpp:128
> >> +            Storage* storage = frame->domWindow()->optionalLocalStorage();
> > 
> > What is optionalLocalStorage ?
> 
> Can you make a more constructive comment? It's simply a getter that doesn't force the creation of
> the underlying instance whereas the typically getter does. The name was patterned off of the
> pre-existing optionalApplicationCache getter.
>         Storage* optionalSessionStorage() const { return m_sessionStorage.get(); }
>         Storage* optionalLocalStorage() const { return m_localStorage.get(); }
>         DOMApplicationCache* optionalApplicationCache() const { return m_applicationCache.get(); }

Ah, I see.  There's a similar function for getting DOMWindow from Frame called existingDOMWindow.  That might be a slightly clearer name, but we can worry about that later.
Comment 14 Adam Barth 2012-05-22 12:19:32 PDT
> Good question about a test. 5 of the existing test fail if the Storage instances are not created when a handler are attached, so a new test would add no value.

Great.

> >> Source/WebCore/page/DOMWindow.cpp:1550
> >> +#if PLATFORM(CHROMIUM)
> > 
> > We shouldn't be adding PLATFORM(CHROMIUM) ifdefs to DOMWindow.cpp.
> 
> I'm not super happy about that either. I'm not sure any of the alternatives are better. I'm pretty certain some of them are not. Some of the options as i see them...

Can you help me understand why creating these objects is helpful?  Is there some side effect of creating them that we might want to cause happen explicitly rather than implicitly?
Comment 15 Michael Nordman 2012-05-22 12:31:59 PDT
> Can you help me understand why creating these objects is helpful?  Is there some side effect of creating them that we might want to cause happen explicitly rather than implicitly?

First, see http://code.google.com/p/chromium/issues/detail?id=128482. If that doesn't answer your "why" question, please come back with a more specific question.

I don't see any reason to add more webkit API and plumbing than we already have. Three small changes amount to a large reduction in IPC spam.
cr change 1) [commtted] https://chromiumcodereview.appspot.com/10413018
wk change 2) this one
cr change 3) a call to the HasAreaOpen() method introduced in #1

void DOMStorageMessageFilter::SendDomStorageEvent(
    const dom_storage::DomStorageArea* area,
    const GURL& page_url,
    const NullableString16& key,
    const NullableString16& new_value,
    const NullableString16& old_value) {
  DCHECK(!BrowserThread::CurrentlyOn(BrowserThread::IO));
  // Only send mutation events to processes which have the area open.
  bool originated_in_process = connection_dispatching_message_for_ != 0;
  if (originated_in_process ||
      host_->HasAreaOpen(area->namespace_id(), area->origin())) {
    DOMStorageMsg_Event_Params params;
    params.origin = area->origin();
    params.page_url = page_url;
    params.connection_id = connection_dispatching_message_for_;
    params.key = key;
    params.new_value = new_value;
    params.old_value = old_value;
    params.namespace_id = area->namespace_id();
    Send(new DOMStorageMsg_Event(params));
  }
}
Comment 16 Adam Barth 2012-05-22 14:45:03 PDT
> First, see http://code.google.com/p/chromium/issues/detail?id=128482. If that doesn't answer your "why" question, please come back with a more specific question.

Based on reading that bug and grepping around a bit, the answer to my question is that instantiating the WebCore::Storage object causes WebKit to ask the embedder for a WebStorageArea, which calls the WebStorageImpl constructor in webstoragearea_impl.cc, which sends the DOMStorageHostMsg_OpenStorageArea to the host, which causes HasAreaOpen to return true.

It sounds like another approach would be to notify the embedder explicitly that someone is listening for storage events.  The embedder could then send DOMStorageHostMsg_OpenStorageArea (or a similar message) to the host.

The advantage of the second approach is that it's explicit rather than implicit and it avoiding having a wonky special case for PLATFORM(CHROMIUM) in DOMWindow.cpp.  The disadvantage is that there's more total machinery since the notification travels on a different pathway.

As a thought experiment, it might be worth considering how WebKit2 would approach this optimization if/when they support multiple WebProcesses.  I suspect that approach is also more consistent with what would work for WebKit2.
Comment 17 Michael Nordman 2012-05-22 16:10:55 PDT
(In reply to comment #16)
> > First, see http://code.google.com/p/chromium/issues/detail?id=128482. If that doesn't answer your "why" question, please come back with a more specific question.
> 
> Based on reading that bug and grepping around a bit, the answer to my question is that instantiating the WebCore::Storage object causes WebKit to ask the embedder for a WebStorageArea, which calls the WebStorageImpl constructor in webstoragearea_impl.cc, which sends the DOMStorageHostMsg_OpenStorageArea to the host, which causes HasAreaOpen to return true.
> 
> It sounds like another approach would be to notify the embedder explicitly that someone is listening for storage events.  The embedder could then send DOMStorageHostMsg_OpenStorageArea (or a similar message) to the host.

As mentioned earlier, I don't see any reason to add more webkit API and plumbing than we already have. The disadvantages outweigh the advantages, a parallel mechanism just makes more room for error. There would also the balancing "no longer listening" call plumb thru and get right.

> The advantage of the second approach is that it's explicit rather than implicit and it avoiding having a wonky special case for PLATFORM(CHROMIUM) in DOMWindow.cpp.  The disadvantage is that there's more total machinery since the notification travels on a different pathway.

We can do something reasonable with the PLATFORM conditional to eliminate it.

> As a thought experiment, it might be worth considering how WebKit2 would approach this optimization if/when they support multiple WebProcesses.  I suspect that approach is also more consistent with what would work for WebKit2.

webkit2... sorry... no data
Comment 18 Michael Nordman 2012-05-22 16:34:39 PDT
Also something that is not apparent by looking at code currently in the codebase is that the need to send events is not purely a function of whether there are handlers attached. The mutation events are also utilized (or will be soon) to maintain consistency of the renderer side cache. You can see this here...
https://chromiumcodereview.appspot.com/10383123/
Comment 19 Adam Barth 2012-05-22 16:37:54 PDT
Ok.  It sounds like we should try option (1):

> 1) Do this unconditionally regardless of PLATFORM.
> Pros: no gripes about conditional complication
> Cons: doesn't really need to be there for other PLATFORMs (but it "shouldn't" hurt)

Would you also be willing to update the comment to explain why we're doing this with less Chromium-specific jargon?  Here's my hamfisted attempt:

    // Creating these WebCore::Storage objects informs the client that we'd like to receive
    // notifications about storage events that might be triggered in other processes. Rather
    // than subscribe to these notifications explicitly, we subscribe to them implicitly to
    // simply the work done by the client. 

I'm not 100% convinced that doing this implicitly is better, but I'd rather not argue about it.
Comment 20 Michael Nordman 2012-05-22 17:29:48 PDT
(In reply to comment #19)
> Ok.  It sounds like we should try option (1):

I'll run webkit/mac tests and let you know how it goes.

>     // Creating these WebCore::Storage objects informs the client that we'd like to receive
>     // notifications about storage events that might be triggered in other processes. Rather
>     // than subscribe to these notifications explicitly, we subscribe to them implicitly to
>     // simply the work done by the client. 

Comment sgtm.

> I'm not 100% convinced that doing this implicitly is better, but I'd rather not argue about it.

Thnx.
Comment 21 Michael Nordman 2012-05-23 13:57:21 PDT
Created attachment 143644 [details]
lessChatty

New patch with option #1 (no longer PLATFORM conditional). Also updated the ChangeLog to describe the additional side effect of scheduling the area to be read from disk for non-chromium ports.
Comment 22 WebKit Review Bot 2012-05-23 15:04:06 PDT
Comment on attachment 143644 [details]
lessChatty

Clearing flags on attachment: 143644

Committed r118254: <http://trac.webkit.org/changeset/118254>
Comment 23 WebKit Review Bot 2012-05-23 15:04:12 PDT
All reviewed patches have been landed.  Closing bug.