722428 - Fix leaks in SpecialPowers

Status: RESOLVED FIXED

(Testing :: Mochitest, defect)

Description

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Attachment: Patch

Comment 1

[cmtalbert]

Comment on attachment 592816 [details] [diff] [review]
Patch

Thanks for figuring this one out.  This all looks good, but I am surprised to see that we have two snapshotWindow's in this code. Would you mind filing a followup bug to remove one of them? (or just doing it here, if you feel so inclined).

It's been way too long since I've looked back at all this stuff.

Thanks Kyle!

Comment 2

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

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

Comment 3

[Phil Ringnalda (:philor)]

Backed out in https://hg.mozilla.org/mozilla-central/rev/e7386241a147 - Exception... "Illegal operation on WrappedNative prototype object" nsresult: "0x8057000c (NS_ERROR_XPC_BAD_OP_ON_WN_PROTO)" location: "JS frame :: chrome://specialpowers/content/specialpowersAPI.js :: <TOP_LEVEL> :: line 83"

Comment 4

[Serge Gautherie (:sgautherie)]

Comment on attachment 592816 [details] [diff] [review]
Patch

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

::: testing/mochitest/tests/SimpleTest/specialpowersAPI.js
@@ +78,5 @@
>      if (prop in desc && typeof(desc[prop]) == "function") {
>        var oldval = desc[prop];
>        try {
> +        desc[prop] = function() {
> +          utils = weakUtil.get();

Fwiw, 'utils' seems to miss a |var|.

Comment 5

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Attachment: Patch

This turned out to be harder than expected.  Components.utils.getWeakReference gets a reference to the JSObject, not the underlying native object.  Since nsDOMWindowUtils doesn't preserve its wrapper, the JSObject can get GCd while the underlying native object is alive.  This caused the test failures on tinderbox.

Instead of using weakrefs on nsDOMWindowUtils, lets just switch around the ownership to a more sane approach.  Asking mounir for review on the C++ side, carrying forward r=ctalbert on the JS side.

Comment 6

[Mounir Lamouri (:mounir)]

Comment on attachment 593055 [details] [diff] [review]
Patch

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

r=me with the comments fixed.

Please, ask for another review for the tests (can be the same patch with only the tests adde).

::: dom/base/nsDOMWindowUtils.cpp
@@ +906,5 @@
>                                     nsIDOMElement** aReturn)
>  {
> +  nsCOMPtr<nsPIDOMWindow> window = do_QueryReferent(mWindow);
> +  if (!window)
> +    return nsnull;

Return NS_OK or NS_ERROR_FAILURE or whatever but not nsnull.

NOTE: this apply to a lot of other places below. For ease of reading, I will not point them all.

@@ +1056,5 @@
>    }
>  
> +  nsCOMPtr<nsPIDOMWindow> window = do_QueryReferent(mWindow);
> +  if (!window)
> +    return nsnull;

You might want to remove this check. The NS_ENSURE_TRUE just below is doing that for you. In addition, you don't want to return |nsnull| here.

::: dom/base/nsGlobalWindow.cpp
@@ +8418,5 @@
> +      mWindowUtils = new nsDOMWindowUtils(this);
> +    }
> +
> +    *aSink = mWindowUtils;
> +    NS_ADDREF(((nsISupports *) *aSink));

Could you use static_cast?

::: js/xpconnect/tests/chrome/Makefile.in
@@ +75,5 @@
>  		test_getweakmapkeys.xul \
>  		test_weakmaps.xul \
>  		test_bug706301.xul \
>  		test_watchpoints.xul \
> +		test_weakref.xul \

This file seems to be missing.

Comment 7

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

http://hg.mozilla.org/mozilla-central/rev/447ede53509a

Comment 8

[[e-mail]]

Comment on attachment 593055 [details] [diff] [review]
Patch

> nsDOMWindowUtils::nsDOMWindowUtils(nsGlobalWindow *aWindow)
> {
>+  nsCOMPtr<nsISupports> supports = do_QueryObject(aWindow);
Might want to consider changing the type of aWindow to e.g. nsISupportsWeakReference to avoid this rigmarole.