295937 - Adding properties on an XPCNativeWrapper doesn't preserve the wrapper

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect, P1)

Description

[Boris Zbarsky [:bzbarsky]]

For XPCWrappedNatives of DOM nodes, we have some code in the AddProperty hook
that adds the XPCWrappedNative to a hashtable of wrapped natives to mark.  The
result is that such wrapped natives are not garbage collected until the wrapper
for the document of the corresponding DOM nodes is garbage collected.

We have nothing like that for XPCNativeWrapper.  So code that adds a property on
an XPCNativeWrapper may or may not find the property there later.  If the
XPCWrappedNative in question is in the above hashtable, we're ok because of the
marking back and forth between WrappedNative and NativeWrapper.  But there's no
guarantee that the wrapped native is in the hashtable.

It seems to me that the AddProperty hook on an automatic XPCNativeWrapper should
really preserve the wrapped native.  This means either making some assumptions
about what type of wrapped natives want to be preserved (anything whose
scriptable info wants marking, at the moment) or exposing some other classinfo
hooks for this.

So what sort of behavior do we want here?  And how do we want to make it happen?

Comment 1

[Boris Zbarsky [:bzbarsky]]

So I'm having two problems here:

1)  The AddProperty hook of XPCNativeWrapper is called for all sorts of things
    (eg property gets on the wrapper).
2)  I can't seem to get the wrapper to be garbage-collected to start with, not
    sure why.... so can't really test patches, really.

Comment 2

[Brendan Eich [:brendan]]

(In reply to comment #1)
> So I'm having two problems here:
> 
> 1)  The AddProperty hook of XPCNativeWrapper is called for all sorts of things
>     (eg property gets on the wrapper).

What hook do you mean?  XPCNativeWrapper::sXPC_NW_JSClass has JS_PropertyStub as
the initializer for its addProperty hook.

> 2)  I can't seem to get the wrapper to be garbage-collected to start with, not
>     sure why.... so can't really test patches, really.

What reference paths reach a wrapper you expect should be GC'd, according to
GC_MARK_DEBUG and js_LiveThingToFind?

/be

Comment 3

[Boris Zbarsky [:bzbarsky]]

> What hook do you mean?

I replaced JS_PropertyStub with an actual hook that I could then put printfs in
and such.

As for GC... the problem is that by shutdown the wrapper _does_ get GC'd.  But
in a simple "set a property on a wrapper which we don't hold on to, do a bunch
of stuff in other windows that should lead to GC (loading pages, opening/closing
windows, etc), then test whether the property is still set" testcase, the
wrapper has the property set.  I didn't get a chance to really debug last night,
but maybe I'll get a chance to today....

Comment 4

[Boris Zbarsky [:bzbarsky]]

OK, I've figured out why the wrapper wasn't being GCed.  I was accessing the
node as event.target.ownerDocument.firstChild.nextSibling.firstChild.  Now the
wrapped native for the document is always around (it's a property on the
window), so that keeps the document's wrapper around.  Now the XPCNativeWrappers
in question (for document, head, body) end up with properties defined on them,
so that I get my wrapper referenced by the chain:

  (document XPCNativeWrapper).firstChild == (head XPCNativeWrapper)
  (head XPCNativeWrapper).nextSibling == (body XPCNativeWrapper)
  (body XPCNativeWrappwe).firstChild == (the wrapper I wanted)

I switched to using document.getElementById to get the node, and now the wrapper
does get GCed.

Is there a way we can avoid defining these properties on the wrapper?  It
doesn't seem desirable to do it...

Comment 5

[Brendan Eich [:brendan]]

Attachment: fix attempt for bz to run with

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?


Anyway, the key to the fix, what avoids garbage entrainment, is JSPROP_SHARED.

/be

Comment 6

[Boris Zbarsky [:bzbarsky]]

Brendan, that patch fixes the issue I was seeing.

Comment 7

[Boris Zbarsky [:bzbarsky]]

I filed bug 300562 on the referencing issue so this can stay focused on lifetime
of XPCNativeWrappers with expandos.

Comment 8

[Boris Zbarsky [:bzbarsky]]

Attachment: Proposed patch

This should do the right thing (certainly does in my testing).

Comment 9

[Boris Zbarsky [:bzbarsky]]

Attachment: jst prefers this approach

Comment 10

[Mike Shaver (:shaver emeritus)]

Comment on attachment 189736 [details] [diff] [review]
jst prefers this approach

r=shaver.  Do we have a SWAG about what this'll do to Tp and friends?

Comment 11

[Caleb]

+JSBool MaybePreserveWrapper(JSContext* cx, XPCWrappedNative *wn, uintN flags)

should probably be

+JSBool MaybePreserveWrapper(JSContext *cx, XPCWrappedNative *wn, uintN flags)

Comment 12

[Boris Zbarsky [:bzbarsky]]

> Do we have a SWAG about what this'll do to Tp and friends?

"nothing", unless I screwed up.  The only codepath change here is when chrome
assigns an expando property to an XPCNativeWrapper, which I expect happens about
never in our default chrome.

Comment 13

[Brendan Eich [:brendan]]

Comment on attachment 189736 [details] [diff] [review]
jst prefers this approach

Nice, sr+a=me.

/be

Comment 14

[Boris Zbarsky [:bzbarsky]]

Fixed.

Comment 15

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

So did this change ever call PreserveWrapper on something like a Window?  If so,
nsWindowSH::OnFinalize needs to do what nsNodeSH::OnFinalize does (call
ReleaseWrapper).

Comment 16

[Boris Zbarsky [:bzbarsky]]

while we do call PreserveWrapper() on Window objects with this change.  But
Window doesn't implement nsIDOMNode, so the very first lines of code in
nsDOMClassInfo::PreserveWrapper make that call a no-op; as a result we're still
only preserving Node wrappers.