Closed
Bug 283129
Opened 19 years ago
Closed 19 years ago
mechanism for preserving JS properties on elements is bad for GC
Categories
(Core :: DOM: Core & HTML, defect, P1)
Core
DOM: Core & HTML
Tracking
()
RESOLVED
FIXED
mozilla1.8beta2
People
(Reporter: dbaron, Assigned: dbaron)
References
Details
(Keywords: arch, memory-leak, Whiteboard: [patch])
Attachments
(3 files, 7 obsolete files)
2.72 KB,
patch
|
Details | Diff | Splinter Review | |
1.07 KB,
text/html; charset=UTF-8
|
Details | |
38.99 KB,
patch
|
jst
:
review+
brendan
:
superreview+
|
Details | Diff | Splinter Review |
The problem of how to deal with garbage collection in a system where: 1. the GC runtime contains lazily-created wrappers for other data structures not participating in the GC, 2. where those data structures are reachable from each other in a way that is reflected through the wrappers, and 3. where the wrappers can carry additional information that is not in the wrapped object is a difficult one. XPConnect is such a system. It creates wrappers around native objects that do not participate in the JS GC and which are reachable from each other outside of the knowledge of the JS engine (but through getters or functions that are accessible to JS users) and it allows these wrappers to be decorated with JS properties. It is worth noting one very important use case for our toolkit: XBL, which uses JS properties on the bound element for storing fields, methods, etc. Our current solution to it is far from ideal, and takes two forms (I'll call them A and B), each of which has a major problem: Form A: For DOM nodes (this includes the XBL case), the instant a JS property is set on the wrapper, we call nsDocument::AddReference on the element. This roots the wrapper for the lifetime of the document, which ensures that the wrapper is not garbage-collected, even if the node is removed from the document and thus unreachable by both the native and the JS data structures. Problem with Form A: This effectively defeats the point of garbage collection, and its implications on garbage collection are quite painful for things like <tabbrowser>, which is a complex XBL binding whose members refer to large data structures. Form B: For everything else, we let users set properties on wrappers but do nothing to protect those wrappers from garbage collection. Problem with Form B: This means that we're leaving things wide open for users of our APIs to write code that relies on setting JS properties but breaks when GC happens at certain points. The solution to problem B is simple, assuming it's not too big a break with API compatibility: for objects where we don't preserve JS properties set on wrappers, we simply shouldn't allow those properties to be set in the first place. This at least protects users of our toolkit and web authors (for example, those dealing with stylesheet objects) from dealing with hard-to-reproduce GC-related bugs. The solution to problem A is much harder, and that's what this bug is about. I don't see any way to solve this without incorporating knowledge about reachability within the underlying data structures into the Mark phase of the GC. The only saving grace here is that we can optimize based on the assumption that reachability in the is usually transitive. In fact, if we only care about DOM nodes, it's always transitive. Thus we can optimize an algorithm that would require walking the entire graph by picking a reference node for each strongly connected component (i.e., component of the graph in which reachibility is transitive). For DOM nodes, the obvious choice is the result of walking up the |parentNode| chain (either a Document, a DocumentFragment, or some other Node that's not attached to one of those). Doing this would require that XPConnect expose or use (perhaps via nsIClassInfo) an API for special mark operations. (JS already does -- the mark member of JSObjectOps.) The table currently maintained by nsDocument::AddReference (perhaps now in nsJSEnvironment?) would maintain a list of all the wrappers associated with each reference node and a mark bit for that list (which could be cleared for each new GC in DOMGCCallback). When one of the wrappers was marked, the special mark callback would use an API (via nsIClassInfo?) for mapping wrapped object to reference node, map reference node to list of wrappers, and if the mark bit on that list was unset, set the mark bit and mark all the wrappers. (And, if we were dealing with a set of objects where reachability was not exactly transitive, it would also ask for the set of reachable strongly connected components and mark those components as well.) This is only a very rough sketch. Is it crazy? I think we do need to look at solving this problem.
Assignee | ||
Comment 1•19 years ago
|
||
(In reply to comment #0) > The table currently maintained by nsDocument::AddReference > (perhaps now in nsJSEnvironment?) would maintain a list of all the wrappers > associated with each reference node and a mark bit for that list Oops, it would only need to track the wrappers on which JS properties were set.
Assignee | ||
Comment 2•19 years ago
|
||
Brendan pointed out that I wrote transitive where I meant symmetric.
Comment 3•19 years ago
|
||
Let me rewrite the proposal to see if I understand it correctly: XPCOM objects that wish to allow arbitrary JS properties set on them should implement an interface (nsIXPCGCParticipant) which will be called during GC to mark objects which are reachable through that XPCOM object. If an XPCOM object does not implement this interface, code will not be allowed to set "custom" JS properties on it. 1) What sort of rules will the nsIXPCGCParticipant.mark method have to prevent xpconnect reentrancy? 2) What about XPCOM objects implemented in JS? Is it important to allow custom properties on these? Will xpconnect implement nsIXPCGCParticipant for them?
What forms of "XPConnect reentrancy" do you think we need to prevent? We should probably make a list, if it's a concern.
Assignee | ||
Comment 5•19 years ago
|
||
(In reply to comment #3) > 2) What about XPCOM objects implemented in JS? Is it important to allow custom > properties on these? Will xpconnect implement nsIXPCGCParticipant for them? If they're double-wrapped, I think the answer is no.
so, dom extensibility comes to mind. currently i can't seem to add properties to window.sidebar, but perhaps i should be able to, or perhaps for xmlhttprequest or some other random dom extension that is implemented in js.
Assignee | ||
Comment 7•19 years ago
|
||
We'll need the asymmetric reachibility handling to fix bug 241518, I think.
Blocks: 241518
Assignee | ||
Comment 8•19 years ago
|
||
I've started poking at patching this (after some discussions with jst over the past week).
Assignee | ||
Comment 9•19 years ago
|
||
This doesn't get to all of the callers of nsDocument::AddReference, so it doesn't compile yet. It also doesn't really leave a good way open to solving the event listener issues (bug 241518).
Assignee | ||
Comment 10•19 years ago
|
||
A testcase is to apply this patch and see if the ++DOMWINDOW leaks caused by this patch when opening and closing tabs are fixed by the GC changes.
Assignee | ||
Comment 11•19 years ago
|
||
This passes the testcase (attachment 178057 [details] [diff] [review]), and doesn't seem to crash in a few minutes. The change I made to provide a hook for XBL to nsDOMClassInfo (via nsIScriptGlobalObject, which unfortunately has 3 implementations) is a bit ugly. I definitely want to put some printfs in and make sure that this is actually doing what I think it is...
Attachment #178023 -
Attachment is obsolete: true
Assignee | ||
Comment 12•19 years ago
|
||
Assignee | ||
Comment 13•19 years ago
|
||
With debugging printfs as well.
Attachment #178059 -
Attachment is obsolete: true
Comment 14•19 years ago
|
||
Comment on attachment 178059 [details] [diff] [review] patch >+void >+nsDOMClassInfo::MarkPreservedWrappersFor(nsIDOMNode *aDOMNode, JSContext *cx, >+ void *arg) >+{ >+ // Magic value indicating we've had OOM earlier in this GC. >+ if (sWrapperSCCTable.ops == (void*)1) >+ return; >+ >+ if (!sWrapperSCCTable.ops) { >+ if (!nsDOMClassInfo::SortWrappers()) { >+ sWrapperSCCTable.ops = (PLDHashTableOps*)1; >+ >+ // We're out of memory, so just mark all the preserved wrappers. >+ // It won't help the GC free up memory, but there's nothing better >+ // to do. >+ MarkAllWrappersData data; >+ data.cx = cx; >+ data.arg = arg; >+ PL_DHashTableEnumerate(&sPreservedWrapperTable, MarkAllWrappers, &data); Don't you want to return here? >+ } Hmm, or here -- or combine if (A) if (B) {...} into if (A && B) {...; return;} ? >+ } >+ >+ WrapperSCCEntry *entry = NS_STATIC_CAST(WrapperSCCEntry*, >+ PL_DHashTableOperate(&sWrapperSCCTable, GetSCCRootFor(aDOMNode), >+ PL_DHASH_LOOKUP)); If you need a JSGC_MARK_BEGIN notification for DOMGCCallback, just let me know. It looks like laziness here means less code, though -- if so, the comment could note that win and lack of requirement for JSGC_MARK_BEGIN? /be
Assignee | ||
Comment 15•19 years ago
|
||
Better function names and comments.
Attachment #178067 -
Attachment is obsolete: true
Assignee | ||
Comment 16•19 years ago
|
||
Oops, didn't see brendan's comment until after submitting the previous patch. Missing return added, and nested ifs merged. (In reply to comment #14) > If you need a JSGC_MARK_BEGIN notification for DOMGCCallback, just let me know. > It looks like laziness here means less code, though -- if so, the comment > could note that win and lack of requirement for JSGC_MARK_BEGIN? It's not less code, but it's not more either, so I don't think there's any need.
Attachment #178071 -
Attachment is obsolete: true
Assignee | ||
Comment 17•19 years ago
|
||
I'm wondering if these reachability tests miss some reachability via XBL APIs, or perhaps something else. (There's probably nothing we could do about C++ code holding on to something for a long time, though.)
Assignee | ||
Comment 18•19 years ago
|
||
This uses a simpler hack to give XBL access to nsDOMClassInfo::PreserveWrapper and fixes some missing ops checks on sPreservedWrapperTable.
Attachment #178073 -
Attachment is obsolete: true
Comment 19•19 years ago
|
||
Comment on attachment 178073 [details] [diff] [review] patch - In dom/public/nsIScriptGlobalObject.h: + + virtual nsresult PreserveWrapper(nsIDOMNode *aDOMNode, + nsIXPConnectWrappedNative *aWrapper) = 0; As you suggested (in person) you might just want to not do this and have the callers of this simply #include nsDOMClassInfo.h directly. - In dom/src/base/nsDOMClassInfo.cpp: +static nsIDOMNode* GetSCCRootFor(nsIDOMNode *aDOMNode) +{ + nsCOMPtr<nsIDOMNode> cur(aDOMNode), next; + for (;;) { + cur->GetParentNode(getter_AddRefs(next)); + if (!next) + return cur; + next.swap(cur); + } +} This, and maybe some other code in this patch could be *faster* if you'd use nsIContent in favor of nsIDOMNode to save cycles on refcounting etc, but you'll probably pay in code size due to the fact that you'll need to deal with the fact that documents don't implement nsIContent. That way you you wouldn't always need to walk up the tree to find the root if the node is in a document (GetCurrentDoc()), but since XUL is broken and lies about the current doc, you'd need to special-case XUL here. May very well not be worth it... + // The JS engine doesn't have a notification for the beginning of + // the mark phase. However, we can determine that the mark phase + // has begun by detecting the first mark and lazily call + // BeginGCMark. Doesn't JSGC_BEGIN work as a notification of the mark phase beginning in this case? Not that it matters since this works just as well. - In nsNodeSH::AddProperty(): + // XXX What happens if this fails? + return nsDOMClassInfo::PreserveWrapper(node, wrapper); XPConnect will convert the return value into a JS exception, so this should be fine. - In content/base/public/nsIDocument.h: - virtual void AddReference(void *aKey, nsISupports *aReference) = 0; - virtual already_AddRefed<nsISupports> RemoveReference(void *aKey) = 0; You should bump the IID in nsIDocument.h since you're changing the interface here... r=jst, thanks for fixing.
Attachment #178073 -
Flags: review+
Assignee | ||
Comment 20•19 years ago
|
||
(In reply to comment #19) > Doesn't JSGC_BEGIN work as a notification of the mark phase beginning in this > case? Not that it matters since this works just as well. No, since the mark phase could begin multiple times (if finalization triggers another GC). Also, I think there are some XBL-related problems caused by GetSCCRootFor being too simple, so I'll probably be posting a new patch once I figure that out...
Assignee | ||
Comment 21•19 years ago
|
||
This mainly contains a little more debugging code I used to convince myself that there weren't problems with anonymous content.
Attachment #178159 -
Attachment is obsolete: true
Attachment #178315 -
Flags: superreview?(brendan)
Attachment #178315 -
Flags: review?(jst)
Assignee | ||
Updated•19 years ago
|
Assignee: general → dbaron
Priority: -- → P1
Whiteboard: [patch]
Target Milestone: --- → mozilla1.8beta2
Assignee | ||
Updated•19 years ago
|
Attachment #178315 -
Flags: superreview?(brendan)
Attachment #178315 -
Flags: review?(jst)
Assignee | ||
Comment 22•19 years ago
|
||
OK, why don't I address jst's previous round of comments before requesting reviews again? :-)
Attachment #178315 -
Attachment is obsolete: true
Attachment #178318 -
Flags: superreview?(brendan)
Attachment #178318 -
Flags: review?(jst)
Comment 23•19 years ago
|
||
(In reply to comment #0) > Thus we can optimize an algorithm that would require walking the entire graph by > picking a reference node for each strongly connected component (i.e., component > of the graph in which reachibility is transitive). For DOM nodes, the obvious > choice is the result of walking up the |parentNode| chain (either a Document, a > DocumentFragment, or some other Node that's not attached to one of those). Will this fail for attribute nodes? Their parentNode is null even when they're attached to an element. It might not be a problem now because they're not cached, but we're looking into changing that (see bug 235512).
Assignee | ||
Comment 24•19 years ago
|
||
Yeah, it isn't a problem now (and in fact, given the current code, we're better off leaving it since it reduces leaks), but it would be a problem once that's fixed.
Comment 25•19 years ago
|
||
Comment on attachment 178318 [details] [diff] [review] patch +#ifdef DEBUG_dbaron +#define DEBUG_PRESERVE_WRAPPERS +#endif Me too! Please add DEBUG_jst to that this :) r=jst
Attachment #178318 -
Flags: review?(jst) → review+
Assignee | ||
Comment 26•19 years ago
|
||
Filed bug 288004 on the attributes issue (comment 23 and comment 24).
Assignee | ||
Comment 27•19 years ago
|
||
I renamed |SortWrapper| to |ClassifyWrapper| since it's not really a sort. (Anyone have any better ideas?)
Comment 28•19 years ago
|
||
Comment on attachment 178318 [details] [diff] [review] patch I wonder whether we should expose PreserveWrapper on some interface that XBL could use. I'm also wondering whether http://www-2.cs.cmu.edu/~roc/HetGC.html sheds light. sr=me. /be
Attachment #178318 -
Flags: superreview?(brendan) → superreview+
Assignee | ||
Comment 29•19 years ago
|
||
I tried exposing PreserveWrapper on an interface (see comment 11), but it wasn't worth it. Fix checked in to trunk, 2005-03-29 15:26 -0800.
Status: NEW → RESOLVED
Closed: 19 years ago
Resolution: --- → FIXED
Assignee | ||
Comment 30•19 years ago
|
||
I just filed bug 310620 on "Problem with Form B" from comment 0.
Comment 31•15 years ago
|
||
This documentation page [1] still has a link to this bug as if it was a problem. Since it's now FIXED, can someone with expertise on this update that page? Thanks. [1] https://developer.mozilla.org/en/Using_XPCOM_in_JavaScript_without_leaking
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
•