Last Comment Bug 727446 - Let the window owning a storage dispatch an event when the storage changes
: Let the window owning a storage dispatch an event when the storage changes
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: DOM: Events (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla14
Assigned To: Tim Taubert [:ttaubert]
:
: Andrew Overholt [:overholt]
Mentors:
Depends on:
Blocks: 669603
  Show dependency treegraph
 
Reported: 2012-02-15 07:30 PST by Tim Taubert [:ttaubert]
Modified: 2013-02-20 16:11 PST (History)
4 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
patch v1 (8.33 KB, patch)
2012-02-15 07:30 PST, Tim Taubert [:ttaubert]
no flags Details | Diff | Splinter Review
patch v2 (8.69 KB, patch)
2012-02-15 11:11 PST, Tim Taubert [:ttaubert]
bugs: review-
Details | Diff | Splinter Review
patch v3 (8.70 KB, patch)
2012-02-20 04:59 PST, Tim Taubert [:ttaubert]
no flags Details | Diff | Splinter Review
patch v4 (9.80 KB, patch)
2012-02-20 07:26 PST, Tim Taubert [:ttaubert]
bugs: review-
Details | Diff | Splinter Review
patch v5 (9.84 KB, patch)
2012-02-20 15:31 PST, Tim Taubert [:ttaubert]
bugs: review+
Details | Diff | Splinter Review
patch v6 (9.85 KB, patch)
2012-02-20 16:12 PST, Tim Taubert [:ttaubert]
no flags Details | Diff | Splinter Review
patch v7 (13.93 KB, patch)
2012-03-28 07:10 PDT, Tim Taubert [:ttaubert]
bugs: feedback-
Details | Diff | Splinter Review
patch v8 (11.66 KB, patch)
2012-03-29 13:48 PDT, Tim Taubert [:ttaubert]
bugs: review+
Details | Diff | Splinter Review

Description Tim Taubert [:ttaubert] 2012-02-15 07:30:20 PST
Created attachment 597400 [details] [diff] [review]
patch v1

http://mxr.mozilla.org/mozilla-central/source/dom/base/nsGlobalWindow.cpp#8577

nsGlobalWindow doesn't dispatch a "storage" event if it detects that (e.storageArea == window.session/localStorage) to not fire events for any storage objects owned by itself.

In bug 669603 we re-implement serializing sessionStorage data to restore it after a crash or a normal session restore. We currently always write/read this data and thus need a way to be notified when to mark a browsing context as "dirty".

A good solution would be to let a window dispatch a custom Moz* event when a storage it owns changes. So the session store service could just listen for these events and quickly retrieve the affected <browser> with event.currentTarget and mark it as dirty.
Comment 1 Olli Pettay [:smaug] 2012-02-15 07:50:27 PST
Comment on attachment 597400 [details] [diff] [review]
patch v1

Could you please not dispatch events meant for
Firefox UI to web content.
Adding NS_EVENT_FLAG_ONLY_CHROME_DISPATCH to the event's flags
should take care of that.

QI to nsIPrivateDOMEvent, GetInternalNSEvent()->flags |= NS_EVENT_FLAG_ONLY_CHROME_DISPATCH;
Comment 2 Honza Bambas (:mayhemer) 2012-02-15 08:13:15 PST
Tim, please run tests at tests\dom\tests\mochitest\localstorage, sessionstorage, globalstorage and storageevent prior to submitting the patch.
Comment 3 Tim Taubert [:ttaubert] 2012-02-15 11:11:15 PST
Created attachment 597490 [details] [diff] [review]
patch v2

(In reply to Olli Pettay [:smaug] from comment #1)
> Could you please not dispatch events meant for
> Firefox UI to web content.
> Adding NS_EVENT_FLAG_ONLY_CHROME_DISPATCH to the event's flags
> should take care of that.

Done.

(In reply to Honza Bambas (:mayhemer) from comment #2)
> Tim, please run tests at tests\dom\tests\mochitest\localstorage,
> sessionstorage, globalstorage and storageevent prior to submitting the patch.

All tests are green.
Comment 4 Olli Pettay [:smaug] 2012-02-19 17:16:51 PST
Comment on attachment 597490 [details] [diff] [review]
patch v2


>+    if (fireMozStorageChanged) {
>+      nsCOMPtr<nsIPrivateDOMEvent> privateEvent = do_QueryInterface(event);
>+      if (!privateEvent)
>+        return NS_OK;
if (expr) {
  stmt;
}


>+
>+      // Fire a "MozStorageChanged" event if this window owns changingStorage.
>+      rv = event->InitEvent(NS_LITERAL_STRING("MozStorageChanged"), false, false);
>+      NS_ENSURE_SUCCESS(rv, rv);
Doesn't this change the behavior of the event in the other windows.
IIRC storage code has the crazy setup firing the same event in many windows.

Honza would know better.
Comment 5 Tim Taubert [:ttaubert] 2012-02-20 04:59:02 PST
Created attachment 598832 [details] [diff] [review]
patch v3

(In reply to Olli Pettay [:smaug] from comment #4)
> >+    if (fireMozStorageChanged) {
> >+      nsCOMPtr<nsIPrivateDOMEvent> privateEvent = do_QueryInterface(event);
> >+      if (!privateEvent)
> >+        return NS_OK;
> if (expr) {
>   stmt;
> }

Fixed.

> >+      // Fire a "MozStorageChanged" event if this window owns changingStorage.
> >+      rv = event->InitEvent(NS_LITERAL_STRING("MozStorageChanged"), false, false);
> >+      NS_ENSURE_SUCCESS(rv, rv);
> Doesn't this change the behavior of the event in the other windows.
> IIRC storage code has the crazy setup firing the same event in many windows.

This patch re-uses the event because we fire MozStorageChanged only when we wouldn't have dispatched any event at all without the patch.

> Honza would know better.

Asking Honza for review.
Comment 6 Tim Taubert [:ttaubert] 2012-02-20 05:12:42 PST
Comment on attachment 598832 [details] [diff] [review]
patch v3

This patch modifies the event that might be re-used in other windows. We need to clone it or create a new one.
Comment 7 Tim Taubert [:ttaubert] 2012-02-20 07:26:04 PST
Created attachment 598866 [details] [diff] [review]
patch v4

nsGlobalWindow does now always create a clone of the event included in dom-storage2-changed.
Comment 8 Tim Taubert [:ttaubert] 2012-02-20 07:27:11 PST
Tests are green on my machine.
Comment 9 Olli Pettay [:smaug] 2012-02-20 11:03:09 PST
Comment on attachment 598866 [details] [diff] [review]
patch v4


>+      nsAutoString key;
>+      nsAutoString oldValue;
>+      nsAutoString newValue;
>+      nsAutoString url;
>+
>+      rv = event->GetKey(key);
>+      NS_ENSURE_SUCCESS(rv, rv);
>+
>+      rv = event->GetOldValue(oldValue);
>+      NS_ENSURE_SUCCESS(rv, rv);
>+
>+      rv = event->GetNewValue(newValue);
>+      NS_ENSURE_SUCCESS(rv, rv);
>+
>+      rv = event->GetUrl(url);
>+      NS_ENSURE_SUCCESS(rv, rv);
I really thing using NS_ENSURE_SUCCESS isn't needed in this case.

>+
>+      event = new nsDOMStorageEvent();
>+      rv = event->InitStorageEvent(fireMozStorageChanged ?
>+                                   NS_LITERAL_STRING("MozStorageChanged") :
>+                                   NS_LITERAL_STRING("storage"),
>+                                   false,
>+                                   false,
Could you read even these values from the original event.


>+
>+    // Make sure the MozStorageChanged event is dispatched to chrome, only.
>+    if (fireMozStorageChanged) {
>+      nsCOMPtr<nsIPrivateDOMEvent> privateEvent = do_QueryInterface(event);
>+      if (!privateEvent) {
>+        return NS_OK;
>+      }
>+
>+      nsEvent *internalEvent = privateEvent->GetInternalNSEvent();
>+      if (!internalEvent) {
>+        return NS_OK;
>+      }
If nsIPrivateDOMEvent doesn't have internalEvent, it is ok to just crash.

You don't set element trusted anywhere.
Comment 10 Tim Taubert [:ttaubert] 2012-02-20 15:31:02 PST
Created attachment 598993 [details] [diff] [review]
patch v5

(In reply to Olli Pettay [:smaug] from comment #9)
> >+      rv = event->GetUrl(url);
> >+      NS_ENSURE_SUCCESS(rv, rv);
> I really thing using NS_ENSURE_SUCCESS isn't needed in this case.

Yeah... removed.

> >+      rv = event->InitStorageEvent(fireMozStorageChanged ?
> >+                                   NS_LITERAL_STRING("MozStorageChanged") :
> >+                                   NS_LITERAL_STRING("storage"),
> >+                                   false,
> >+                                   false,
> Could you read even these values from the original event.

Done.

> >+      nsEvent *internalEvent = privateEvent->GetInternalNSEvent();
> >+      if (!internalEvent) {
> >+        return NS_OK;
> >+      }
> If nsIPrivateDOMEvent doesn't have internalEvent, it is ok to just crash.

Removed the check.

> You don't set element trusted anywhere.

Added
Comment 11 Olli Pettay [:smaug] 2012-02-20 15:46:24 PST
Comment on attachment 598993 [details] [diff] [review]
patch v5


>+    // Make sure the MozStorageChanged event is dispatched to chrome, only.
>+    if (fireMozStorageChanged) {
>+      nsCOMPtr<nsIPrivateDOMEvent> privateEvent = do_QueryInterface(event, &rv);
>+      NS_ENSURE_SUCCESS(rv, rv);
>+
>+      privateEvent->SetTrusted(true);
All these events should be trusted.
Move privateEvent->SetTrusted outside the if()
Comment 12 Tim Taubert [:ttaubert] 2012-02-20 16:12:34 PST
Created attachment 599003 [details] [diff] [review]
patch v6

(In reply to Olli Pettay [:smaug] from comment #11)
> >+      privateEvent->SetTrusted(true);
> All these events should be trusted.
> Move privateEvent->SetTrusted outside the if()

Done.
Comment 13 Honza Bambas (:mayhemer) 2012-02-22 07:27:12 PST
(In reply to Olli Pettay [:smaug] from comment #11)
> Comment on attachment 598993 [details] [diff] [review]
> patch v5
> 
> 
> >+    // Make sure the MozStorageChanged event is dispatched to chrome, only.
> >+    if (fireMozStorageChanged) {
> >+      nsCOMPtr<nsIPrivateDOMEvent> privateEvent = do_QueryInterface(event, &rv);
> >+      NS_ENSURE_SUCCESS(rv, rv);
> >+
> >+      privateEvent->SetTrusted(true);
> All these events should be trusted.
> Move privateEvent->SetTrusted outside the if()

So, the current code, w/o this patch, is wrong?
Comment 14 Honza Bambas (:mayhemer) 2012-02-22 07:30:21 PST
(In reply to Olli Pettay [:smaug] from comment #4)
> IIRC storage code has the crazy setup firing the same event in many windows.

Hmm.. I don't know whether firing the same event among other windows is correct or not.

So, the same question, is this aspect in the current code also wrong?
Comment 15 Olli Pettay [:smaug] 2012-02-22 07:43:36 PST
(In reply to Honza Bambas (:mayhemer) from comment #13)
> > Move privateEvent->SetTrusted outside the if()
> 
> So, the current code, w/o this patch, is wrong?
Storage events should be trusted. They are dispatched by the UA as a result of some storage
changes.
Comment 16 Olli Pettay [:smaug] 2012-02-22 07:44:56 PST
"the user agent must queue a task to fire an event with the name storage ...
at each Window object whose Document object has a Storage object that is affected."
So, there is a new event for each window.
Comment 17 Honza Bambas (:mayhemer) 2012-02-22 07:49:18 PST
Comment on attachment 599003 [details] [diff] [review]
patch v6

Review of attachment 599003 [details] [diff] [review]:
-----------------------------------------------------------------

This looks generally good.  Maybe just the cloning code could be moved to a separate method.  Best would be to have a new interface with nsIDOMEvent clone(type) that nsDOMStorageEvent would implement to keep the code well encapsulated and also might save a half of string copying (keys and values may be quit large!).  

Also Olli should make sure we need to clone the event for each window (the copying may be just unnecessary overhead).

Please let me take a look at the updated patch, it will be quicker the next time.

::: dom/tests/mochitest/localstorage/test_localStorageBase.html
@@ +160,5 @@
> +
> +  // Listen for MozStorageChanged
> +  SpecialPowers.addChromeEventListener("MozStorageChanged", function onStorageChanged(e) {
> +    SpecialPowers.removeChromeEventListener("MozStorageChanged", onStorageChanged, false);
> +    is(e.storageArea, localStorage, "check e.storageArea");

I think you should save |localStorage| to some local var in the parent context.  This is window.localStorage but when the window here breaks or the event gets by mistake fired for a different window, we may not catch the storage on the event is different from the one we were working with all the time.  Also, here the storage will be empty so you may not recognize the storage is absolutely a different one (maybe just a fresh instance).

I would expect to a check for prefilled localStorage to check the content is as expected.

Also, you should add a check the event is fired just ones for one change on the storage, it also means to do it sooner then right before the end of the test.
Comment 18 Honza Bambas (:mayhemer) 2012-02-22 09:29:55 PST
(In reply to Olli Pettay [:smaug] from comment #16)
> "the user agent must queue a task to fire an event with the name storage ...
> at each Window object whose Document object has a Storage object that is
> affected."
> So, there is a new event for each window.

Aha, so the implementation is not to just asynchronously invoke one event in all windows one by one (meaning no main thread loop among calls and only loop between change to the storage and the actual event code fire) but to asynchronously fire to *all* windows independently with potential for main thread loops even among the events.  Is that how you understand the spec?

If the letter is the case then even just the cloning is still not enough.  We do post of the event to the main thread ("dom-storage2-changed" observer notification is posted, [1]), but then the DOM event is fired at all windows in a single main thread event queue loop (I checked the code in a debugger).  If you clone the event, we still don't allow main thread event queue loops between each fire of the event in each window.  So, the cloning is just unnecessary overhead (string copying).  As I understand, the event is immutable, right?  So, no need to have a copy for each window IMO.

According the spec, it is hard to say whether our implementation is wrong or not.  The main purpose of "posting a task" here is to not call the event handler "from inside" of call to localStorage.setItem (or other modifying method).  What we do now is IMO enough.

If want to do more work here according the above, then it has to be done in a separate bug blocking this one.  Otherwise, the patch from the logical point of view looks OK.

Adding Ian, even though he may not read bugmail...  He may know more what is the correct implementation here.


[1] http://hg.mozilla.org/mozilla-central/annotate/e722d2ab78da/dom/src/storage/nsDOMStorage.cpp#l1984
Comment 19 Tim Taubert [:ttaubert] 2012-03-19 05:00:49 PDT
So what's the current state of this bug? Should we change storage events to be dispatched asynchronously? In all windows? Btw, if we dispatch in all windows then there would be no need for a custom MozStorageChanged event because then I could just use the standard storage event.

This blocks bug 669603 and I'd really like to bring it forward with a new patch if you let me know what needs to be done.
Comment 20 Honza Bambas (:mayhemer) 2012-03-19 10:20:17 PDT
(In reply to Tim Taubert [:ttaubert] from comment #19)
> So what's the current state of this bug? Should we change storage events to
> be dispatched asynchronously? In all windows? Btw, if we dispatch in all
> windows then there would be no need for a custom MozStorageChanged event
> because then I could just use the standard storage event.
> 
> This blocks bug 669603 and I'd really like to bring it forward with a new
> patch if you let me know what needs to be done.

There are no complains to the current implementation.  Since there is no answer so far, I'd stick with what we have.  So, just fire the same event to all windows, except your new event that needs to be from obvious reasons a clone.
Comment 21 Olli Pettay [:smaug] 2012-03-19 17:41:12 PDT
Uh, please no. We should fix the implementation to do what the spec says.
Each window should get their own event.
Comment 22 Honza Bambas (:mayhemer) 2012-03-19 17:43:37 PDT
(In reply to Olli Pettay [:smaug] from comment #21)
> Uh, please no. We should fix the implementation to do what the spec says.
> Each window should get their own event.

OK, so bug 737085 is then invalid?
Comment 23 Olli Pettay [:smaug] 2012-03-19 17:52:36 PDT
Or it is a duplicate of this one?
Comment 24 Honza Bambas (:mayhemer) 2012-03-19 17:57:56 PDT
(In reply to Olli Pettay [:smaug] from comment #23)
> Or it is a duplicate of this one?

It's a sub-part of this one.  I wanted this bug to be just about the new event, as simple as possible.

Can we somehow share the string buffer while cloning using some const nsACString mXXX or so?
Comment 25 Olli Pettay [:smaug] 2012-03-19 17:58:55 PDT
(In reply to Honza Bambas (:mayhemer) from comment #24)
> It's a sub-part of this one.  I wanted this bug to be just about the new
> event, as simple as possible.
Ah, ok.


> Can we somehow share the string buffer while cloning using some const
> nsACString mXXX or so?
Strings are automatically shared.
Comment 26 Tim Taubert [:ttaubert] 2012-03-26 07:02:16 PDT
(In reply to Olli Pettay [:smaug] from comment #21)
> Uh, please no. We should fix the implementation to do what the spec says.
> Each window should get their own event.

If we fix the implementation to have *each* window get their own storage event then we don't need the custom event anymore because the window owning the storage receives an event, too.

Shouldn't this bug then be about correcting the storage event and implementing it like the spec says?
Comment 27 Olli Pettay [:smaug] 2012-03-26 09:35:35 PDT
(In reply to Tim Taubert [:ttaubert] from comment #26)
> (In reply to Olli Pettay [:smaug] from comment #21)
> > Uh, please no. We should fix the implementation to do what the spec says.
> > Each window should get their own event.
> 
> If we fix the implementation to have *each* window get their own storage
> event then we don't need the custom event anymore because the window owning
> the storage receives an event, too.
Er, I mean each window except the one...

> 
> Shouldn't this bug then be about correcting the storage event and
> implementing it like the spec says?
IIRC per spec the event shouldn't be dispatched to the that one window.
Comment 28 Tim Taubert [:ttaubert] 2012-03-26 12:56:45 PDT
Ok, thank you for clarifying.
Comment 29 Honza Bambas (:mayhemer) 2012-03-26 14:26:50 PDT
Exactly, the originating window whom script made the change MUST NOT get the event for that change.
Comment 30 Tim Taubert [:ttaubert] 2012-03-28 07:10:47 PDT
Created attachment 610124 [details] [diff] [review]
patch v7

Ok, I introduced a new interface named nsIDOMCloneableEvent that provides a 'NSIDOMEvent Clone(aType)' method. I'm now always cloning the event so that every window gets its own.

All storage tests are passing. I didn't address Honza's review comments from comment #17, yet. Wanted to get some feedback on the new approach first.

Please be nice if I'm doing something unreasonable, that's a whole new area to me and http://bit.ly/wk8bXA
Comment 31 Olli Pettay [:smaug] 2012-03-28 08:23:07 PDT
Comment on attachment 610124 [details] [diff] [review]
patch v7

nsIDOMCloneableEvent is just a bit too much. Just copy the values manually.
(And the patch would make  "CloneableEvent" in window to return true)
Comment 32 Tim Taubert [:ttaubert] 2012-03-29 13:48:59 PDT
Created attachment 610667 [details] [diff] [review]
patch v8

Reverted the patch to just normally cloning the event, moved the cloning itself to a separate method. Corrected the tests as Honza suggested, verified that the respective MOZ_STORAGE_CHANGED_COUNTs make sense and are valid. All tests are green on my machine.
Comment 33 Olli Pettay [:smaug] 2012-03-30 09:14:11 PDT
Comment on attachment 610667 [details] [diff] [review]
patch v8

What about dispatching events on a task? Isn't that what the
spec requires?
Comment 34 Tim Taubert [:ttaubert] 2012-03-30 09:18:14 PDT
I thought that's what bug 737085 is about. Or should I include it in my patch as well?
Comment 35 Olli Pettay [:smaug] 2012-03-30 09:19:35 PDT
Ok, let's do that in bug 737085
Comment 36 Tim Taubert [:ttaubert] 2012-03-30 12:21:14 PDT
https://hg.mozilla.org/integration/fx-team/rev/9987bba302ca
Comment 37 Tim Taubert [:ttaubert] 2012-04-01 09:50:37 PDT
Pushed two follow-up test fixes which should have been only one, sorry :/

https://hg.mozilla.org/integration/fx-team/rev/de5dde8bfa29
https://hg.mozilla.org/integration/fx-team/rev/9669e95f51ab

Note You need to log in before you can comment on or make changes to this bug.