Bug 110206

Summary: [v8] ScriptValue has dangerous copy semantics
Product: WebKit Reporter: Dan Carney <dcarney>
Component: New BugsAssignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: abarth, alecflett, dglazkov, haraken, japhet, jsbell, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on: 110376    
Bug Blocks: 111002    
Attachments:
Description Flags
Patch
none
Patch
none
Patch
none
Patch none

Dan Carney
Reported 2013-02-19 04:03:52 PST
[v8] ScriptValue has dangerous copy semantics
Attachments
Patch (6.75 KB, patch)
2013-02-19 04:07 PST, Dan Carney
no flags
Patch (9.39 KB, patch)
2013-02-19 06:19 PST, Dan Carney
no flags
Patch (9.42 KB, patch)
2013-02-19 23:16 PST, Dan Carney
no flags
Patch (9.41 KB, patch)
2013-03-01 02:42 PST, Dan Carney
no flags
Dan Carney
Comment 1 2013-02-19 04:07:25 PST
Kentaro Hara
Comment 2 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.
Dan Carney
Comment 3 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.
Dan Carney
Comment 4 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.
Dan Carney
Comment 5 2013-02-19 06:19:29 PST
Dan Carney
Comment 6 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.
Kentaro Hara
Comment 7 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.
Dan Carney
Comment 8 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.
Kentaro Hara
Comment 9 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.
Dan Carney
Comment 10 2013-02-19 23:16:46 PST
WebKit Review Bot
Comment 11 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>
WebKit Review Bot
Comment 12 2013-02-20 02:00:24 PST
All reviewed patches have been landed. Closing bug.
WebKit Review Bot
Comment 13 2013-02-20 13:44:18 PST
Re-opened since this is blocked by bug 110376
Joshua Bell
Comment 14 2013-02-27 11:57:08 PST
*** Bug 105363 has been marked as a duplicate of this bug. ***
Alec Flett
Comment 15 2013-02-27 12:05:01 PST
See also, bug 111002 which works around some of this in IDB land.
Alec Flett
Comment 16 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.
Dan Carney
Comment 17 2013-03-01 02:42:55 PST
Dan Carney
Comment 18 2013-03-01 02:43:36 PST
Comment on attachment 190915 [details] Patch rebased. trying again to roll into chrome
WebKit Review Bot
Comment 19 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>
WebKit Review Bot
Comment 20 2013-03-01 08:26:44 PST
All reviewed patches have been landed. Closing bug.
Note You need to log in before you can comment on or make changes to this bug.