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

RESOLVED FIXED in Firefox 12

Status

()

Toolkit
Places
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: mak, Assigned: mak)

Tracking

({dev-doc-needed})

unspecified
mozilla13
dev-doc-needed
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox12 fixed)

Details

(Whiteboard: [see also the discussion in bug 716163][qa-])

Attachments

(1 attachment)

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.
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".
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/integration/mozilla-inbound/rev/df86f29bd1a3
Target Milestone: --- → mozilla13
https://hg.mozilla.org/mozilla-central/rev/df86f29bd1a3
Status: ASSIGNED → RESOLVED
Last Resolved: 6 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+
https://hg.mozilla.org/releases/mozilla-aurora/rev/ba73c174bac6
status-firefox12: --- → fixed
I assume this should be documented in https://developer.mozilla.org/en/How_to_Build_an_XPCOM_Component_in_Javascript#Using_XPCOMUtils
Keywords: dev-doc-needed
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.