Bug 110206 - [v8] ScriptValue has dangerous copy semantics
Summary: [v8] ScriptValue has dangerous copy semantics
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords:
: 105363 (view as bug list)
Depends on: 110376
Blocks: 111002
  Show dependency treegraph
 
Reported: 2013-02-19 04:03 PST by Dan Carney
Modified: 2013-03-01 08:26 PST (History)
7 users (show)

See Also:


Attachments
Patch (6.75 KB, patch)
2013-02-19 04:07 PST, Dan Carney
no flags Details | Formatted Diff | Diff
Patch (9.39 KB, patch)
2013-02-19 06:19 PST, Dan Carney
no flags Details | Formatted Diff | Diff
Patch (9.42 KB, patch)
2013-02-19 23:16 PST, Dan Carney
no flags Details | Formatted Diff | Diff
Patch (9.41 KB, patch)
2013-03-01 02:42 PST, Dan Carney
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Dan Carney 2013-02-19 04:03:52 PST
[v8] ScriptValue has dangerous copy semantics
Comment 1 Dan Carney 2013-02-19 04:07:25 PST
Created attachment 189048 [details]
Patch
Comment 2 Kentaro Hara 2013-02-19 04:28:50 PST
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.
Comment 3 Dan Carney 2013-02-19 04:55:21 PST
(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.
Comment 4 Dan Carney 2013-02-19 05:15:51 PST
> 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.
Comment 5 Dan Carney 2013-02-19 06:19:29 PST
Created attachment 189072 [details]
Patch
Comment 6 Dan Carney 2013-02-19 06:21:09 PST
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.
Comment 7 Kentaro Hara 2013-02-19 09:11:38 PST
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.
Comment 8 Dan Carney 2013-02-19 09:16:36 PST
(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.
Comment 9 Kentaro Hara 2013-02-19 09:19:21 PST
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.
Comment 10 Dan Carney 2013-02-19 23:16:46 PST
Created attachment 189253 [details]
Patch
Comment 11 WebKit Review Bot 2013-02-20 02:00:20 PST
Comment on attachment 189253 [details]
Patch

Clearing flags on attachment: 189253

Committed r143441: <http://trac.webkit.org/changeset/143441>
Comment 12 WebKit Review Bot 2013-02-20 02:00:24 PST
All reviewed patches have been landed.  Closing bug.
Comment 13 WebKit Review Bot 2013-02-20 13:44:18 PST
Re-opened since this is blocked by bug 110376
Comment 14 Joshua Bell 2013-02-27 11:57:08 PST
*** Bug 105363 has been marked as a duplicate of this bug. ***
Comment 15 Alec Flett 2013-02-27 12:05:01 PST
See also, bug 111002 which works around some of this in IDB land.
Comment 16 Alec Flett 2013-02-27 12:08:39 PST
And I can confirm that this patch fixes the IDB V8 flakiness issues that we've been seeing.
Comment 17 Dan Carney 2013-03-01 02:42:55 PST
Created attachment 190915 [details]
Patch
Comment 18 Dan Carney 2013-03-01 02:43:36 PST
Comment on attachment 190915 [details]
Patch

rebased. trying again to roll into chrome
Comment 19 WebKit Review Bot 2013-03-01 08:26:39 PST
Comment on attachment 190915 [details]
Patch

Clearing flags on attachment: 190915

Committed r144458: <http://trac.webkit.org/changeset/144458>
Comment 20 WebKit Review Bot 2013-03-01 08:26:44 PST
All reviewed patches have been landed.  Closing bug.