New issue
Advanced search Search tips

Issue 755520 link

Starred by 1 user
Status: Assigned
Owner:
yukishiino@chromium.org
Cc:
peria@chromium.org
haraken@chromium.org
lfg@chromium.org
Components:
EstimatedDays: ----
NextAction: ----
OS: All
Pri: 3
Type: Bug



Sign in to add a comment

ScriptValue shouldn't hold a ScriptState except for its CreationContext().

Project Member Reported by yukishiino@chromium.org, Aug 15 2017 
ScriptValue should be a simple thin wrapper of v8::Value.
It means that ScriptValue::script_state_ should be a ScriptState of ScriptValue::V8Value().As<v8::Object>()->CreationContext().

The current implementation allows call sites to associate a ScriptValue with an arbitrary v8::Context, and it's quite confusing or even danger to leak a realm.

Some of clients of ScriptValue are (ab)using ScriptValue::GetContext() and/or ScriptValue::GetScriptState().

My ideas at this moment are to let ScriptValue hold:
a) only v8::Value (value_).
b) v8::Value (value_) and v8::Isolate*.
c) v8::Value (value_) and ScriptState that is extracted from value_'s CreationContext().

Idea c) is closest to the current implementation, however, we need to handle non-v8::Object case because v8::Value in general is not associated with any (creation) context.  We could use the current context of creation time of ScriptValue, but it's a kind of compromise.

Comment 1 by jbroman@chromium.org, Aug 15 2017 

Would a DCHECK suffice? Roughly:

if (value.IsObject())
  DCHECK(value.As<v8::Object>()->CreationContext() == script_state->context_);

Comment 2 by haraken@chromium.org, Aug 15 2017 

Do you know why we need to make ScriptValue remember script_state_ in the first place? Would it be hard to make the caller sites of ScriptValue maintain the script_state_ they should use?

Comment 3 by yukishiino@chromium.org, Aug 15 2017 

Re: #1

I didn't actually test it, but at a glance, that DCHECK obviously fails because most of call sites are passing the current realm or relevant realm of the receiver object.  In case that a ScriptValue is an argument, it may have its own realm, which may be different from the current nor relevant of the receiver object.

In case of
  foo.bar(baz);
foo, bar and baz could be associated with each different realm.

By the way, we have a similar problem for callback functions.


Re: #2

ScriptValue shouldn't need to remember any realm in the first place.  It seems possible to make the call sites not use the ScriptState of ScriptValue.  But it needs some amount of work.

Comment 4 by jbroman@chromium.org, Aug 15 2017 

Some places (like CustomEvent) use ScriptValue to clone the object in the case of cross-realm access. I haven't audited the uses to see whether that's everything.

If we don't want this sort of functionality, why not just use v8::Global<v8::Value> instead of ScriptValue?

Comment 5 by yukishiino@chromium.org, Aug 15 2017 

Cloning is an important part that is sensitive to the realm.  So, I think that it would make sense to hold script_state_ member just as a cache of value_->CreationContext().  If value_ is not a v8::Object, i.e. value_ is not associated with any realm, we don't need to clone it (e.g. v8::String).

One of benefits of ScriptValue may be that it supports wrapper-tracing.  Maybe, copying a ScriptValue is cheaper than copying v8::Global (not quite sure)?  And, cloning across worlds?

Comment 6 by haraken@chromium.org, Aug 15 2017 

> If we don't want this sort of functionality, why not just use v8::Global<v8::Value> instead of ScriptValue?

Yeah, this would be an option. I think that most existing ScriptValues should be replaced with v8::Local<v8::Value> because they are used on a machine stack. Other ScriptValues should be replaced with ScopedPersistents (or SharedPersistents).

The only question on my side is whether we want to scatter lots of v8::Local<v8::Value>s to core/ and modules/...

Comment 7 by yukishiino@chromium.org, Aug 15 2017 

Meh, I was wrong.  ScriptValue doesn't directly support wrapper-tracing atm.  I agree that it would be an option to retire ScriptValue itself.
Project Member

Comment 9 by bugdroid1@chromium.org, Dec 13 

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/8d579fc15d93d5fd9677bbfda8c5144569d27240

commit 8d579fc15d93d5fd9677bbfda8c5144569d27240
Author: Yuki Shiino <yukishiino@chromium.org>
Date: Thu Dec 13 06:06:01 2018

v8binding: Unify ScriptValue::ToWorldSafeScriptValue to WorldSafeV8Reference

Remove ScriptValue::ToWorldSafeScriptValue and replace its usage
with WorldSafeV8Reference.

Bug: 501866, 755520, 803478
Change-Id: I75f38dd4d4dd8c3730bec1e9d002fadaa236395b
Reviewed-on: https://chromium-review.googlesource.com/c/1373894
Reviewed-by: Kentaro Hara <haraken@chromium.org>
Reviewed-by: Hitoshi Yoshida <peria@chromium.org>
Commit-Queue: Yuki Shiino <yukishiino@chromium.org>
Cr-Commit-Position: refs/heads/master@{#616228}
[modify] https://crrev.com/8d579fc15d93d5fd9677bbfda8c5144569d27240/third_party/blink/renderer/bindings/core/v8/script_value.cc
[modify] https://crrev.com/8d579fc15d93d5fd9677bbfda8c5144569d27240/third_party/blink/renderer/bindings/core/v8/script_value.h
[modify] https://crrev.com/8d579fc15d93d5fd9677bbfda8c5144569d27240/third_party/blink/renderer/bindings/core/v8/world_safe_v8_reference.h
[modify] https://crrev.com/8d579fc15d93d5fd9677bbfda8c5144569d27240/third_party/blink/renderer/modules/payments/payment_method_change_event.cc
[modify] https://crrev.com/8d579fc15d93d5fd9677bbfda8c5144569d27240/third_party/blink/renderer/modules/payments/payment_method_change_event.h
[modify] https://crrev.com/8d579fc15d93d5fd9677bbfda8c5144569d27240/third_party/blink/renderer/modules/payments/payment_response.cc
[modify] https://crrev.com/8d579fc15d93d5fd9677bbfda8c5144569d27240/third_party/blink/renderer/modules/payments/payment_response.h
►

Sign in to add a comment