300562 - XPCNativeWrappers can end up holding refs to other XPCNativeWrappers for too long

Status: RESOLVED FIXED

(Core :: XPConnect, defect, P1)

Description

[Boris Zbarsky [:bzbarsky]]

See bug 295937 comment 1, bug 295937 comment 4, bug 295937 comment 5

Comment 1

[Brendan Eich [:brendan]]

Attachment: proposed fix, v2

Thoughts: could we optimize the attribute case by using specialized, thinner
get and set hooks?  Should we add JSPROP_READONLY if the attribute is readonly?


/be

Comment 2

[Boris Zbarsky [:bzbarsky]]

That patch fixes the issue for me.

Comment 3

[Brendan Eich [:brendan]]

Reviewers flush your queues! ;-)

/be

Comment 4

[Johnny Stenback (:jst)]

Comment on attachment 189140 [details] [diff] [review]
proposed fix, v2

r=jst

Comment 5

[Mike Shaver (:shaver -- probably not reading bugmail closely)]

Comment on attachment 189140 [details] [diff] [review]
proposed fix, v2

sr=shaver.  I think we should JSPROP_READONLY if the property is readonly, but
we can do that in a follow-on.

Comment 6

[Brendan Eich [:brendan]]

Comment on attachment 189140 [details] [diff] [review]
proposed fix, v2

self-approving, we need this for XPCNativeWrapper victory in 1.8/1.1.

/be

Comment 7

[Brendan Eich [:brendan]]

Fixed.  Not sure about the JSPROP_READONLY -- do I need a followup bug?  I'll
file something, or wiki at least.

/be

Comment 8

[Brendan Eich [:brendan]]

jst points out that we want to try setting the wn attribute from the nw, so that
we get high-fidelity XPConnect exception-throwing on readonly attribute, instead
of bad ol' ECMA silent-failure-to-mutate-but-return-the-RHS-value-to-confuse-you
behavior.

/be

Comment 9

[Boris Zbarsky [:bzbarsky]]

So... any idea why luna went orange?

Comment 10

[Brendan Eich [:brendan]]

No.  I could back out and see if it goes green again.  Anyone have access and
the know-how to extract why the DHTMLTest: test failed?

/be

Comment 11

[Brendan Eich [:brendan]]

Backed out, we'll see what luna does.

/be

Comment 12

[Brendan Eich [:brendan]]

Grrr, why would this break the DHTMLTest?  I'll try to make time (new baby is
nigh so I may not be able to).

/be

Comment 13

[Boris Zbarsky [:bzbarsky]]

Attachment: Testcase that's broken by that patch for some reason...

brendan, I ran into some major weirdness that I traced to this patch... if you
load this testcase in a build with the patch and follow directions, the second
click on the button does nothing.  For some reason, the XPCWrappedNative that
has the event handler defined on it gets garbage collected when that second
window is opened... In fact, XPCWrappedNative preservation seems broken in
general with this patch applied.  :(

(And yes, I meant XPCWrappedNative above, not XPCNativeWrapper.  Why this patch
affects XPCWrappedNatives, I do not know.)

Comment 14

[Boris Zbarsky [:bzbarsky]]

OK, I think I know what's going on here...   I filed bug 301316 on the problem.

Comment 15

[Brendan Eich [:brendan]]

Attachment: proposed fix, v3

Testing help welcome, too.

/be

Comment 16

[Mike Shaver (:shaver -- probably not reading bugmail closely)]

Comment on attachment 189822 [details] [diff] [review]
proposed fix, v3

r=shaver.

Comment 17

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 189822 [details] [diff] [review]
proposed fix, v3

This patch doesn't fix the issue... when running with it, I get nativeExists
false in almost all cases (including resolves of "firstChild", "nextSibling",
etc, in particular).

Comment 18

[Boris Zbarsky [:bzbarsky]]

Checked in "proposed fix, v2" for brendan as requested.

Comment 19

[Brendan Eich [:brendan]]

I guess this was left for me to close.

/be