1043106 - event.storageArea === localStorage or === sessionStorage is always false

Status: RESOLVED FIXED

(Core :: DOM: Events, defect)

Description

[Mathias Panzenböck]

User Agent: Mozilla/5.0 (X11; Linux x86_64; rv:30.0) Gecko/20100101 Firefox/30.0 (Beta/Release)
Build ID: 20140611060104

Steps to reproduce:

I registered a storage event listener and tried to filter out events that originate from the wrong storage area. Simplified code:

window.addEventListener("storage", function (event) {
    if (event.storageArea !== localStorage) { return; }
    // handle event using event.key and event.newValue...
}, false);


Actual results:

event.storageArea !== localStorage is always false. In fact comparing event.storageArea to localStorage or sessionStorage always fails.


Expected results:

event.storageArea !== localStorage should be false if the manipulated storageArea was in fact localStorage.

Comment 1

[Mathias Panzenböck]

Just tested with Firefox 31: still the same. Here is an example: http://fiddle.jshell.net/AUzxP/3/show/
Open it in 2 windows, click the button in one of them and see the output in the other.
Source: http://jsfiddle.net/AUzxP/3/

Comment 2

[Andrea Marchesini [:baku]]

Attachment: storageevent.patch

I'm not sure this is correct from the spec point of view. I read:

"When a user agent is to send a storage notification for a Document, the user agent must queue a task to fire a trusted event with the name storage, which does not bubble and is not cancelable, and which uses the StorageEvent interface, at the Document object's Window object.

The storageArea attribute must return the value it was initialised to. When the object is created, this attribute must be initialised to null. It represents the Storage object that was affected."

BTW: at the moment we emit 'storage' events only when another window changes the storage. I don't think this is correct. I didn't find this in the spec.

Comment 3

[Olli Pettay [:smaug][[e-mail]]]

As far as I know, it is correct to emit the events only in that case.
IIRC we used to have the bug that we fired the event too often.

But honza should know.

Comment 4

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8470784 [details] [diff] [review]
storageevent.patch

>-  dict.mStorageArea = aEvent->GetStorageArea();
>+
>+  nsRefPtr<DOMStorage> storageArea = aEvent->GetStorageArea();
>+  MOZ_ASSERT(storageArea);
>+
>+  nsRefPtr<DOMStorage> storage;
>+  if (storageArea->GetType() == DOMStorage::LocalStorage) {
>+    storage = GetLocalStorage(aRv);
>+  } else {
>+    MOZ_ASSERT(storageArea->GetType() == DOMStorage::SessionStorage);
>+    storage = GetSessionStorage(aRv);
>+  }
Can we somehow assert that the internal storage impl of 'storage' is the same as
the internal storage impl of aEvent->GetStorageArea().

(Would like to see a patch with such change)

Comment 5

[Olli Pettay [:smaug][[e-mail]]]

http://dev.w3.org/html5/webstorage/#sessionStorageEvent

'When the setItem(), removeItem(), and clear() methods are called on a Storage object x that is associated with a session storage area, if the methods did not throw an exception or "do nothing" as defined above, then for every Document object whose Window object's sessionStorage attribute's Storage object is associated with the same storage area, other than x, send a storage notification.'

Comment 6

[Andrea Marchesini [:baku]]

Attachment: storageevent.patch

Comment 7

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8470849 [details] [diff] [review]
storageevent.patch

>+  nsRefPtr<DOMStorage> storageArea = aEvent->GetStorageArea();
>+  MOZ_ASSERT(storageArea);
>+
>+  nsRefPtr<DOMStorage> storage;
>+  if (storageArea->GetType() == DOMStorage::LocalStorage) {
>+    storage = GetLocalStorage(aRv);
>+  } else {
>+    MOZ_ASSERT(storageArea->GetType() == DOMStorage::SessionStorage);
>+    storage = GetSessionStorage(aRv);
>+  }
>+
>+  if (aRv.Failed()) {
>+    return nullptr;
>+  }
>+
>+  MOZ_ASSERT(storage);
Based on the implementation of Get*Storage, they may return null, and not throw.
So, you should just do 
if (!storage || aRv.Failed()) {
  return nullptr;
}

Comment 8

[Andrea Marchesini [:baku]]

Pushed to try: https://tbpl.mozilla.org/?tree=Try&rev=a057410c33bd

Comment 9

[Andrea Marchesini [:baku]]

Some mochitests in e10s fail. I'll debug this issue.

Comment 10

[Andrea Marchesini [:baku]]

Attachment: storageevent.patch

Comment 11

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8472280 [details] [diff] [review]
storageevent.patch

Could you file a followup to clean up InnerSetNewDocument usage.
It almost works for reuse case too.

Comment 12

[Andrea Marchesini [:baku]]

https://tbpl.mozilla.org/?tree=Try&rev=63bd7560ca39

https://hg.mozilla.org/integration/mozilla-inbound/rev/c771f2431395

Comment 13

[Olli Pettay [:smaug][[e-mail]]]

Oh, I just realized, we want
+      // The storage objects contain the URL of the window. We have to recreate
+      // them when the innerWindow is reused.
+      newInnerWindow->mLocalStorage = nullptr;
+      newInnerWindow->mSessionStorage = nullptr;
only in case the document changes, to be safe.

Comment 14

[Andrea Marchesini [:baku]]

Attachment: patch 2

https://hg.mozilla.org/integration/mozilla-inbound/rev/c2d008090340

Comment 15

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/c771f2431395
https://hg.mozilla.org/mozilla-central/rev/c2d008090340

Comment 16

[Ian Nartowicz]

event.storageArea !== localStorage when a storage item was changed in a different tab, even if it is on the same page and using the same localStorage.  Effectively this is still broken.

Comment 17

[Olli Pettay [:smaug][[e-mail]]]

Ian, do you have a _minimal_ testcase for the issue you see?
Could you file a new bug, and make it block this one. And add the testcase as an attachment to that bug. Thanks.

Comment 18

[Ian Nartowicz]

I'm actually only seeing a problem using the dom-storage2-changed notification.  Using "storage" or "MozStorageChanged" directly is OK.  I've raised bug 1192366 on the dom-storage2-changed glitch.