As a security precaution, we have turned on the setting "Require API key authentication for API requests" for everyone. If this has broken something, please contact bugzilla-admin@mozilla.org
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 User image 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 User image 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 User image 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 User image 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 User image 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 User image 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 User image 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 User image 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 User image 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 User image 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 User image 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 User image 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 User image Rafael Ávila de Espíndola (:espindola) (not reading bugmail) 2011-11-28 13:15:26 PST
ping
Comment 13 User image Honza Bambas (:mayhemer) 2011-11-29 05:15:05 PST
I'll do your reviews soon, currently occupied with more priority stuff..
Comment 14 User image (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 User image Honza Bambas (:mayhemer) 2011-11-30 11:31:09 PST
I plan on reviewing this today.
Comment 16 User image 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 User image 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 User image 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.