[V8] Do not create a local handle for a cached v8 string that is returned to V8 immediately

• Component: WebCore JavaScript
• Status: RESOLVED FIXED

[Kentaro Hara]

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.
  }

[Kentaro Hara]

Created attachment 182121 [details]
Patch

[Kentaro Hara]

Created attachment 182123 [details]
Patch

[Adam Barth]

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.

[Adam Barth]

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?

[Kentaro Hara]

Created attachment 182284 [details]
Patch

[Kentaro Hara]

(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.

[Dan Carney]

(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.

[Kentaro Hara]

(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...

[Dan Carney]

(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.

[Kentaro Hara]

(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.

[Adam Barth]

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!

[Kentaro Hara]

Created attachment 182371 [details]
patch for landing

[Kentaro Hara]

(In reply to comment #11)
> ReturnPersistentHandle -> ReturnWeakHandle? ReturnUnsafeHandle?

Fixed. And added a comment about the situation.

[WebKit Review Bot]

Comment on attachment 182371 [details]
patch for landing

Clearing flags on attachment: 182371

Committed r139469: <http://trac.webkit.org/changeset/139469>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Ryosuke Niwa]

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?

[Kentaro Hara]

(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.

[Kentaro Hara]

Fixed in r139483.

[Ryosuke Niwa]

Thanks.