Bug 110206 – [v8] ScriptValue has dangerous copy semantics

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Dan Carney]

[v8] ScriptValue has dangerous copy semantics

[Dan Carney]

Created attachment 189048 [details]
Patch

[Kentaro Hara]

Comment on attachment 189048 [details]
Patch

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

> Source/WebCore/ChangeLog:9
> +        Update ScriptValue to use a reference counted ScopedPersistent
> +        as the existing implementation makes it easy to return dead references.

Would you elaborate on what is dangerous?

We're intentionally replacing SharedPersistent with ScopedPersistent, since it is easy to misuse SharedPersistent.

[Dan Carney]

(In reply to comment #2)
> (From update of attachment 189048 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=189048&action=review
> 
> > Source/WebCore/ChangeLog:9
> > +        Update ScriptValue to use a reference counted ScopedPersistent
> > +        as the existing implementation makes it easy to return dead references.
> 
> Would you elaborate on what is dangerous?
> 
> We're intentionally replacing SharedPersistent with ScopedPersistent, since it is easy to misuse SharedPersistent.

The following returns a disposed handle:

v8::Handle<v8::Value> someFunction() {
  ScriptValue value = getSomeValue();
  return value->v8Value();
  // Should be
  // return v8::Local<v8::Value>::New(value.v8Value());
}

This happens in a few places around the codebase.  I'm fixing them one by one, but in IDBAny::toV8, I cannot do it, as toV8 is expected to return a persistent handle and I don't want to change the semantics for one case.

[Dan Carney]

> We're intentionally replacing SharedPersistent with ScopedPersistent, since it is easy to misuse SharedPersistent.

With my proposed change, only the v8Value() function remains a problem, and I think there are a number of ways around the issue.

[Dan Carney]

Created attachment 189072 [details]
Patch

[Dan Carney]

Comment on attachment 189072 [details]
Patch

Local::New is really the only way to solve these problems, so I'm giving it a try here.  Otherwise the v8Value is simply too dangerous, as it's bound to the life of these ScriptValue objects.

[Kentaro Hara]

Comment on attachment 189072 [details]
Patch

The change looks better than before.

In terms of performance, is it really worth introducing v8ValueRaw() ? What happens if you always create a new local handle in v8Value() ? I think that Local::New() is very light as long as you pass an Isolate parameter to it.

[Dan Carney]

(In reply to comment #7)
> (From update of attachment 189072 [details])
> The change looks better than before.
> 
> In terms of performance, is it really worth introducing v8ValueRaw() ? What happens if you always create a new local handle in v8Value() ? I think that Local::New() is very light as long as you pass an Isolate parameter to it.

I didn't add expose v8ValueRaw() because of performance, but because there are places it's used without a handlescope.  Perhaps those can be fixed, but first I wanted the basic concept working correctly.

[Kentaro Hara]

Comment on attachment 189072 [details]
Patch

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

Understood. Let's kill v8ValueRaw() in a follow-up.

> Source/WebCore/ChangeLog:9
> +        Update ScriptValue to use a reference counted ScopedPersistent
> +        as the existing implementation makes it easy to return dead references.

Update the ChangeLog.

> Source/WebCore/bindings/v8/ScriptValue.h:138
> +    v8::Handle<v8::Value> v8ValueRaw() const

Please add FIXME to indicate that v8ValueRaw() should be deprecated.

[Dan Carney]

Created attachment 189253 [details]
Patch

[WebKit Review Bot]

Comment on attachment 189253 [details]
Patch

Clearing flags on attachment: 189253

Committed r143441: <http://trac.webkit.org/changeset/143441>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[WebKit Review Bot]

Re-opened since this is blocked by bug 110376

[Joshua Bell]

*** Bug 105363 has been marked as a duplicate of this bug. ***

[Alec Flett]

See also, bug 111002 which works around some of this in IDB land.

[Alec Flett]

And I can confirm that this patch fixes the IDB V8 flakiness issues that we've been seeing.

[Dan Carney]

Created attachment 190915 [details]
Patch

[Dan Carney]

Comment on attachment 190915 [details]
Patch

rebased. trying again to roll into chrome

[WebKit Review Bot]

Comment on attachment 190915 [details]
Patch

Clearing flags on attachment: 190915

Committed r144458: <http://trac.webkit.org/changeset/144458>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.