If you think a bug might affect users in the 57 release, please set the correct tracking and status flags for Release Management.

Fix leak of PStorage actor when reloading a page

RESOLVED FIXED in 1.0.1 IOT1 (10may)

Status

()

Core
DOM
RESOLVED FIXED
5 years ago
4 years ago

People

(Reporter: jdm, Assigned: jdm)

Tracking

18 Branch
1.0.1 IOT1 (10may)
x86_64
Linux
Points:
---

Firefox Tracking Flags

(blocking-b2g:tef+, firefox21 wontfix, firefox22 wontfix, firefox23 unaffected, b2g18 fixed, b2g18-v1.0.0 wontfix, b2g18-v1.0.1 fixed)

Details

(Whiteboard: [MemShrink:P1])

Attachments

(1 attachment, 1 obsolete attachment)

(Assignee)

Description

5 years ago
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.
Whiteboard: [MemShrink]
(Assignee)

Updated

5 years ago
Version: unspecified → 18 Branch
(Assignee)

Comment 1

5 years ago
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.
(Assignee)

Comment 2

5 years ago
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 :)

Comment 4

5 years ago
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?
Whiteboard: [MemShrink] → [MemShrink:P1]
> 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
blocking-b2g: --- → tef?
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+
Assignee: nobody → josh
blocking-b2g: tef? → tef+
(Assignee)

Comment 9

4 years ago
I'm back on the case.
(Assignee)

Comment 10

4 years ago
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.
Attachment #746583 - Flags: review?(bugs)
(Assignee)

Updated

4 years ago
Attachment #742654 - Attachment is obsolete: true
(Assignee)

Updated

4 years ago
Attachment #746583 - Flags: review?(honzab.moz)
(Assignee)

Comment 11

4 years ago
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.
Attachment #746583 - Flags: review?(bugs) → review+
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.
Attachment #746583 - Flags: review?(honzab.moz) → review+
(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.
(Assignee)

Comment 16

4 years ago
https://hg.mozilla.org/releases/mozilla-b2g18_v1_0_1/rev/8a72a7cf4a42
https://hg.mozilla.org/releases/mozilla-b2g18/rev/5d0de39b146d
status-b2g18: --- → fixed
status-b2g18-v1.0.0: --- → wontfix
status-b2g18-v1.0.1: --- → fixed
status-firefox23: --- → wontfix
Target Milestone: --- → 1.0.1 IOT1 (10may)
(Assignee)

Updated

4 years ago
Status: NEW → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
status-firefox21: --- → wontfix
status-firefox22: --- → verified
status-firefox23: wontfix → unaffected
status-firefox22: verified → wontfix
You need to log in before you can comment on or make changes to this bug.