Bug 95519 – Add new V8DependentRetained that allows keeping a v8::Object alive as long as another v8::Object is alive

• Component: DOM
• Status: RESOLVED FIXED

[Elliott Sprehn]

Add new V8DependentRetained that allows keeping a v8::Object alive as long as another v8::Object is alive

[Elliott Sprehn]

Created attachment 161599 [details]
Patch

[WebKit Review Bot]

Attachment 161599 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1
Source/WebCore/bindings/v8/V8HiddenPropertyName.h:53:  Code inside a namespace should not be indented.  [whitespace/indent] [4]
Source/WebCore/bindings/v8/V8HiddenPropertyName.h:62:  The parameter name "type" adds no information, so it should be removed.  [readability/parameter_name] [5]
Total errors found: 2 in 5 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Elliott Sprehn]

(In reply to comment #2)
> Attachment 161599 [details] did not pass style-queue:
> 
> Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1
> Source/WebCore/bindings/v8/V8HiddenPropertyName.h:53:  Code inside a namespace should not be indented.  [whitespace/indent] [4]
> Source/WebCore/bindings/v8/V8HiddenPropertyName.h:62:  The parameter name "type" adds no information, so it should be removed.  [readability/parameter_name] [5]
> Total errors found: 2 in 5 files
> 
> 
> If any of these errors are false positives, please file a bug against check-webkit-style.

Should I reformat the whole file to fix the indentation? It's weird to have one line that's indented wrong.

Also, the default argument value "useless name" warning seems like a bug in check-webkit-style.

[Kentaro Hara]

Comment on attachment 161599 [details]
Patch

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

> Source/WebCore/ChangeLog:11
> +        v8::Object is alive. This is useful for keeping callbacks attached to wrappers without
> +        keeping strong references to v8::Objects in the C++ side which can result in leaks
> +        when cycles are created.

Would you elaborate on what use cases you are assuming?

> Source/WebCore/ChangeLog:13
> +        No new tests needed, this will be used to fix MutationObservers which will have tests.

I might want to see the patch to understand use cases.

> Source/WebCore/bindings/v8/V8DependentRetained.h:76
> +        static double nextId = 0;

This is not thread-safe (i.e. not Worker-safe).

[Erik Arvidsson]

Comment on attachment 161599 [details]
Patch

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

This seems useful

>> Source/WebCore/bindings/v8/V8DependentRetained.h:76
>> +        static double nextId = 0;
> 
> This is not thread-safe (i.e. not Worker-safe).

Why is this a double?

> Source/WebCore/bindings/v8/V8DependentRetained.h:81
> +        name.append(V8_DEPENDENT_RETAINED, sizeof(V8_DEPENDENT_RETAINED) - 1);

Having this long common prefix makes lookups slower. Consider having a short prefix or make the common substring a suffix.

[Erik Arvidsson]

Comment on attachment 161599 [details]
Patch

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

This seems useful

>> Source/WebCore/bindings/v8/V8DependentRetained.h:76
>> +        static double nextId = 0;
> 
> This is not thread-safe (i.e. not Worker-safe).

Why is this a double?

> Source/WebCore/bindings/v8/V8DependentRetained.h:81
> +        name.append(V8_DEPENDENT_RETAINED, sizeof(V8_DEPENDENT_RETAINED) - 1);

Having this long common prefix makes lookups slower. Consider having a short prefix or make the common substring a suffix.

[Adam Barth]

> Should I reformat the whole file to fix the indentation? It's weird to have one line that's indented wrong.

You should reformat the whole file, but in a separate patch.

> Also, the default argument value "useless name" warning seems like a bug in check-webkit-style.

You don't need a name when defining a default argument.

[Adam Barth]

Comment on attachment 161599 [details]
Patch

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

> Source/WebCore/bindings/v8/V8DependentRetained.h:77
> +        v8::HandleScope scope;

I don't think you need a HandleScope here.

> Source/WebCore/bindings/v8/V8DependentRetained.h:83
> +        return v8::Persistent<v8::String>::New(V8HiddenPropertyName::hiddenReferenceName(name.data(), NewString));

Calling v8::Persistent<v8::String>::New here causes a leak.  ScopedPersistent calls New and Dispose for you.  You don't need to call it.

Did you 0-terminate name somewhere?  How does hiddenReferenceName know how long the string is?

> Source/WebCore/bindings/v8/V8DependentRetained.h:96
> +        m_owner.clear();

Should we clear m_propertyName too?

> Source/WebCore/bindings/v8/V8HiddenPropertyName.cpp:63
>      prefixedName.append(name, strlen(name));

Yeah, so, we're assuming 0-termination here.

[Adam Barth]

Do we plan to use this mechanism for EventListeners?  If so, we'll need a thread-safe solution.  If not, we can add an ASSERT(isMainThread()) and solve the thread safety issues if/when we need to use this mechanism in workers.

[Elliott Sprehn]

(In reply to comment #4)
> (From update of attachment 161599 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=161599&action=review
> 
> > Source/WebCore/ChangeLog:11
> > +        v8::Object is alive. This is useful for keeping callbacks attached to wrappers without
> > +        keeping strong references to v8::Objects in the C++ side which can result in leaks
> > +        when cycles are created.
> 
> Would you elaborate on what use cases you are assuming?
> 

See WKBug 93661 for context. This is so a MutationObserver or UndoManager can keep it's callback alive without creating cycles between the JS heap and the C++ world.

[Elliott Sprehn]

(In reply to comment #5)
> (From update of attachment 161599 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=161599&action=review
> 
> This seems useful
> 
> >> Source/WebCore/bindings/v8/V8DependentRetained.h:76
> >> +        static double nextId = 0;
> > 
> > This is not thread-safe (i.e. not Worker-safe).
> 
> Why is this a double?

No particular reason except that numberToString takes a double. I'll switch to int.

> 
> > Source/WebCore/bindings/v8/V8DependentRetained.h:81
> > +        name.append(V8_DEPENDENT_RETAINED, sizeof(V8_DEPENDENT_RETAINED) - 1);
> 
> Having this long common prefix makes lookups slower. Consider having a short prefix or make the common substring a suffix.

This was per the request of abarth. I originally had a separate weak handle for direct access and skipped the hash lookup on read entirely, but he said to ignore perf for now and use the long prefix.

[Elliott Sprehn]

(In reply to comment #8)
> (From update of attachment 161599 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=161599&action=review
> ...
> 
> Did you 0-terminate name somewhere?  How does hiddenReferenceName know how long the string is?
> 
> > 

This is a duplication of the logic in hiddenReferenceName() which assumed you passed a cont char* to a NUL terminated string.

[Elliott Sprehn]

(In reply to comment #9)
> Do we plan to use this mechanism for EventListeners?  If so, we'll need a thread-safe solution.  If not, we can add an ASSERT(isMainThread()) and solve the thread safety issues if/when we need to use this mechanism in workers.

I already have this ASSERT. I would like to attempt to move event listeners eventually, but for now MutationObservers and UndoManager are the targets, and they're not accessible inside a Worker.

If it's trivial to make this thread safe I'll do it now, otherwise I'd like to land this and fix MutationObservers before trying to tackle that.

[Adam Barth]

> This is a duplication of the logic in hiddenReferenceName() which assumed you passed a cont char* to a NUL terminated string.

Which line of code NUL terminates the string?

> I already have this ASSERT.

Yes you do!  :)

The easiest way to make this thread safe is to put the counter in V8PerIsolateData and use V8PerIsolateData::current() to retrieve it.

[Elliott Sprehn]

Created attachment 161804 [details]
Patch

Make this thread safe, fix NUL termination, and make get() faster by using a separate weak Handle. In discussions with adamk and ojan, and per Arvs comment it seems like we should use a little more memory here and not worry about the hashing speed for access. You're likely to have only a few MutationObservers or DOMTransactions, but they may execute many times.

[WebKit Review Bot]

Attachment 161804 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1
Source/WebCore/bindings/v8/V8HiddenPropertyName.h:53:  Code inside a namespace should not be indented.  [whitespace/indent] [4]
Total errors found: 1 in 7 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Kentaro Hara]

(In reply to comment #10)
> > Would you elaborate on what use cases you are assuming?
> 
> See WKBug 93661 for context. This is so a MutationObserver or UndoManager can keep it's callback alive without creating cycles between the JS heap and the C++ world.

Thanks for the clarification! This feature looks useful:)

[Adam Barth]

Comment on attachment 161804 [details]
Patch

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

This looks great.  One nit about using the preprocessor.

> Source/WebCore/bindings/v8/V8DependentRetained.h:43
> +#define V8_DEPENDENT_RETAINED "V8DependentRetained"

Generally we prefer to avoid using the preprocessor for constants.  Perhaps this can be a local constant in createPropertyName?

> Source/WebCore/bindings/v8/V8DependentRetained.h:56
> +        m_owner.get().MakeWeak(this, &V8DependentRetained::weakCallback);
> +        m_value.get().MakeWeak(this, &V8DependentRetained::weakCallback);

It seems like we'll always get the weak callback from m_owner first because no one else is going to drop the reference from m_owner to m_value.

[Elliott Sprehn]

Created attachment 162380 [details]
Patch for landing

[Elliott Sprehn]

Created attachment 162381 [details]
Patch for landing

[Elliott Sprehn]

Created attachment 162383 [details]
Patch for landing

[WebKit Review Bot]

Comment on attachment 162383 [details]
Patch for landing

Clearing flags on attachment: 162383

Committed r127718: <http://trac.webkit.org/changeset/127718>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Kentaro Hara]

Quick question: V8DependentRetailed is not yet used. Is it going to be used by MutationObservers in the future?

[Elliott Sprehn]

(In reply to comment #24)
> Quick question: V8DependentRetailed is not yet used. Is it going to be used by MutationObservers in the future?

No, the plan now is delete both V8 and JSDependentRetained.