Bug 95519 - Add new V8DependentRetained that allows keeping a v8::Object alive as long as another v8::Object is alive
Summary: Add new V8DependentRetained that allows keeping a v8::Object alive as long as...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: DOM (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Elliott Sprehn
URL:
Keywords:
Depends on:
Blocks: 93661
  Show dependency treegraph
 
Reported: 2012-08-30 18:35 PDT by Elliott Sprehn
Modified: 2012-12-05 17:59 PST (History)
6 users (show)

See Also:


Attachments
Patch (8.64 KB, patch)
2012-08-30 18:38 PDT, Elliott Sprehn
no flags Details | Formatted Diff | Diff
Patch (10.63 KB, patch)
2012-08-31 18:07 PDT, Elliott Sprehn
no flags Details | Formatted Diff | Diff
Patch for landing (11.05 KB, patch)
2012-09-05 17:38 PDT, Elliott Sprehn
no flags Details | Formatted Diff | Diff
Patch for landing (11.06 KB, patch)
2012-09-05 17:47 PDT, Elliott Sprehn
no flags Details | Formatted Diff | Diff
Patch for landing (11.07 KB, patch)
2012-09-05 17:48 PDT, Elliott Sprehn
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Elliott Sprehn 2012-08-30 18:35:47 PDT
Add new V8DependentRetained that allows keeping a v8::Object alive as long as another v8::Object is alive
Comment 1 Elliott Sprehn 2012-08-30 18:38:17 PDT
Created attachment 161599 [details]
Patch
Comment 2 WebKit Review Bot 2012-08-30 18:40:20 PDT
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.
Comment 3 Elliott Sprehn 2012-08-30 18:49:38 PDT
(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.
Comment 4 Kentaro Hara 2012-08-31 05:48:06 PDT
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).
Comment 5 Erik Arvidsson 2012-08-31 06:37:15 PDT
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.
Comment 6 Erik Arvidsson 2012-08-31 06:37:16 PDT
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.
Comment 7 Adam Barth 2012-08-31 08:59:14 PDT
> 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.
Comment 8 Adam Barth 2012-08-31 09:06:37 PDT
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.
Comment 9 Adam Barth 2012-08-31 09:08:15 PDT
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.
Comment 10 Elliott Sprehn 2012-08-31 10:27:51 PDT
(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.
Comment 11 Elliott Sprehn 2012-08-31 10:32:15 PDT
(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.
Comment 12 Elliott Sprehn 2012-08-31 10:35:28 PDT
(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.
Comment 13 Elliott Sprehn 2012-08-31 10:37:53 PDT
(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.
Comment 14 Adam Barth 2012-08-31 11:11:24 PDT
> 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.
Comment 15 Elliott Sprehn 2012-08-31 18:07:05 PDT
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.
Comment 16 WebKit Review Bot 2012-08-31 18:10:52 PDT
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.
Comment 17 Kentaro Hara 2012-08-31 18:45:36 PDT
(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:)
Comment 18 Adam Barth 2012-09-01 01:07:39 PDT
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.
Comment 19 Elliott Sprehn 2012-09-05 17:38:46 PDT
Created attachment 162380 [details]
Patch for landing
Comment 20 Elliott Sprehn 2012-09-05 17:47:39 PDT
Created attachment 162381 [details]
Patch for landing
Comment 21 Elliott Sprehn 2012-09-05 17:48:26 PDT
Created attachment 162383 [details]
Patch for landing
Comment 22 WebKit Review Bot 2012-09-06 03:05:25 PDT
Comment on attachment 162383 [details]
Patch for landing

Clearing flags on attachment: 162383

Committed r127718: <http://trac.webkit.org/changeset/127718>
Comment 23 WebKit Review Bot 2012-09-06 03:05:30 PDT
All reviewed patches have been landed.  Closing bug.
Comment 24 Kentaro Hara 2012-12-05 17:58:36 PST
Quick question: V8DependentRetailed is not yet used. Is it going to be used by MutationObservers in the future?
Comment 25 Elliott Sprehn 2012-12-05 17:59:05 PST
(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.