Closed Bug 722428 Opened 8 years ago Closed 8 years ago

Fix leaks in SpecialPowers

Categories

(Testing :: Mochitest, defect)

defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED
mozilla14

People

(Reporter: khuey, Assigned: khuey)

Details

Attachments

(1 file, 1 obsolete file)

Attached patch Patch (obsolete) — Splinter Review
No description provided.
Attachment #592816 - Attachment is patch: true
Attachment #592816 - Flags: review?(ctalbert)
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!
Attachment #592816 - Flags: review?(ctalbert) → review+
https://hg.mozilla.org/mozilla-central/rev/f410bdf30132
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
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"
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Assignee: nobody → khuey
Version: unspecified → Trunk
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|.
Attached patch PatchSplinter Review
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.
Attachment #592816 - Attachment is obsolete: true
Attachment #593055 - Flags: review?(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.
Attachment #593055 - Flags: review?(mounir) → review+
http://hg.mozilla.org/mozilla-central/rev/447ede53509a
Status: REOPENED → RESOLVED
Closed: 8 years ago8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla14
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.
You need to log in before you can comment on or make changes to this bug.