Closed
Bug 295937
Opened 19 years ago
Closed 19 years ago
Adding properties on an XPCNativeWrapper doesn't preserve the wrapper
Categories
(Core :: DOM: Core & HTML, defect, P1)
Core
DOM: Core & HTML
Tracking
()
RESOLVED
FIXED
mozilla1.8beta4
People
(Reporter: bzbarsky, Assigned: bzbarsky)
References
Details
Attachments
(2 files, 1 obsolete file)
5.24 KB,
patch
|
Details | Diff | Splinter Review | |
13.58 KB,
patch
|
shaver
:
review+
brendan
:
superreview+
brendan
:
approval1.8b4+
|
Details | Diff | Splinter Review |
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?
Assignee | ||
Comment 1•19 years ago
|
||
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•19 years ago
|
||
(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
Assignee | ||
Comment 3•19 years ago
|
||
> 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....
Assignee | ||
Comment 4•19 years ago
|
||
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•19 years ago
|
||
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
Assignee | ||
Comment 6•19 years ago
|
||
Brendan, that patch fixes the issue I was seeing.
Assignee | ||
Comment 7•19 years ago
|
||
I filed bug 300562 on the referencing issue so this can stay focused on lifetime of XPCNativeWrappers with expandos.
Depends on: 300562
Assignee | ||
Comment 8•19 years ago
|
||
This should do the right thing (certainly does in my testing).
Attachment #189257 -
Flags: superreview?(brendan)
Attachment #189257 -
Flags: review?(shaver)
Assignee | ||
Comment 9•19 years ago
|
||
Attachment #189257 -
Attachment is obsolete: true
Attachment #189736 -
Flags: superreview?(brendan)
Attachment #189736 -
Flags: review?(shaver)
Assignee | ||
Updated•19 years ago
|
Attachment #189257 -
Flags: superreview?(brendan)
Attachment #189257 -
Flags: review?(shaver)
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?
Attachment #189736 -
Flags: review?(shaver) → review+
Comment 11•19 years ago
|
||
+JSBool MaybePreserveWrapper(JSContext* cx, XPCWrappedNative *wn, uintN flags) should probably be +JSBool MaybePreserveWrapper(JSContext *cx, XPCWrappedNative *wn, uintN flags)
Assignee | ||
Comment 12•19 years ago
|
||
> 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•19 years ago
|
||
Comment on attachment 189736 [details] [diff] [review] jst prefers this approach Nice, sr+a=me. /be
Attachment #189736 -
Flags: superreview?(brendan)
Attachment #189736 -
Flags: superreview+
Attachment #189736 -
Flags: approval1.8b4+
Assignee | ||
Updated•19 years ago
|
Assignee: general → bzbarsky
Priority: -- → P1
Target Milestone: --- → mozilla1.8beta4
Assignee | ||
Comment 14•19 years ago
|
||
Fixed.
Status: NEW → RESOLVED
Closed: 19 years ago
Resolution: --- → FIXED
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).
Assignee | ||
Comment 16•19 years ago
|
||
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.
Updated•5 years ago
|
Component: DOM → DOM: Core & HTML
You need to log in
before you can comment on or make changes to this bug.
Description
•