Summary: | Objective-C API: Need a good way to preserve custom properties on JS wrappers | ||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Mark Hahnenberg <mhahnenberg> | ||||||||||||
Component: | JavaScriptCore | Assignee: | Mark Hahnenberg <mhahnenberg> | ||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||
Severity: | Normal | CC: | buildbot, ddkilzer, levin+threading, rniwa, webkit-ews, webkit.review.bot | ||||||||||||
Priority: | P2 | ||||||||||||||
Version: | 528+ (Nightly build) | ||||||||||||||
Hardware: | Unspecified | ||||||||||||||
OS: | Unspecified | ||||||||||||||
Bug Depends on: | |||||||||||||||
Bug Blocks: | 113054 | ||||||||||||||
Attachments: |
|
Description
Mark Hahnenberg
2013-03-18 12:39:08 PDT
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! |