864941 - Fix leak of PStorage actor when reloading a page

Status: RESOLVED FIXED

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

Description

[Josh Matthews [:jdm]]

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.

Comment 1

[Josh Matthews [:jdm]]

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.

Comment 2

[Josh Matthews [:jdm]]

Attachment: 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.

Comment 3

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Thanks jdm! Enjoy your vacation :)

Comment 4

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

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.

Comment 5

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Olli do you want to take this?

Comment 6

[Justin Lebar (not reading bugmail)]

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

Comment 7

[Gregor Wagner [:gwagner]]

Blocking a blocker

Comment 8

[Andrew Overholt [:overholt]]

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+

Comment 9

[Josh Matthews [:jdm]]

I'm back on the case.

Comment 10

[Josh Matthews [:jdm]]

Attachment: 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.

Comment 11

[Josh Matthews [:jdm]]

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 12

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

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.

Comment 13

[Honza Bambas (:mayhemer)]

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 14

[Honza Bambas (:mayhemer)]

Comment on attachment 746583 [details] [diff] [review]
Invetigative hacks.

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

Thanks, this is a good patch.

Comment 15

[Honza Bambas (:mayhemer)]

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

Comment 16

[Josh Matthews [:jdm]]

https://hg.mozilla.org/releases/mozilla-b2g18_v1_0_1/rev/8a72a7cf4a42
https://hg.mozilla.org/releases/mozilla-b2g18/rev/5d0de39b146d