Bug 131634

Summary: Objective-C API external object graphs don't handle generational collection properly
Product: WebKit Reporter: Mark Hahnenberg <mhahnenberg>
Component: JavaScriptCoreAssignee: Mark Hahnenberg <mhahnenberg>
Status: RESOLVED FIXED    
Severity: Normal CC: commit-queue, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on:    
Bug Blocks: 121074    
Attachments:
Description Flags
Patch
none
Patch
none
Patch
none
Patch none

Description Mark Hahnenberg 2014-04-14 14:47:27 PDT
If the set of Objective-C objects transitively reachable through an object changes, we need to update the set of opaque roots accordingly. If we don't, the next EdenCollection won't rescan the external object graph, which would lead us to consider a newly allocated JSManagedValue to be dead.
Comment 1 Radar WebKit Bug Importer 2014-04-14 14:48:15 PDT
<rdar://problem/16612948>
Comment 2 Mark Hahnenberg 2014-04-14 14:55:10 PDT
The fix is to record new pointers from old owners to new owned objects and to rescan those "remembered" owners with scanExternalObject graph during EdenCollections.
Comment 3 Mark Hahnenberg 2014-04-15 11:16:32 PDT
Created attachment 229380 [details]
Patch
Comment 4 Mark Hahnenberg 2014-04-15 11:20:50 PDT
Created attachment 229381 [details]
Patch
Comment 5 Mark Hahnenberg 2014-04-15 11:28:59 PDT
Created attachment 229383 [details]
Patch
Comment 6 Mark Hahnenberg 2014-04-15 11:37:35 PDT
Created attachment 229385 [details]
Patch
Comment 7 Geoffrey Garen 2014-04-15 12:41:05 PDT
Comment on attachment 229385 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=229385&action=review

r=me

> Source/JavaScriptCore/API/JSVirtualMachine.mm:162
> +    [m_externalRememberedSet setObject:[NSNumber numberWithBool:true] forKey:object];

You might be able to use NSSet here with something similar to weakIDOptions.

> Source/JavaScriptCore/API/JSVirtualMachine.mm:293
> +        JSVirtualMachine *virtualMachine = [JSVMWrapperCache wrapperForJSContextGroupRef:toRef(&vm)];
> +        if (!virtualMachine)
> +            return;

Is it possible that the JSVirtualMachine will go away, by virtue of a zero retain count? If so, we'll forget all the external object graph stuff. Probably worth a follow-up bug.
Comment 8 Mark Hahnenberg 2014-04-15 13:22:26 PDT
(In reply to comment #7)
> (From update of attachment 229385 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=229385&action=review
> 
> r=me
> 
> > Source/JavaScriptCore/API/JSVirtualMachine.mm:162
> > +    [m_externalRememberedSet setObject:[NSNumber numberWithBool:true] forKey:object];
> 
> You might be able to use NSSet here with something similar to weakIDOptions.
Looks like it's nontrivial to get NSSet to do weak references, so I guess I'll stick with NSMapTable for now.

> 
> > Source/JavaScriptCore/API/JSVirtualMachine.mm:293
> > +        JSVirtualMachine *virtualMachine = [JSVMWrapperCache wrapperForJSContextGroupRef:toRef(&vm)];
> > +        if (!virtualMachine)
> > +            return;
> 
> Is it possible that the JSVirtualMachine will go away, by virtue of a zero retain count? If so, we'll forget all the external object graph stuff. Probably worth a follow-up bug.
I filed bug 131691 for this.
Comment 9 WebKit Commit Bot 2014-04-15 14:05:39 PDT
Comment on attachment 229385 [details]
Patch

Clearing flags on attachment: 229385

Committed r167326: <http://trac.webkit.org/changeset/167326>
Comment 10 WebKit Commit Bot 2014-04-15 14:05:42 PDT
All reviewed patches have been landed.  Closing bug.