Last Comment Bug 697988 - We should use weak references to observers in nsDOMStorage.cpp
: We should use weak references to observers in nsDOMStorage.cpp
Status: RESOLVED FIXED
[Snappy:P1]
:
Product: Core
Classification: Components
Component: DOM (show other bugs)
: unspecified
: x86_64 Linux
: -- normal (vote)
: mozilla11
Assigned To: Rafael Ávila de Espíndola (:espindola) (not reading bugmail)
:
: Andrew Overholt [:overholt]
Mentors:
Depends on: 697989 702275
Blocks: 662444
  Show dependency treegraph
 
Reported: 2011-10-28 08:05 PDT by Rafael Ávila de Espíndola (:espindola) (not reading bugmail)
Modified: 2011-12-01 04:44 PST (History)
6 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
se weak references to observers in nsDOMStorage.cpp (3.28 KB, patch)
2011-11-01 08:28 PDT, Rafael Ávila de Espíndola (:espindola) (not reading bugmail)
honzab.moz: feedback+
Details | Diff | Splinter Review
use weak references to observers in nsDOMStorage.cpp (3.63 KB, patch)
2011-11-22 09:20 PST, Rafael Ávila de Espíndola (:espindola) (not reading bugmail)
honzab.moz: review+
mak77: feedback+
Details | Diff | Splinter Review

Description Rafael Ávila de Espíndola (:espindola) (not reading bugmail) 2011-10-28 08:05:13 PDT
Currently we add but never remove observers. We should use weak references instead.
Comment 1 Rafael Ávila de Espíndola (:espindola) (not reading bugmail) 2011-11-01 08:28:39 PDT
Created attachment 571013 [details] [diff] [review]
se weak references to observers in nsDOMStorage.cpp

https://tbpl.mozilla.org/?tree=Try&rev=9c4cea00a17e
Comment 2 Kyle Huey [:khuey] (Exited; not receiving bugmail, email if necessary) 2011-11-01 12:27:45 PDT
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] 2011-11-03 07:46:04 PDT
(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 4 Marco Bonardo [::mak] 2011-11-03 07:47:43 PDT
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 [:khuey] (Exited; not receiving bugmail, email if necessary) 2011-11-03 08:18:38 PDT
Just trying to get a feeling of how important this is from a MemShrink perspective.
Comment 6 Honza Bambas (:mayhemer) 2011-11-03 09:38:44 PDT
(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.
Comment 7 Kyle Huey [:khuey] (Exited; not receiving bugmail, email if necessary) 2011-11-03 09:41:10 PDT
(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) 2011-11-03 12:52:53 PDT
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] 2011-11-04 16:11:03 PDT
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) 2011-11-22 09:20:57 PST
Created attachment 576188 [details] [diff] [review]
use weak references to observers in nsDOMStorage.cpp

https://tbpl.mozilla.org/?tree=Try&rev=b885ac241691
Comment 11 Marco Bonardo [::mak] 2011-11-22 16:10:43 PST
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) 2011-11-28 13:15:26 PST
ping
Comment 13 Honza Bambas (:mayhemer) 2011-11-29 05:15:05 PST
I'll do your reviews soon, currently occupied with more priority stuff..
Comment 14 (dormant account) 2011-11-30 11:28:12 PST
(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) 2011-11-30 11:31:09 PST
I plan on reviewing this today.
Comment 16 Honza Bambas (:mayhemer) 2011-11-30 14:33:44 PST
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) 2011-11-30 16:10:20 PST
https://tbpl.mozilla.org/?tree=Mozilla-Inbound&rev=414534199a74
Comment 18 Marco Bonardo [::mak] 2011-12-01 04:44:38 PST
https://hg.mozilla.org/mozilla-central/rev/414534199a74

Note You need to log in before you can comment on or make changes to this bug.