Marco Bonardo [:mak] Assignee
	Description • 13 years ago

Looks like not all services are properly forcing that.

	Marco Bonardo [:mak] 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".

	Kyle Huey (Exited; not receiving bugmail, old account, do not use)
	Comment 2 • 13 years ago

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.

	Benjamin Smedberg
	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.

	Marco Bonardo [:mak] 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?

	:Ms2ger (he/him; ⌚ UTC+1/+2)
	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?

	Marco Bonardo [:mak] Assignee
	Comment 6 • 13 years ago

(In reply to Ms2ger from comment #5)
> Any reason for the parens?

No, I will remove them.

	Marco Bonardo [:mak] Assignee
	Comment 7 • 13 years ago

https://hg.mozilla.org/integration/mozilla-inbound/rev/df86f29bd1a3

	Marco Bonardo [:mak] Assignee
	Comment 8 • 13 years ago

https://hg.mozilla.org/mozilla-central/rev/df86f29bd1a3

	Marco Bonardo [:mak] 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

	Alex Keybl [:akeybl]
	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.

	Marco Bonardo [:mak] Assignee
	Comment 11 • 13 years ago

https://hg.mozilla.org/releases/mozilla-aurora/rev/ba73c174bac6

	Marco Bonardo [:mak] 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

	neil@parkwaycc.co.uk
	Comment 13 • 13 years ago

It's annoying that you have to make a self-reference back to your component...

	Marco Bonardo [:mak] 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.