Summary: | [v8] ScriptValue has dangerous copy semantics | ||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Dan Carney <dcarney> | ||||||||||
Component: | New Bugs | Assignee: | 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
Dan Carney
2013-02-19 04:03:52 PST
Created attachment 189048 [details]
Patch
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. (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.
> 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.
Created attachment 189072 [details]
Patch
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 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.
(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 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. Created attachment 189253 [details]
Patch
Comment on attachment 189253 [details] Patch Clearing flags on attachment: 189253 Committed r143441: <http://trac.webkit.org/changeset/143441> All reviewed patches have been landed. Closing bug. Re-opened since this is blocked by bug 110376 *** Bug 105363 has been marked as a duplicate of this bug. *** See also, bug 111002 which works around some of this in IDB land. And I can confirm that this patch fixes the IDB V8 flakiness issues that we've been seeing. Created attachment 190915 [details]
Patch
Comment on attachment 190915 [details]
Patch
rebased. trying again to roll into chrome
Comment on attachment 190915 [details] Patch Clearing flags on attachment: 190915 Committed r144458: <http://trac.webkit.org/changeset/144458> All reviewed patches have been landed. Closing bug. |