Closed Bug 490718 Opened 12 years ago Closed 12 years ago

XPCWrappedNativeScope creates a needless cycle with its principal provider

Categories

(Core :: XPConnect, defect)

x86
macOS
defect
Not set
normal

Tracking

()

RESOLVED FIXED

People

(Reporter: bent.mozilla, Assigned: bent.mozilla)

Details

Attachments

(1 file)

Attached patch PatchSplinter Review
We could just use a raw ptr, patch attached.
Attachment #375102 - Flags: superreview?(jst)
Attachment #375102 - Flags: review?(peterv)
Attachment #375102 - Flags: review?(peterv) → review+
Comment on attachment 375102 [details] [diff] [review]
Patch

>diff --git a/js/src/xpconnect/src/xpcwrappednativescope.cpp b/js/src/xpconnect/src/xpcwrappednativescope.cpp

>@@ -447,17 +452,19 @@ XPCWrappedNativeScope::FinishedMarkPhase

>+#ifndef XPCONNECT_STANDALONE
>+            cur->mScriptObjectPrincipal = nsnull;
>+#endif
>             // Move this scope from the live list to the dying list.

Nit: add a newline after the endif.
Comment on attachment 375102 [details] [diff] [review]
Patch

Duh, totally forgot about this one :(
Attachment #375102 - Flags: superreview?(jst) → superreview+
Comment on attachment 375102 [details] [diff] [review]
Patch

I'd love to get this in for 1.9.1 as it helps windows get collected sooner.
Needs to bake on trunk before we'd consider for 191, which means that it might be fodder for a security and stability release, though!
Flags: wanted1.9.1.x?
Attachment #375102 - Flags: approval1.9.1?
Comment on attachment 375102 [details] [diff] [review]
Patch

Please request approval when it has baked on trunk.
Oops, I landed this yesterday, changeset 6dd95b1cba8e.
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Comment on attachment 375102 [details] [diff] [review]
Patch

Sorry guys, I neglected to indicate that I had landed already. Dunno how much baking you'd want this to have, though I think we would have noticed right away if this patch caused problems.
Ben: any idea of risk here?
Attachment #375102 - Flags: approval1.9.1? → approval1.9.1-
Comment on attachment 375102 [details] [diff] [review]
Patch

Should ask again for 3.5.1, but I don't know if we understand the full risk here. Feel free to renom if you're willing to let us know.
You need to log in before you can comment on or make changes to this bug.