The default bug view has changed. See this FAQ.

We should use weak references to observers in nsDOMStorage.cpp

RESOLVED FIXED in mozilla11

Status

()

Core
DOM
RESOLVED FIXED
6 years ago
5 years ago

People

(Reporter: espindola, Assigned: espindola)

Tracking

(Blocks: 1 bug)

unspecified
mozilla11
x86_64
Linux
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [Snappy:P1])

Attachments

(1 attachment, 1 obsolete attachment)

Currently we add but never remove observers. We should use weak references instead.
Depends on: 697989
Whiteboard: [MemShrink]
Created attachment 571013 [details] [diff] [review]
se weak references to observers in nsDOMStorage.cpp

https://tbpl.mozilla.org/?tree=Try&rev=9c4cea00a17e
Assignee: nobody → respindola
Status: NEW → ASSIGNED
Attachment #571013 - Flags: review?(mak77)
Attachment #571013 - Flags: feedback?(honzab.moz)
Does nsDOMStorageManager hold on to anything interesting that would cause large amounts of memory to be held alive (e.g. Windows, documents, etc)?
(In reply to Kyle Huey [:khuey] (khuey@mozilla.com) from comment #2)
> Does nsDOMStorageManager hold on to anything interesting that would cause
> large amounts of memory to be held alive (e.g. Windows, documents, etc)?

From the code doesn't look like, and it seems to also participate in cycle collection, so this may not have an interesting effect, unless there is some unexpected cycle. Do you have something specific in mind?
Comment on attachment 571013 [details] [diff] [review]
se weak references to observers in nsDOMStorage.cpp

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

::: dom/src/storage/nsDOMStorage.cpp
@@ +291,4 @@
>    // Used for temporary table flushing
> +  os->AddObserver(gStorageManager, "profile-before-change", true);
> +  os->AddObserver(gStorageManager, NS_XPCOM_SHUTDOWN_OBSERVER_ID, true);
> +  os->AddObserver(gStorageManager, NS_DOMSTORAGE_FLUSH_TIMER_OBSERVER, true);

the newly added topic from bug 697989 should be converted as well.
Just trying to get a feeling of how important this is from a MemShrink perspective.
Whiteboard: [MemShrink] → [MemShrink:P3]
(In reply to Kyle Huey [:khuey] (khuey@mozilla.com) from comment #5)
> Just trying to get a feeling of how important this is from a MemShrink
> perspective.

IMO, absolutely in no way.  This is just about a code cleanup and correct removal of observers.  It may, though, protect from leaks in the future, but honestly, as I have tested, there are no _shutdown_ leaks in the current code anyway.  There is no potential for massive run-time leaks.
(In reply to Honza Bambas (:mayhemer) from comment #6)
> There is no potential for massive run-time leaks.

Yeah, that's the part we were interested in.
Whiteboard: [MemShrink:P3]
Comment on attachment 571013 [details] [diff] [review]
se weak references to observers in nsDOMStorage.cpp

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

Push the new version to try or tell us and somebody can do that.  That is precondition to land the patch.

::: dom/src/storage/nsDOMStorage.cpp
@@ +291,4 @@
>    // Used for temporary table flushing
> +  os->AddObserver(gStorageManager, "profile-before-change", true);
> +  os->AddObserver(gStorageManager, NS_XPCOM_SHUTDOWN_OBSERVER_ID, true);
> +  os->AddObserver(gStorageManager, NS_DOMSTORAGE_FLUSH_TIMER_OBSERVER, true);

+1

And also, since this has changed, check on rv after each call to AddObserver.  The code will be a bit larger, but we will be 100% sure this works.
Attachment #571013 - Flags: feedback?(honzab.moz) → feedback+
Comment on attachment 571013 [details] [diff] [review]
se weak references to observers in nsDOMStorage.cpp

Please address Honza's requests, then I'll gladly r+ this.
Attachment #571013 - Flags: review?(mak77)
Depends on: 702275
Created attachment 576188 [details] [diff] [review]
use weak references to observers in nsDOMStorage.cpp

https://tbpl.mozilla.org/?tree=Try&rev=b885ac241691
Attachment #571013 - Attachment is obsolete: true
Attachment #576188 - Flags: review?(mak77)
Attachment #576188 - Flags: feedback?(honzab.moz)
Comment on attachment 576188 [details] [diff] [review]
use weak references to observers in nsDOMStorage.cpp

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

looks good to me. Btw, since honza is a more valid peer than me on this code, I'm inverting the requests.
Attachment #576188 - Flags: review?(mak77)
Attachment #576188 - Flags: review?(honzab.moz)
Attachment #576188 - Flags: feedback?(honzab.moz)
Attachment #576188 - Flags: feedback+
ping
I'll do your reviews soon, currently occupied with more priority stuff..

Updated

5 years ago
Whiteboard: [Snappy:P1]

Comment 14

5 years ago
(In reply to Honza Bambas (:mayhemer) from comment #13)
> I'll do your reviews soon, currently occupied with more priority stuff..

Honza,
This is blocking progress on 662444 which is a high priority bug with regard to mozilla responsiveness.
I plan on reviewing this today.
Blocks: 662444
Comment on attachment 576188 [details] [diff] [review]
use weak references to observers in nsDOMStorage.cpp

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

r=honzab, try run is green
Attachment #576188 - Flags: review?(honzab.moz) → review+
https://tbpl.mozilla.org/?tree=Mozilla-Inbound&rev=414534199a74
https://hg.mozilla.org/mozilla-central/rev/414534199a74
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla11
You need to log in before you can comment on or make changes to this bug.