Bug 106557 - [V8] Do not create a local handle for a cached v8 string that is returned to V8 immediately
Summary: [V8] Do not create a local handle for a cached v8 string that is returned to ...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore JavaScript (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Kentaro Hara
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2013-01-10 05:52 PST by Kentaro Hara
Modified: 2013-01-11 12:29 PST (History)
5 users (show)

See Also:


Attachments
Patch (22.57 KB, patch)
2013-01-10 05:55 PST, Kentaro Hara
no flags Details | Formatted Diff | Diff
Patch (22.63 KB, patch)
2013-01-10 05:57 PST, Kentaro Hara
no flags Details | Formatted Diff | Diff
Patch (22.37 KB, patch)
2013-01-11 00:45 PST, Kentaro Hara
no flags Details | Formatted Diff | Diff
patch for landing (22.55 KB, patch)
2013-01-11 11:03 PST, Kentaro Hara
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Kentaro Hara 2013-01-10 05:52:22 PST
Currently we are always creating a local handle for a cached V8 string returned to V8:

  Handle<Value> v8String(StringImpl* impl, Isolate* isolate) {
    ...;
    return Local<String>::New(isolate, m_cachedString);
  }

However, we don't need to create a local handle in a case where it is guaranteed that no V8 object allocation is conducted before a control flow returns back to V8. In particular, in a case where a cached V8 string is immediately returned to V8, we don't need to create a local handle:

  Handle<Value> xxxxAttrGetter() {
    ...;
    return v8String(imp->xxxx(), isolate);  // This can return a persistent handle safely.
  }
Comment 1 Kentaro Hara 2013-01-10 05:55:13 PST
Created attachment 182121 [details]
Patch
Comment 2 Kentaro Hara 2013-01-10 05:57:01 PST
Created attachment 182123 [details]
Patch
Comment 3 Adam Barth 2013-01-10 12:24:08 PST
Comment on attachment 182123 [details]
Patch

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

> Source/WebCore/bindings/v8/V8Binding.h:148
> -    inline v8::Handle<v8::String> v8String(const String& string, v8::Isolate* isolate = 0)
> +    inline v8::Handle<v8::String> v8String(const String& string, v8::Isolate* isolate = 0, bool canReturnPersistentHandle = false)

Can we use an enum rather than a boolean parameter?  In this patch, the call sites have this mysterious "true" at the end that's likely to be misunderstood in the future.
Comment 4 Adam Barth 2013-01-10 12:25:00 PST
The V8 team also cautioned us to make sure we can move GC safety at compile time.  Is there any kind of compile-time (or even runtime) check to make sure that people use this flag safely in the future?
Comment 5 Kentaro Hara 2013-01-11 00:45:18 PST
Created attachment 182284 [details]
Patch
Comment 6 Kentaro Hara 2013-01-11 00:46:54 PST
(In reply to comment #4)
> Is there any kind of compile-time (or even runtime) check to make sure that people use this flag safely in the future?

Do you have any idea? "Make ReturnLocalHandle a default parameter and use ReturnPersistentHandle only in auto-generated code" is the current approach.
Comment 7 Dan Carney 2013-01-11 01:03:58 PST
(In reply to comment #6)
> (In reply to comment #4)
> > Is there any kind of compile-time (or even runtime) check to make sure that people use this flag safely in the future?
> 
> Do you have any idea? "Make ReturnLocalHandle a default parameter and use ReturnPersistentHandle only in auto-generated code" is the current approach.

We could wrap the handle in a class in debug mode, UnsafePersistent or something, ( a typedef when not) which asserts when disposed handles are found on any access.
Comment 8 Kentaro Hara 2013-01-11 01:19:18 PST
(In reply to comment #7)
> We could wrap the handle in a class in debug mode, UnsafePersistent or something, ( a typedef when not) which asserts when disposed handles are found on any access.

That's possible but that's not perfect because disposed handles can be resurrected. (I've observed bugs caused by resurrection.) I agree that the access check you're proposing is helpful but I might not want to complicate the code for something non-perfect...
Comment 9 Dan Carney 2013-01-11 01:38:39 PST
(In reply to comment #8)
> (In reply to comment #7)
> > We could wrap the handle in a class in debug mode, UnsafePersistent or something, ( a typedef when not) which asserts when disposed handles are found on any access.
> 
> That's possible but that's not perfect because disposed handles can be resurrected. (I've observed bugs caused by resurrection.) I agree that the access check you're proposing is helpful but I might not want to complicate the code for something non-perfect...

A perfect solution would be add a pointer to the raw handle in the debug object and compare with it on access.
Comment 10 Kentaro Hara 2013-01-11 10:23:23 PST
(In reply to comment #9)
> A perfect solution would be add a pointer to the raw handle in the debug object and compare with it on access.

I discussed this with Dan offline. The approach is implementing a proxy class for Persistent:

#if DEBUG
template<typename T>
class UnsafePersistent {
  UnsafePersistent(handle) {
    handle_ = handle;
    original_object_ = **handle;
  }

  bool IsWeak() {
    // Checks that GC hasn't happened.
    ASSERT(original_object_ == **handle_);
    return handle_.IsWeak();
  }

  // Write all other methods for Persistent

  Persistent<T> handle_;
  void* original_object_;
};
#else
typedef UnsafePersistent Persistent
#endif

Then we can replace a bunch of existing Persistents in V8 bindings with UnsafePersistents.

Actually, I guess that this would be a bit over-engineering and this messes up the code base.

IMHO, "make ReturnLocalHandle a default parameter, use ReturnPersistentHandle only in auto-generated code, and write a comment about it" might be better here.
Comment 11 Adam Barth 2013-01-11 10:48:00 PST
Comment on attachment 182284 [details]
Patch

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

> Source/WebCore/bindings/scripts/test/V8/V8TestInterface.cpp:62
> +    return v8String(TestSupplemental::supplementalStaticAttr(), info.GetIsolate(), ReturnPersistentHandle);

ReturnPersistentHandle -> ReturnWeakHandle? ReturnUnsafeHandle?

We want to pick a name that communicates the danger.  ReturnPersistentHandle makes it sound like the hand that gets return is going to stick around persistently, which is the opposite of the truth!
Comment 12 Kentaro Hara 2013-01-11 11:03:47 PST
Created attachment 182371 [details]
patch for landing
Comment 13 Kentaro Hara 2013-01-11 11:04:44 PST
(In reply to comment #11)
> ReturnPersistentHandle -> ReturnWeakHandle? ReturnUnsafeHandle?

Fixed. And added a comment about the situation.
Comment 14 WebKit Review Bot 2013-01-11 11:29:03 PST
Comment on attachment 182371 [details]
patch for landing

Clearing flags on attachment: 182371

Committed r139469: <http://trac.webkit.org/changeset/139469>
Comment 15 WebKit Review Bot 2013-01-11 11:29:07 PST
All reviewed patches have been landed.  Closing bug.
Comment 16 Ryosuke Niwa 2013-01-11 12:14:32 PST
This patch caused binding tests to fail.

http://build.webkit.org/builders/Apple%20MountainLion%20Release%20WK1%20%28Tests%29/builds/5580/steps/bindings-generation-tests/logs/stdio

Maybe you just need to reabseline it?
Comment 17 Kentaro Hara 2013-01-11 12:15:31 PST
(In reply to comment #16)
http://build.webkit.org/builders/Apple%20MountainLion%20Release%20WK1%20%28Tests%29/builds/5580/steps/bindings-generation-tests/logs/stdio
> 
> Maybe you just need to reabseline it?

Will do.
Comment 18 Kentaro Hara 2013-01-11 12:23:16 PST
Fixed in r139483.
Comment 19 Ryosuke Niwa 2013-01-11 12:29:29 PST
Thanks.