Bug 112608 - Objective-C API: Need a good way to preserve custom properties on JS wrappers
Summary: Objective-C API: Need a good way to preserve custom properties on JS wrappers
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Mark Hahnenberg
URL:
Keywords:
Depends on:
Blocks: 113054
  Show dependency treegraph
 
Reported: 2013-03-18 12:39 PDT by Mark Hahnenberg
Modified: 2013-03-22 08:24 PDT (History)
6 users (show)

See Also:


Attachments
Patch (30.25 KB, patch)
2013-03-18 14:50 PDT, Mark Hahnenberg
no flags Details | Formatted Diff | Diff
Patch (33.34 KB, patch)
2013-03-18 15:09 PDT, Mark Hahnenberg
no flags Details | Formatted Diff | Diff
Patch (36.79 KB, patch)
2013-03-18 15:31 PDT, Mark Hahnenberg
no flags Details | Formatted Diff | Diff
Patch (36.85 KB, patch)
2013-03-18 18:35 PDT, Mark Hahnenberg
no flags Details | Formatted Diff | Diff
Patch (22.27 KB, patch)
2013-03-21 17:01 PDT, Mark Hahnenberg
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Mark Hahnenberg 2013-03-18 12:39:08 PDT
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.
Comment 1 Mark Hahnenberg 2013-03-18 12:39:40 PDT
<rdar://problem/12991582>
Comment 2 Mark Hahnenberg 2013-03-18 14:50:43 PDT
Created attachment 193659 [details]
Patch
Comment 3 Early Warning System Bot 2013-03-18 15:00:03 PDT
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 4 Early Warning System Bot 2013-03-18 15:03:40 PDT
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
Comment 5 Mark Hahnenberg 2013-03-18 15:09:41 PDT
Created attachment 193667 [details]
Patch
Comment 6 Early Warning System Bot 2013-03-18 15:21:07 PDT
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 7 Early Warning System Bot 2013-03-18 15:28:39 PDT
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
Comment 8 Mark Hahnenberg 2013-03-18 15:31:26 PDT
Created attachment 193674 [details]
Patch
Comment 9 Build Bot 2013-03-18 18:21:34 PDT
Comment on attachment 193674 [details]
Patch

Attachment 193674 [details] did not pass mac-ews (mac):
Output: http://webkit-commit-queue.appspot.com/results/17238211
Comment 10 Mark Hahnenberg 2013-03-18 18:35:55 PDT
Created attachment 193712 [details]
Patch
Comment 11 Geoffrey Garen 2013-03-19 14:30:32 PDT
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?
Comment 12 Mark Hahnenberg 2013-03-21 17:01:27 PDT
Created attachment 194389 [details]
Patch
Comment 13 Geoffrey Garen 2013-03-21 17:45:52 PDT
Comment on attachment 194389 [details]
Patch

r=me
Comment 14 WebKit Review Bot 2013-03-21 20:15:14 PDT
Comment on attachment 194389 [details]
Patch

Clearing flags on attachment: 194389

Committed r146558: <http://trac.webkit.org/changeset/146558>
Comment 15 WebKit Review Bot 2013-03-21 20:15:18 PDT
All reviewed patches have been landed.  Closing bug.
Comment 16 David Kilzer (:ddkilzer) 2013-03-22 06:37:37 PDT
(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>
Comment 17 David Kilzer (:ddkilzer) 2013-03-22 06:56:50 PDT
(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.
Comment 18 David Kilzer (:ddkilzer) 2013-03-22 07:08:10 PDT
(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.
Comment 19 David Kilzer (:ddkilzer) 2013-03-22 07:12:45 PDT
(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.
Comment 20 David Kilzer (:ddkilzer) 2013-03-22 07:25:09 PDT
(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>
Comment 21 Mark Hahnenberg 2013-03-22 08:24:48 PDT
(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!