Last Comment Bug 722254 - Ensure Places js services can't be instanced multiple times through createInstance.
: Ensure Places js services can't be instanced multiple times through createIns...
Status: RESOLVED FIXED
[see also the discussion in bug 71616...
: dev-doc-needed
Product: Toolkit
Classification: Components
Component: Places (show other bugs)
: unspecified
: All All
: -- normal (vote)
: mozilla13
Assigned To: Marco Bonardo [::mak]
:
:
Mentors:
Depends on:
Blocks: 674475 716163
  Show dependency treegraph
 
Reported: 2012-01-30 02:20 PST by Marco Bonardo [::mak]
Modified: 2012-03-29 14:57 PDT (History)
6 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
fixed


Attachments
patch v1.0 (15.14 KB, patch)
2012-01-30 08:52 PST, Marco Bonardo [::mak]
benjamin: review+
akeybl: approval‑mozilla‑aurora+
Details | Diff | Splinter Review

Description Marco Bonardo [::mak] 2012-01-30 02:20:53 PST
Looks like not all services are properly forcing that.
Comment 1 Marco Bonardo [::mak] 2012-01-30 08:52:15 PST
Created attachment 592734 [details] [diff] [review]
patch v1.0

so, this should not have bad effects regarding compatibility.
though, well the test may conflict with your changes (easy to remove that part) and globally your changes are more "global".
Comment 2 Kyle Huey [:khuey] (Exited; not receiving bugmail, email if necessary) 2012-01-30 09:02:04 PST
Comment on attachment 592734 [details] [diff] [review]
patch v1.0

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

I skimmed this and it looks fine, but I think bsmedberg should review it.
Comment 3 Benjamin Smedberg [:bsmedberg] 2012-02-06 07:25:19 PST
Comment on attachment 592734 [details] [diff] [review]
patch v1.0

Hrm, we're relying on final GC to clean up this object... I wonder if we should be doing that or explicitly clearing it on xpcom-shutdown? I guess this is ok.
Comment 4 Marco Bonardo [::mak] 2012-02-06 07:32:52 PST
(In reply to Benjamin Smedberg  [:bsmedberg] from comment #3)
> Hrm, we're relying on final GC to clean up this object... I wonder if we
> should be doing that or explicitly clearing it on xpcom-shutdown? I guess
> this is ok.

Should I file a bug to track this concern?
Comment 5 :Ms2ger (⌚ UTC+1/+2) 2012-02-06 07:36:35 PST
Comment on attachment 592734 [details] [diff] [review]
patch v1.0

>--- a/js/xpconnect/loader/XPCOMUtils.jsm
>+++ b/js/xpconnect/loader/XPCOMUtils.jsm
>+        return (this._instance).QueryInterface(aIID);

Any reason for the parens?
Comment 6 Marco Bonardo [::mak] 2012-02-06 07:42:43 PST
(In reply to Ms2ger from comment #5)
> Any reason for the parens?

No, I will remove them.
Comment 8 Marco Bonardo [::mak] 2012-02-07 15:04:13 PST
https://hg.mozilla.org/mozilla-central/rev/df86f29bd1a3
Comment 9 Marco Bonardo [::mak] 2012-02-08 08:31:38 PST
Comment on attachment 592734 [details] [diff] [review]
patch v1.0

[Approval Request Comment]
Regression caused by (bug #): no regression
User impact if declined: add-ons using Places can cause large memory usage and GB-sized leaks, up to OOM. See bug 716163 for a case, surely other add-ons exist using a similar path.
Testing completed (on m-c, etc.): m-c
Risk to taking this patch (and alternatives if risky): The patch is relatively simple, the alternative is to just delay the fix to FF13.
String changes made by this patch: none
Comment 10 Alex Keybl [:akeybl] 2012-02-09 13:20:59 PST
Comment on attachment 592734 [details] [diff] [review]
patch v1.0

[Triage Comment]
Given where we are in the cycle, and the possible OOM effects, approving for Aurora 12.
Comment 12 Marco Bonardo [::mak] 2012-02-14 03:57:46 PST
I assume this should be documented in https://developer.mozilla.org/en/How_to_Build_an_XPCOM_Component_in_Javascript#Using_XPCOMUtils
Comment 13 neil@parkwaycc.co.uk 2012-02-15 03:42:12 PST
It's annoying that you have to make a self-reference back to your component...
Comment 14 Marco Bonardo [::mak] 2012-02-15 04:43:34 PST
(In reply to neil@parkwaycc.co.uk from comment #13)
> It's annoying that you have to make a self-reference back to your
> component...

Ideally there could be a way to annotate the component as a service in the manifest and it may automatically generate the singleton without having to add anything.
Btw, if you have suggestions to improve this they are welcome.

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