Currently, we just use a weak map, which means that garbage collection can cause a wrapper to disappear if it isn't directly referenced from JavaScript. The most straightforward and safe way (with respect to garbage collection and concurrency) is to have clients add and remove their external references along with their owners. Effectively, the client is recording the structure of the external object graph so that the garbage collector can make sure to mark any wrappers that are reachable through either the JS object graph of the external Obj-C object graph. By keeping these wrappers alive, this has the effect that custom properties on these wrappers will also remain alive. The rule for if an object needs to be tracked by the runtime (and therefore whether the client should report it) is as follows: For a particular object, its references to its children should be added if: (1) The child is referenced from JavaScript. (2) The child contains references to other objects for which (1) or (2) are true.
<rdar://problem/12991582>
Created attachment 193659 [details] Patch
Comment on attachment 193659 [details] Patch Attachment 193659 [details] did not pass qt-ews (qt): Output: http://webkit-commit-queue.appspot.com/results/17245005
Comment on attachment 193659 [details] Patch Attachment 193659 [details] did not pass qt-wk2-ews (qt): Output: http://webkit-commit-queue.appspot.com/results/17236142
Created attachment 193667 [details] Patch
Comment on attachment 193667 [details] Patch Attachment 193667 [details] did not pass qt-ews (qt): Output: http://webkit-commit-queue.appspot.com/results/17210523
Comment on attachment 193667 [details] Patch Attachment 193667 [details] did not pass qt-wk2-ews (qt): Output: http://webkit-commit-queue.appspot.com/results/17202356
Created attachment 193674 [details] Patch
Comment on attachment 193674 [details] Patch Attachment 193674 [details] did not pass mac-ews (mac): Output: http://webkit-commit-queue.appspot.com/results/17238211
Created attachment 193712 [details] Patch
Comment on attachment 193712 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=193712&action=review r=me > Source/JavaScriptCore/ChangeLog:20 > + 1. The child is referenced from JavaScript. Let's say, "The child is exported to JavaScript". > Source/JavaScriptCore/API/JSManagedReference.h:39 > ++ (void)addManagedReference:(id)object withOwner:(id)owner toVirtualMachine:(JSVirtualMachine *)virtualMachine; > ++ (void)removeManagedReference:(id)object withOwner:(id)owner fromVirtualMachine:(JSVirtualMachine *)virtualMachine; Let's put these methods on JSVirtualMachine. Since we never create a JSManagedReference object, it's probably clearer that way. > Source/JavaScriptCore/API/JSManagedReference.mm:93 > +void scanExternalObjectGraph(JSC::JSGlobalData& globalData, JSC::SlotVisitor& visitor, void* root) Let's use a WTF data structure for the stack here, since it's so much more efficient. Let's use a weak NSMapTable for the ownedObjects set to avoid retaining things longer than necessary if the client forgets to call removeManagedReference. Let's put counting in the set, so you can call this in a multiple owner style. > Source/JavaScriptCore/heap/SlotVisitorInlines.h:124 > +inline bool SlotVisitor::threadSafeContainsOpaqueRoot(void* root) "Thread safe" doesn't fully describe the requirements here. How about containsOpaqueRootTriState(), returning either TrueTriStae or MixedTriState, but never FalseTriState, since we can never say for sure that other threads haven't locally added this thing?
Created attachment 194389 [details] Patch
Comment on attachment 194389 [details] Patch r=me
Comment on attachment 194389 [details] Patch Clearing flags on attachment: 194389 Committed r146558: <http://trac.webkit.org/changeset/146558>
All reviewed patches have been landed. Closing bug.
(In reply to comment #14) > (From update of attachment 194389 [details]) > Clearing flags on attachment: 194389 > > Committed r146558: <http://trac.webkit.org/changeset/146558> Follow-up build fix in r146599: <http://trac.webkit.org/r146599>
(In reply to comment #16) > (In reply to comment #14) > > (From update of attachment 194389 [details] [details]) > > Clearing flags on attachment: 194389 > > > > Committed r146558: <http://trac.webkit.org/changeset/146558> > > Follow-up build fix in r146599: <http://trac.webkit.org/r146599> Reverted r146599 in r146603: <http://trac.webkit.org/r146603> Not sure why this didn't fail locally on my Mac Pro.
(In reply to comment #17) > (In reply to comment #16) > > (In reply to comment #14) > > > (From update of attachment 194389 [details] [details] [details]) > > > Clearing flags on attachment: 194389 > > > > > > Committed r146558: <http://trac.webkit.org/changeset/146558> > > > > Follow-up build fix in r146599: <http://trac.webkit.org/r146599> > > Reverted r146599 in r146603: <http://trac.webkit.org/r146603> > > Not sure why this didn't fail locally on my Mac Pro. So what's happening is that -[TinyDOMNode dealloc] probably leaks on Lion since it still uses gcc to build, and gcc doesn't have a warning about not calling [super dealloc]. On other OS X versions, -[TinyDOMNode dealloc] doesn't leak because we're using clang with ARC enabled.
(In reply to comment #18) > (In reply to comment #17) > > (In reply to comment #16) > > > (In reply to comment #14) > > > > (From update of attachment 194389 [details] [details] [details] [details]) > > > > Clearing flags on attachment: 194389 > > > > > > > > Committed r146558: <http://trac.webkit.org/changeset/146558> > > > > > > Follow-up build fix in r146599: <http://trac.webkit.org/r146599> > > > > Reverted r146599 in r146603: <http://trac.webkit.org/r146603> > > > > Not sure why this didn't fail locally on my Mac Pro. > > So what's happening is that -[TinyDOMNode dealloc] probably leaks on Lion since it still uses gcc to build, and gcc doesn't have a warning about not calling [super dealloc]. On other OS X versions, -[TinyDOMNode dealloc] doesn't leak because we're using clang with ARC enabled. Filed Bug 113054 about this (relatively minor) issue.
(In reply to comment #17) > (In reply to comment #16) > > (In reply to comment #14) > > > (From update of attachment 194389 [details] [details] [details]) > > > Clearing flags on attachment: 194389 > > > > > > Committed r146558: <http://trac.webkit.org/changeset/146558> > > > > Follow-up build fix in r146599: <http://trac.webkit.org/r146599> > > Reverted r146599 in r146603: <http://trac.webkit.org/r146603> > > Not sure why this didn't fail locally on my Mac Pro. Better iOS build fix in r146604: <http://trac.webkit.org/r146604>
(In reply to comment #20) > (In reply to comment #17) > > (In reply to comment #16) > > > (In reply to comment #14) > > > > (From update of attachment 194389 [details] [details] [details] [details]) > > > > Clearing flags on attachment: 194389 > > > > > > > > Committed r146558: <http://trac.webkit.org/changeset/146558> > > > > > > Follow-up build fix in r146599: <http://trac.webkit.org/r146599> > > > > Reverted r146599 in r146603: <http://trac.webkit.org/r146603> > > > > Not sure why this didn't fail locally on my Mac Pro. > > Better iOS build fix in r146604: <http://trac.webkit.org/r146604> Thanks for tracking this down!