Closed
Bug 722254
Opened 13 years ago
Closed 13 years ago
Ensure Places js services can't be instanced multiple times through createInstance.
Categories
(Toolkit :: Places, defect)
Toolkit
Places
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)
15.14 KB,
patch
|
benjamin
:
review+
akeybl
:
approval-mozilla-aurora+
|
Details | Diff | Splinter Review |
Looks like not all services are properly forcing that.
Assignee | ||
Updated•13 years ago
|
Summary: Ensure Places js services can't be instanced with createInstance. → Ensure Places js services can't be instanced multiple times throug createInstance.
Assignee | ||
Comment 1•13 years ago
|
||
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 3•13 years ago
|
||
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+
Assignee | ||
Comment 4•13 years ago
|
||
(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•13 years ago
|
||
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?
Assignee | ||
Comment 6•13 years ago
|
||
(In reply to Ms2ger from comment #5)
> Any reason for the parens?
No, I will remove them.
Assignee | ||
Comment 7•13 years ago
|
||
Target Milestone: --- → mozilla13
Assignee | ||
Comment 8•13 years ago
|
||
Status: ASSIGNED → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
Assignee | ||
Comment 9•13 years ago
|
||
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?
Assignee | ||
Updated•13 years ago
|
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 10•13 years ago
|
||
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+
Assignee | ||
Comment 11•13 years ago
|
||
Assignee | ||
Updated•13 years ago
|
status-firefox12:
--- → fixed
Assignee | ||
Comment 12•13 years ago
|
||
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
Assignee | ||
Updated•13 years ago
|
Whiteboard: [see also the discussion in bug 716163]
Comment 13•13 years ago
|
||
It's annoying that you have to make a self-reference back to your component...
Assignee | ||
Comment 14•13 years ago
|
||
(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.
Description
•