Closed
Bug 727446
Opened 13 years ago
Closed 13 years ago
Let the window owning a storage dispatch an event when the storage changes
Categories
(Core :: DOM: Events, defect)
Core
DOM: Events
Tracking
()
RESOLVED
FIXED
mozilla14
People
(Reporter: ttaubert, Assigned: ttaubert)
References
Details
Attachments
(1 file, 7 obsolete files)
11.66 KB,
patch
|
smaug
:
review+
|
Details | Diff | Splinter Review |
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.
Attachment #597400 -
Flags: review?(honzab.moz)
Assignee | ||
Updated•13 years ago
|
Component: DOM → DOM: Events
QA Contact: general → events
Comment 1•13 years ago
|
||
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•13 years ago
|
||
Tim, please run tests at tests\dom\tests\mochitest\localstorage, sessionstorage, globalstorage and storageevent prior to submitting the patch.
Assignee | ||
Comment 3•13 years ago
|
||
(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.
Attachment #597400 -
Attachment is obsolete: true
Attachment #597490 -
Flags: review?(bugs)
Attachment #597400 -
Flags: review?(honzab.moz)
Comment 4•13 years ago
|
||
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.
Attachment #597490 -
Flags: review?(bugs) → review-
Assignee | ||
Comment 5•13 years ago
|
||
(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.
Attachment #597490 -
Attachment is obsolete: true
Attachment #598832 -
Flags: review?(honzab.moz)
Assignee | ||
Comment 6•13 years ago
|
||
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.
Attachment #598832 -
Attachment is obsolete: true
Attachment #598832 -
Flags: review?(honzab.moz)
Assignee | ||
Comment 7•13 years ago
|
||
nsGlobalWindow does now always create a clone of the event included in dom-storage2-changed.
Attachment #598866 -
Flags: review?(honzab.moz)
Attachment #598866 -
Flags: review?(bugs)
Assignee | ||
Comment 8•13 years ago
|
||
Tests are green on my machine.
Comment 9•13 years ago
|
||
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.
Attachment #598866 -
Flags: review?(bugs) → review-
Assignee | ||
Comment 10•13 years ago
|
||
(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
Attachment #598866 -
Attachment is obsolete: true
Attachment #598993 -
Flags: review?(honzab.moz)
Attachment #598993 -
Flags: review?(bugs)
Attachment #598866 -
Flags: review?(honzab.moz)
Comment 11•13 years ago
|
||
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()
Attachment #598993 -
Flags: review?(bugs) → review+
Assignee | ||
Comment 12•13 years ago
|
||
(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.
Attachment #598993 -
Attachment is obsolete: true
Attachment #599003 -
Flags: review?(honzab.moz)
Attachment #598993 -
Flags: review?(honzab.moz)
Comment 13•13 years ago
|
||
(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•13 years ago
|
||
(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•13 years ago
|
||
(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•13 years ago
|
||
"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•13 years ago
|
||
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.
Attachment #599003 -
Flags: review?(honzab.moz)
Comment 18•13 years ago
|
||
(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
Assignee | ||
Comment 19•13 years ago
|
||
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•13 years ago
|
||
(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•13 years ago
|
||
Uh, please no. We should fix the implementation to do what the spec says.
Each window should get their own event.
Comment 22•13 years ago
|
||
(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•13 years ago
|
||
Or it is a duplicate of this one?
Comment 24•13 years ago
|
||
(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•13 years ago
|
||
(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.
Assignee | ||
Comment 26•13 years ago
|
||
(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•13 years ago
|
||
(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.
Assignee | ||
Comment 28•13 years ago
|
||
Ok, thank you for clarifying.
Comment 29•13 years ago
|
||
Exactly, the originating window whom script made the change MUST NOT get the event for that change.
Assignee | ||
Comment 30•13 years ago
|
||
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
Attachment #599003 -
Attachment is obsolete: true
Attachment #610124 -
Flags: feedback?(honzab.moz)
Attachment #610124 -
Flags: feedback?(bugs)
Comment 31•13 years ago
|
||
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)
Attachment #610124 -
Flags: feedback?(bugs) → feedback-
Assignee | ||
Comment 32•13 years ago
|
||
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.
Attachment #610124 -
Attachment is obsolete: true
Attachment #610667 -
Flags: review?(bugs)
Attachment #610124 -
Flags: feedback?(honzab.moz)
Comment 33•13 years ago
|
||
Comment on attachment 610667 [details] [diff] [review]
patch v8
What about dispatching events on a task? Isn't that what the
spec requires?
Assignee | ||
Comment 34•13 years ago
|
||
I thought that's what bug 737085 is about. Or should I include it in my patch as well?
Comment 35•13 years ago
|
||
Ok, let's do that in bug 737085
Updated•13 years ago
|
Attachment #610667 -
Flags: review?(bugs) → review+
Assignee | ||
Comment 36•13 years ago
|
||
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → mozilla14
Assignee | ||
Comment 37•13 years ago
|
||
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
Assignee | ||
Comment 38•13 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/9987bba302ca
https://hg.mozilla.org/mozilla-central/rev/de5dde8bfa29
https://hg.mozilla.org/mozilla-central/rev/9669e95f51ab
Status: ASSIGNED → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
You need to log in
before you can comment on or make changes to this bug.
Description
•