660237 - implement nsIDOMStorage with a proxy

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Comment 1

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Attachment: WIP

Doesn't have a deleter yet (bug 788225), and I still need to find a parent object.

Comment 2

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Attachment: WIP

Comment 3

[Honza Bambas (:mayhemer)]

So, as I understand, the object that you want to inherit from nsWrapperCache has to be the object that implements the DOM exposed API, in this case DOMStorage implementing nsIDOMStorage.

What you are missing is the parent object.  The parent object should be held and found in DOMStorage, not in underlying DOMStorageCache, IMO.  Thus, we should extend the DOMStorageManager API to accept window argument.  DOMStorage must keep a weak ptr to the window, otherwise we have a cycle and I would like to avoid CC on the DOMStorage unless there is no other way around.  Both sessionStorage and localStorage objects (both DOMStorage) are cached, i.e. hard referenced by their nsGlobalWindows, so it's safe to do weak ref in reverse order (nsWeakReference can be used).

If we don't want to modify the DOMStorageManager API, we can have a setter for window on nsPIDOMStorage interface, but that seems to me a bit cumbersome.



I'd just like to know what all the wrapper cache saves.  Are those some complicated JS constructions and invocations that may be saved or is just about to cache the same stuff also in DOM cache?  Because DOMStorage is caching very much efficiently and at a single place.  I'm worried about memory consumption and unnecessary code complication, but you know more.

Comment 4

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

The nsWrapperCache caches the JSObject reflection; it just uses one word of memory.

I don't think we can avoid CC, though, even if we make the window a weak reference. The DOMStorage will (through nsWrapperCache) keep the JSObject alive, which will keep its global object (the window) alive, and the window, as you said, keeps the DOMStorage alive.

Stepping back, though, I'm not sure we *need* to cache the wrapper, because Storage can't have expandos. Boris?

Comment 5

[Boris Zbarsky [:bzbarsky]]

Hmm.

So wrappercache is used to cache the JS object reflection, to make sure the right WrapObject method gets called in xpconnect, and possibly for something to do with weakmap keys...  Andrew, am I remembering that right?

I think you might be right that we don't need it here offhand, especially if we don't care about Storage objects being weakmap keys, if we somehow make sure that we're always doing the wrapping correctly ourselves in all the cases people care about.

Once we convert window to webidl we'll need to figure out something, though...

Comment 6

[Andrew McCreight [:mccr8]]

Yeah, if a native object can be repeatedly gotten from JS and doesn't support wrapper caching, then it will silently disappear from weak maps: if you drop all JS references to it, then grab it from the native side again, it won't be in the weak map any more.  Currently, we throw in those situations when putting things into the weakmap.

Comment 7

[Andrew McCreight [:mccr8]]

I guess mozilla::dom::TryPreserveWrapper could be modified to test for nsIDOMStorage and throw in that case...

Comment 8

[Boris Zbarsky [:bzbarsky]]

We could also resurrect https://bug812617.bugzilla.mozilla.org/attachment.cgi?id=682816 if we wanted to... assuming that hook would ever be called to start with.

Comment 9

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Attachment: WIP

Comment 10

[Honza Bambas (:mayhemer)]

Comment on attachment 755518 [details] [diff] [review]
WIP

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

::: dom/src/storage/DOMStorage.cpp
@@ +353,3 @@
>    }
>  
> +  return mCache->GetKeys(this, aKeys);

return can be removed I think ;)

Also see bug 832544.

::: dom/src/storage/DOMStorage.h
@@ +83,5 @@
> +  {
> +    // We can lie and always say we found the key, because there can be no
> +    // expando's on this object.
> +    aFound = true;
> +    RemoveItem(aKey, aRv);

For info:
If you want to stop lying, just let at [1] return rv instead.  NS_SUCCESS_DOM_NO_OPERATION then tells you the key was not found.  The same result is used on more places, see [2]

[1] http://hg.mozilla.org/mozilla-central/annotate/26ab72bfa9df/dom/src/storage/DOMStorage.cpp#l137
[2] http://mxr.mozilla.org/mozilla-central/ident?i=NS_SUCCESS_DOM_NO_OPERATION

Comment 11

[Andrea Marchesini [:baku]]

Attachment: patch 1

Comment 12

[Andrea Marchesini [:baku]]

Attachment: patch 2

There are no reasons to have 2 separated patches but this second one removes nsIDOMStorage and nsPIDOMStorage completely. I want to see if it is green on try and to know if this approach is ok. Then I'll ask for a review.

Comment 13

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Any news?

Comment 14

[Andrea Marchesini [:baku]]

Something. The main issue is that we need [Unenumerable] for having the same behaviour of the current XPConnect implementation. I'm still proceeding without it modifying the "broken" mochitests.
bz, are you OK with this or do we want to wait until [Unenumerable] is implemented?

Comment 15

[Boris Zbarsky [:bzbarsky]]

I don't think we have any plans to implement [Unenumerable] at the moment...  You should just go ahead; if we decide we need it, it'll be pretty simple to add post-facto.

Comment 16

[Andrea Marchesini [:baku]]

Attachment: WIP

we are almost there with this patch.

Comment 17

[Andrea Marchesini [:baku]]

Comment on attachment 8438427 [details] [diff] [review]
WIP

green on try: https://tbpl.mozilla.org/?tree=Try&rev=867b35dab461
Bz can you review this patch?

Comment 18

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8438427 [details] [diff] [review]
WIP

>+++ b/docshell/base/nsDocShell.cpp
> nsDocShell::AddSessionStorage(nsIPrincipal* aPrincipal,
>+    nsRefPtr<DOMStorage> storage = static_cast<DOMStorage*>(aStorage);

Why is that cast safe?  I don't think it is.  You may want to give DOMStorage an IID and QI here or something...  Or leave an empty nsIDOMStorage interface.

>+++ b/dom/base/nsGlobalWindow.cpp
>+    NS_ASSERTION(canAccess,
>+                 "window %x owned sessionStorage "

I know you just copied that, but get rid of the useless %x please?

>+    mSessionStorage = static_cast<DOMStorage*>(storage.get());

I wonder whether it'd make sense to have a noscript thing declared on storage manager that actually has a DOMStorage** outparam...  Followup OK for this part.

>+++ b/dom/imptests/failures/webapps/WebStorage/tests/submissions/Ms2ger/mochitest.ini

Can't this file go away altogether?  And possibly some of the parent directories?

>+++ b/dom/src/storage/DOMStorage.cpp

Maybe a followup to s/DOMStorage/Storage/?

>+NS_IMPL_CYCLE_COLLECTION_WRAPPERCACHE(DOMStorage, mManager, mPrincipal, mWindow)

What about mCache?

>+++ b/dom/src/storage/DOMStorage.h
>+  bool NameIsEnumerable(const nsAString& aName) const

This should be down in the "WebIDL" section.

>+  void GetSupportedNames(unsigned, nsTArray<nsString>& aKeys);

And this.

>+  void NamedDeleter(const nsAString& aKey, bool& aFound, ErrorResult& aRv)
>+    aRv = rv;

Hrm.  That's really pretty bad; we shouldn't allow copy-assignment of ErrorResult!

In any case, why can you not just pass aRv to RemoveItem() here?

>+++ b/dom/src/storage/DOMStorageManager.cpp
>+DOMStorageManager::CloneStorage(nsISupports* aStorage)
>+  nsRefPtr<DOMStorage> storage = static_cast<DOMStorage*>(aStorage);

That cast isn't safe.

> DOMStorageManager::CheckStorage(nsIPrincipal* aPrincipal,
>+  nsRefPtr<DOMStorage> storage = static_cast<DOMStorage*>(aStorage);

Nor this one.

>+++ b/dom/tests/mochitest/localstorage/test_clear_browser_data.html
>+    gApp = request. result;

Drop the extra space?

Can you explain why the changes to this test are needed, please?

>+++ b/embedding/components/windowwatcher/src/nsWindowWatcher.cpp
>+      nsRefPtr<dom::DOMStorage> storage =
>+        static_cast<dom::DOMStorage*>(supports.get());

Again, not safe...

>+      nsCOMPtr<nsIDOMWindow> window = do_QueryInterface(aParent);

Hrm.

This will pass an outer window, no?  What do the other callsites pass?  We should be consistent about the parent being the inner window imo.

>+++ b/toolkit/components/social/FrameWorkerContent.js
>+    let needsWaive = ['XMLHttpRequest', 'WebSocket', 'Worker', 'localStorage' ];

Why do we need to waive here?  At the very least document!

Definitely need the casting bits fixed, at the very least.

Comment 19

[Andrea Marchesini [:baku]]

> >+NS_IMPL_CYCLE_COLLECTION_WRAPPERCACHE(DOMStorage, mManager, mPrincipal, mWindow)
> 
> What about mCache?

mCache doesn't implement CC.

> >+++ b/dom/tests/mochitest/localstorage/test_clear_browser_data.html
> >+    gApp = request. result;
> 
> Can you explain why the changes to this test are needed, please?

Yes, the issue here is that the test takes a copy of localStorage and sessionStorage from an iframe.
Then, when the app has finished its task, with a timeout, checks() is called and in that function we try to get data from these 2 storages. But they are dead object at that time. Previously, without webidl, a wrapper was preserved.

I discussed this test with smaug. The change creates the app in order to check if localStorage and sessionStorage contain right values.

> >+++ b/toolkit/components/social/FrameWorkerContent.js
> >+    let needsWaive = ['XMLHttpRequest', 'WebSocket', 'Worker', 'localStorage' ];
> 
> Why do we need to waive here?  At the very least document!

I have to test it again. I don't remember. There was some issue with the wrapping of the object.

Comment 20

[Boris Zbarsky [:bzbarsky]]

> mCache doesn't implement CC.

Neither does mPrincipal or mManager.  But you should just CC any strong pointers you hold, so you don't leak if that changes.

> But they are dead object at that time. 

Dead in what sense?  A dead object proxy (hueyfix)?  And it used to work because we got per-origin reflectors instead of proxies?  Or something else?

Comment 21

[Andrea Marchesini [:baku]]

(In reply to Boris Zbarsky [:bz] from comment #20)
> > mCache doesn't implement CC.
> 
> Neither does mPrincipal or mManager.  But you should just CC any strong
> pointers you hold, so you don't leak if that changes.

mCache is not a nsISupports and doesn't have cycleCollection:

../../../dist/include/nsCycleCollectionNoteChild.h: In instantiation of ‘static void CycleCollectionNoteChildImpl<T, false>::Run(nsCycleCollectionTraversalCallback&, T*) [with T = mozilla::dom::DOMStorageCache]’:
../../../dist/include/nsCycleCollectionNoteChild.h:85:57:   required from ‘void CycleCollectionNoteChild(nsCycleCollectionTraversalCallback&, T*, const char*, uint32_t) [with T = mozilla::dom::DOMStorageCache; uint32_t = unsigned int]’
../../../dist/include/nsAutoPtr.h:1124:66:   required from ‘void ImplCycleCollectionTraverse(nsCycleCollectionTraversalCallback&, nsRefPtr<T>&, const char*, uint32_t) [with T = mozilla::dom::DOMStorageCache; uint32_t = unsigned int]’
/home/baku/Sources/m/foobar/src/dom/src/storage/DOMStorage.cpp:29:1201:   required from here
../../../dist/include/nsCycleCollectionNoteChild.h:74:74: error: no type named ‘cycleCollection’ in ‘class mozilla::dom::DOMStorageCache’
     aCallback.NoteNativeChild(aChild, NS_CYCLE_COLLECTION_PARTICIPANT(T));

BTW: for the  needsWaive, something is changed and that part of the patch is not needed.

Comment 22

[Boris Zbarsky [:bzbarsky]]

> mCache is not a nsISupports and doesn't have cycleCollection:

Hrm.  That's annoying (the fact that you can't optimistically add it to CC and have it start Just Working if it ever ends up cced).

I guess add comments to DOMStorageCache that if it's ever made cycle-collected it has to be added here.  :(

Comment 23

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

I guess we could add dummy support for refcounted objects. Some template magic which compiler would then hopefully optimize out. Then if such thing is made CCable, different template would be used and
things would just work... Though, I'm not sure this really matters. If one makes some stuff
CCable, all the classes owning such thing need to be examined anyway.

Comment 24

[Andrea Marchesini [:baku]]

Attachment: Interdiff

I'm still investigating the kind of dead objects we have in that chrome test.

Comment 25

[Andrea Marchesini [:baku]]

Attachment: WIP (2)

Comment 26

[Honza Bambas (:mayhemer)]

Please go reviews through me as well.  I'm the author of the code.  Seems like you want to do complicated change I should at least know of.

Comment 27

[Andrea Marchesini [:baku]]

Attachment: domstorage.patch

Comment 28

[Boris Zbarsky [:bzbarsky]]

Interdiff?

Comment 29

[Andrea Marchesini [:baku]]

Attachment: interdiff

Comment 30

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8440874 [details] [diff] [review]
domstorage.patch

>+++ b/docshell/base/nsDocShell.cpp
> nsDocShell::AddSessionStorage(nsIPrincipal* aPrincipal,

Why not just leave the IDL taking nsIDOMStorage so you can just null-check here instead of doing the extra QI?

In either case, please make nsIDOMStorage a builtinclass interface, since you're assuming that any nsIDOMStorage is actually a DOMStorage under the hood.

>+DOMStorageManager::CloneStorage(nsISupports* aStorage)

Again, why not just nsIDOMStorage?

> DOMStorageManager::CheckStorage(nsIPrincipal* aPrincipal,
>+                                nsISupports* aStorage,

And here.

>+++ b/dom/tests/mochitest/localstorage/frame_clear_browser_data.html

I'd use textContent here instead of innerHTML.  Same thing in test_clear_browser_data.html.

>+++ b/embedding/components/windowwatcher/src/nsWindowWatcher.cpp

Are there actually cases here when aParent is an inner window?  I wouldn't think there are, and if there are not, we should remove the code that tries to handle that, no?

r=me with those fixed.

Comment 31

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8441203 [details] [diff] [review]
interdiff

Thank you for posting this!  It helped a lot.

Comment 32

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8440874 [details] [diff] [review]
domstorage.patch

Er, one more nit.  In nsIDOMStorage.idl:

>+ * Empty interface for client side storage. DOMStorage is not ported to WEbIDL

s/not/now/ and s/WEb/Web/

Comment 33

[Andrea Marchesini [:baku]]

Attachment: domstorage.patch

Comment 34

[Andrea Marchesini [:baku]]

Attachment: domstorage.patch

Comment 35

[Andrea Marchesini [:baku]]

> Are there actually cases here when aParent is an inner window?  I wouldn't
> think there are, and if there are not, we should remove the code that tries
> to handle that, no?

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

Actually aParent can be a innerWindow and an outerWindow. I reintroduced the check:

+      nsCOMPtr<nsPIDOMWindow> pInnerWin = pWin->IsInnerWindow()
+                                            ? pWin.get()
+                                            : pWin->GetCurrentInnerWindow();

Comment 36

[Honza Bambas (:mayhemer)]

Comment on attachment 8441879 [details] [diff] [review]
domstorage.patch

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

::: dom/base/nsGlobalWindow.cpp
@@ +10437,5 @@
> +      return nullptr;
> +    }
> +
> +    mLocalStorage = static_cast<DOMStorage*>(storage.get());
> +    MOZ_ASSERT(mLocalStorage);

nice next step would be to decom the manager :)

@@ +11292,5 @@
> +      }
> +#endif
> +
> +      nsCOMPtr<nsIDOMStorage> iSessionStorage = mSessionStorage.get();
> +      fireMozStorageChanged = SameCOMIdentity(iSessionStorage, istorage);

I think now you can just compare directly DOMStorage* pointers.

@@ +11311,5 @@
>          return NS_OK;
>  
> +      nsCOMPtr<nsIDOMStorage> iLocalStorage = mLocalStorage.get();
> +      nsCOMPtr<nsIDOMStorage> iStorage = changingStorage.get();
> +      fireMozStorageChanged = SameCOMIdentity(iLocalStorage, iStorage);

as well here.

::: dom/src/storage/DOMStorage.cpp
@@ +26,5 @@
>  namespace mozilla {
>  namespace dom {
>  
> +// XXX DOMStorageCache is not CCed and it cannot be added here.
> +// This has to be fixed.

DOMStorageCache has its own life time and I don't think it should ever be considered as a CC participant or touched by the collector - IMO would harm the stability.  

DOMStorageCache never refs DOMStorage - its by design and will not change.  

Also there is never a cycle between manager - cache - storage.  It's also by design.  

Note that referencing is different for localStorage and sessionsStorage.  

* localStorage cache (for DOMStorage.type == localStorage) is never hard referenced by the manager (but hard refers the manager) 

* sessionStorage cache is always hard-reffed by the manager (and doesn't hard ref back).  All by careful design to have correct life time and to avoid unwanted cycles and leaks.

see also https://developer.mozilla.org/en-US/docs/Mozilla/Gecko/DOM_Storage_implementation_notes

@@ +121,5 @@
>        : Telemetry::SESSIONDOMSTORAGE_VALUE_SIZE_BYTES, aData.Length());
>  
>    nsString old;
> +  aRv = mCache->SetItem(this, aKey, nsString(aData), old);
> +  if (NS_WARN_IF(aRv.Failed())) {

this should not warn, it's not an error and will be either handled by JS code or left unhandled and will show up in error console (where it belongs).  but this doesn't belong to the stdout/err console.

@@ +140,5 @@
>    }
>  
>    nsAutoString old;
> +  aRv = mCache->RemoveItem(this, aKey, old);
> +  if (NS_WARN_IF(aRv.Failed())) {

as well here and elsewhere.

@@ +327,3 @@
>  {
>    if (!CanUseStorage(this)) {
> +    // return just an empty array

is there any need to clear aKeys?  if not certain, I'd rather do it

::: dom/src/storage/DOMStorage.h
@@ +45,5 @@
> +  {
> +    return mManager;
> +  }
> +
> +  const DOMStorageCache* GetCache() const

DOMStorageCache const * GetCache() const

@@ +114,5 @@
> +      aFound = false;
> +      return;
> +    }
> +
> +    aFound = true;

aFound = aRv.ErrorCode() != NS_SUCCESS_DOM_NO_OPERATION ?

::: dom/src/storage/DOMStorageManager.cpp
@@ +374,5 @@
>  
>    if (aRetval) {
> +    nsCOMPtr<nsIDOMStorage> storage = new DOMStorage(aWindow, this, cache,
> +                                                    aDocumentURI, aPrincipal,
> +                                                    aPrivate);

could you use:

nsCOMPtr<nsIDOMStorage> storage = new DOMStorage(
  aWindow, this, cache, aDocumentURI, aPrincipal, aPrivate);

?

Comment 37

[Andrea Marchesini [:baku]]

Attachment: domstorage.patch

green on try: https://tbpl.mozilla.org/?tree=Try&rev=c737d2d9e6c8

Comment 38

[Boris Zbarsky [:bzbarsky]]

https://tbpl.mozilla.org/?tree=Try&rev=e2880f9ec74e merged to tip on top of the new patch for bug 1021066 and with explicit tests for this added.

Comment 39

[Boris Zbarsky [:bzbarsky]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/326c91338df0

Comment 40

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/326c91338df0

Comment 41

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Thanks for getting this finished!

Comment 42

[Girish Sharma [:Optimizer]]

Just a note that the changes made in files

toolkit/devtools/server/tests/browser/browser_storage_dynamic_windows.js
toolkit/devtools/server/tests/browser/browser_storage_listings.js

are incorrect as a user would not want to see the prototype objects of storage in devtools. The correct approach would have been to do a for of loop on Object.keys of the storage item.

I am fixing them as part of bug 970517

Comment 43

[Jeferson Hultmann]

[addon-compat]

I've had to make some changes to my code to get it working with Firefox 34.

var oldMoz = Services.vc.compare(Services.appinfo.platformVersion, "34.0a") < 0;
var storage = oldMoz ? Services.domStorageManager.createStorage(principal, "")
                     : Services.domStorageManager.createStorage(null, principal, "");