They're supposed to delete themselves when only the IPDL reference is left, but that doesn't appear to happen until the browser app is closed. This means bad leaks of cached keys and values in the parent process until that occurs.
I investigated what happens when reloading google.com in a b2g desktop build with OOP enabled. It appears that there's a known cycle between StorageChild::mStorage and nsDOMStorage::mStorageImpl (they're both declared correctly in the cycle collection macros), but there's also the persistent IPDL reference. This means that the special case in StorageChild::Release (check if the refcount is 1 and mIPCOpen is true) doesn't kick in, so the IPDL reference never goes away and the refcount remains at 2 indefinitely. This is an idiom we use in a bunch of places, which is slightly worrying.
Created attachment 742654 [details] [diff] [review] Invetigative hacks. Here's something I tried, but it worked a lot worse than I expected. Most of the additions in nsGlobalWindow don't appear to make any difference; I saw maybe one in five StorageChild objects being destroyed by a call to MarkOwnerDead triggered by the cycle collector unlinking, and the rest just lived on seemingly indefinitely. Someone like bz would probably be better able to understand these results.
Thanks jdm! Enjoy your vacation :)
Is it possible to debug this on desktop builds? I guess it is, but I wonder how well test-ipc.xul works in this case.
Olli do you want to take this?
> Is it possible to debug this on desktop builds? I'd imagine so. You need to flip a pref, though. Search for "nice things" in b2g.js.
Blocking a blocker
Josh, seems like you've done some work here so giving this tef+ bug to you but please un-assign yourself if you can't take it from here. (In reply to Gregor Wagner [:gwagner] from comment #7) > Blocking a blocker Yep, tef+
I'm back on the case.
Created attachment 746583 [details] [diff] [review] Invetigative hacks. My previous patch was nearly there, but wasn't quite right. This one seems to have pretty good non-leaking characteristics; we're still at the mercy of the cycle collector, unfortunately, but in my tests we don't tend to retain more than two or three previous instances when refreshing the page every few seconds.
Note that I've only looked into this on the b2g18 branch; I haven't checked what the IPC situation is like on m-c yet.
Comment on attachment 746583 [details] [diff] [review] Invetigative hacks. Could you please add some helper method to call MarkOwnerDead on local/session storage and then just use that in dtor/unlink/InnerSetNewDocument. The method should also set the member variable to null. This is for b2g18, right? Since nsDOMStorage coding style is still the old one. I guess no need to change it here.
For info: this bug is not valid for m-c (there is no actor attached to the window, only a per-process global actor communicating with the database in the parent). If releasing global windows works as expected on child processes, then there is no leak.
Comment on attachment 746583 [details] [diff] [review] Invetigative hacks. Review of attachment 746583 [details] [diff] [review]: ----------------------------------------------------------------- Thanks, this is a good patch.
(In reply to Honza Bambas (:mayhemer) from comment #13) > For info: this bug is not valid for m-c (there is no actor attached to the > window, only a per-process global actor communicating with the database in > the parent). If releasing global windows works as expected on child > processes, then there is no leak. To extend a bit on this: there is no cycle relation between DOMStorage and underlying objects. Leaks and good release management was the most important (and complicated, however simplistically implemented) part of the new design.