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

Tim, please run tests at tests\dom\tests\mochitest\localstorage, sessionstorage, globalstorage and storageevent prior to submitting the patch.

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

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

nsGlobalWindow does now always create a clone of the event included in dom-storage2-changed.

Tests are green on my machine.

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.

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

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

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

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

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

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

(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

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.

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

Uh, please no. We should fix the implementation to do what the spec says. Each window should get their own event.

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

Or it is a duplicate of this one?

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

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

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

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

Ok, thank you for clarifying.

Exactly, the originating window whom script made the change MUST NOT get the event for that change.

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

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 on attachment 610667 [details] [diff] [review] patch v8 What about dispatching events on a task? Isn't that what the spec requires?

I thought that's what bug 737085 is about. Or should I include it in my patch as well?

Ok, let's do that in bug 737085

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

https://hg.mozilla.org/mozilla-central/rev/9987bba302ca https://hg.mozilla.org/mozilla-central/rev/de5dde8bfa29 https://hg.mozilla.org/mozilla-central/rev/9669e95f51ab