Bug 87031 – DomStorage events handling needs TLC (3)

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

[Michael Nordman]

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.

[Michael Nordman]

Created attachment 143073 [details]
lessChatty

[Early Warning System Bot]

Comment on attachment 143073 [details]
lessChatty

Attachment 143073 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/12731715

[Early Warning System Bot]

Comment on attachment 143073 [details]
lessChatty

Attachment 143073 [details] did not pass qt-wk2-ews (qt):
Output: http://queues.webkit.org/results/12731716

[WebKit Review Bot]

Comment on attachment 143073 [details]
lessChatty

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

[Build Bot]

Comment on attachment 143073 [details]
lessChatty

Attachment 143073 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/12741353

[Build Bot]

Comment on attachment 143073 [details]
lessChatty

Attachment 143073 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/12744293

[Michael Nordman]

Created attachment 143076 [details]
lessChatty

[Michael Nordman]

Created attachment 143122 [details]
lessChatty

[Michael Nordman]

cc'ing adam too

[Adam Barth]

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 ?

[Adam Barth]

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.

[Michael Nordman]

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(); }

[Adam Barth]

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

[Adam Barth]

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

[Michael Nordman]

> 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));
  }
}

[Adam Barth]

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

[Michael Nordman]

(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

[Michael Nordman]

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/

[Adam Barth]

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.

[Michael Nordman]

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

[Michael Nordman]

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.

[WebKit Review Bot]

Comment on attachment 143644 [details]
lessChatty

Clearing flags on attachment: 143644

Committed r118254: <http://trac.webkit.org/changeset/118254>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.