697988 - We should use weak references to observers in nsDOMStorage.cpp

Status: RESOLVED FIXED

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

Description

[Rafael Ávila de Espíndola (:espindola) (not reading bugmail)]

Currently we add but never remove observers. We should use weak references instead.

Comment 1

[Rafael Ávila de Espíndola (:espindola) (not reading bugmail)]

Attachment: se weak references to observers in nsDOMStorage.cpp

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

Comment 2

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

Does nsDOMStorageManager hold on to anything interesting that would cause large amounts of memory to be held alive (e.g. Windows, documents, etc)?

Comment 3

[Marco Bonardo [:mak]]

(In reply to Kyle Huey [:khuey] ([e-mail]) 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 4

[Marco Bonardo [:mak]]

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.

Comment 5

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

Just trying to get a feeling of how important this is from a MemShrink perspective.

Comment 6

[Honza Bambas (:mayhemer)]

(In reply to Kyle Huey [:khuey] ([e-mail]) 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.

Comment 7

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

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

Comment 8

[Honza Bambas (:mayhemer)]

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.

Comment 9

[Marco Bonardo [:mak]]

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.

Comment 10

[Rafael Ávila de Espíndola (:espindola) (not reading bugmail)]

Attachment: use weak references to observers in nsDOMStorage.cpp

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

Comment 11

[Marco Bonardo [:mak]]

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.

Comment 12

[Rafael Ávila de Espíndola (:espindola) (not reading bugmail)]

ping

Comment 13

[Honza Bambas (:mayhemer)]

I'll do your reviews soon, currently occupied with more priority stuff..

Comment 14

[(dormant account)]

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

Comment 15

[Honza Bambas (:mayhemer)]

I plan on reviewing this today.

Comment 16

[Honza Bambas (:mayhemer)]

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

Comment 17

[Rafael Ávila de Espíndola (:espindola) (not reading bugmail)]

https://tbpl.mozilla.org/?tree=Mozilla-Inbound&rev=414534199a74

Comment 18

[Marco Bonardo [:mak]]

https://hg.mozilla.org/mozilla-central/rev/414534199a74