Bug 112608 – Objective-C API: Need a good way to preserve custom properties on JS wrappers

• Status: RESOLVED
• Resolution: FIXED
• Component: JavaScriptCore

[Mark Hahnenberg]

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.

[Mark Hahnenberg]

<rdar://problem/12991582>

[Mark Hahnenberg]

Created attachment 193659 [details]
Patch

[Early Warning System Bot]

Comment on attachment 193659 [details]
Patch

Attachment 193659 [details] did not pass qt-ews (qt):
Output: http://webkit-commit-queue.appspot.com/results/17245005

[Early Warning System Bot]

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

[Mark Hahnenberg]

Created attachment 193667 [details]
Patch

[Early Warning System Bot]

Comment on attachment 193667 [details]
Patch

Attachment 193667 [details] did not pass qt-ews (qt):
Output: http://webkit-commit-queue.appspot.com/results/17210523

[Early Warning System Bot]

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

[Mark Hahnenberg]

Created attachment 193674 [details]
Patch

[Build Bot]

Comment on attachment 193674 [details]
Patch

Attachment 193674 [details] did not pass mac-ews (mac):
Output: http://webkit-commit-queue.appspot.com/results/17238211

[Mark Hahnenberg]

Created attachment 193712 [details]
Patch

[Geoffrey Garen]

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?

[Mark Hahnenberg]

Created attachment 194389 [details]
Patch

[Geoffrey Garen]

Comment on attachment 194389 [details]
Patch

r=me

[WebKit Review Bot]

Comment on attachment 194389 [details]
Patch

Clearing flags on attachment: 194389

Committed r146558: <http://trac.webkit.org/changeset/146558>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[David Kilzer (:ddkilzer)]

(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>

[David Kilzer (:ddkilzer)]

(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.

[David Kilzer (:ddkilzer)]

(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.

[David Kilzer (:ddkilzer)]

(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.

[David Kilzer (:ddkilzer)]

(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>

[Mark Hahnenberg]

(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!