501577 - Possible leak in nsXPCWrappedJS::GetNewOrUsed

Status: RESOLVED FIXED

(Core :: XPConnect, defect)

Description

[Blake Kaplan (:mrbkap) (inactive)]

I noticed this trying to figure out the leak caused by bug 500931. bent and I also looked at a similar leak when I was trying to land SOWs and the result was a hack in nsXPCWrappedJSClass::GetRootJSObject to unwrap the return value.

The relationship between the two bugs is that we're wrapping one JS object whose root is another JS object that does not (yet) have a wrapper. Inspecting the control flow in nsXPCWrappedJS::GetNewOrUsed, I noticed that in the case described above, we'll create a new nsXPCWrappedJS (via new) and pass it to the constructor of the actual wrapped JS, which addrefs it *again*. This leaves the root object with 3 references, which is 1 too high.

Comment 1

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: Proposed fix

Peter, what do you think?

Comment 2

[Peter Van der Beken [:peterv]]

Comment on attachment 386213 [details] [diff] [review]
Proposed fix

The only thing I'd maybe do is do the release just after where we create the non-root wrapper. Ideally we'd use nsRefPtr's, only do one addref in the nsXPCWrappedJS constructor and use forget in the right places. Maybe file a follow-up bug to clean it up?

Comment 3

[Blake Kaplan (:mrbkap) (inactive)]

I moved the release before the early return if !wrapper but after the return_wrapper label to be resilient in the face of new early returns that get added.

Comment 4

[Blake Kaplan (:mrbkap) (inactive)]

http://hg.mozilla.org/mozilla-central/rev/ff4c166b63d6

Comment 5

[Samuel Sidler (old account; do not CC)]

Not going to block on this, but I'll approve it.

Comment 6

[Samuel Sidler (old account; do not CC)]

(That is, if you ask for approval having tested that it applies.)

Comment 7

[Samuel Sidler (old account; do not CC)]

Comment on attachment 386213 [details] [diff] [review]
Proposed fix

Approved for 1.9.1.2. a=ss for release-drivers

Please land on mozilla-1.9.1 and use the ".2-fixed" option of the "status1.9.1" flag.

Comment 8

[Blake Kaplan (:mrbkap) (inactive)]

I should have noted this dependency so long ago...

Comment 9

[Blake Kaplan (:mrbkap) (inactive)]

http://hg.mozilla.org/releases/mozilla-1.9.1/rev/7a1e9c849dac

Comment 10

[Tony Chung [:tchung]]

Please advise on how this can be verified on fx3.5.2.  Thanks.

Comment 11

[Daniel Veditz [:dveditz]]

Does this patch apply to 1.9.0 or do we need a back-port?

Comment 12

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: Fix for the 1.9.0 branch

Comment 13

[Daniel Veditz [:dveditz]]

Comment on attachment 393114 [details] [diff] [review]
Fix for the 1.9.0 branch

a190

Comment 14

[Daniel Veditz [:dveditz]]

by which I meant "Approved for 1.9.0.14, a=dveditz for release-drivers"

Comment 15

[Blake Kaplan (:mrbkap) (inactive)]

Checking in js/src/xpconnect/src/xpcJSWeakReference.cpp; new revision: 1.4; Checking in js/src/xpconnect/tests/Makefile.in; new revision: 1.36; previous revision: 1.35
Checking in js/src/xpconnect/tests/mochitest/Makefile.in; new revision: 1.10; previous revision: 1.9
Checking in js/src/xpconnect/tests/mochitest/bug500931_helper.html; initial revision: 1.1
Checking in js/src/xpconnect/tests/chrome/Makefile.in; initial revision: 1.1
Checking in js/src/xpconnect/tests/chrome/test_bug500931.xul; initial revision: 1.1
done