Closed Bug 722254 Opened 9 years ago Closed 9 years ago

Ensure Places js services can't be instanced multiple times through createInstance.

Categories

(Toolkit :: Places, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla13
Tracking Status
firefox12 --- fixed

People

(Reporter: mak, Assigned: mak)

References

Details

(Keywords: dev-doc-needed, Whiteboard: [see also the discussion in bug 716163][qa-])

Attachments

(1 file)

Looks like not all services are properly forcing that.
Blocks: 716163
Summary: Ensure Places js services can't be instanced with createInstance. → Ensure Places js services can't be instanced multiple times throug createInstance.
Attached patch patch v1.0Splinter Review
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".
Attachment #592734 - Flags: review?(khuey)
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.
Attachment #592734 - Flags: review?(khuey) → review?(benjamin)
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.
Attachment #592734 - Flags: review?(benjamin) → review+
(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 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?
(In reply to Ms2ger from comment #5)
> Any reason for the parens?

No, I will remove them.
https://hg.mozilla.org/mozilla-central/rev/df86f29bd1a3
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
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
Attachment #592734 - Flags: approval-mozilla-aurora?
Blocks: 674475
Summary: Ensure Places js services can't be instanced multiple times throug createInstance. → Ensure Places js services can't be instanced multiple times through createInstance.
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.
Attachment #592734 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Whiteboard: [see also the discussion in bug 716163]
It's annoying that you have to make a self-reference back to your component...
(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.
Whiteboard: [see also the discussion in bug 716163] → [see also the discussion in bug 716163][qa-]
You need to log in before you can comment on or make changes to this bug.