|
|
DescriptionStop storing ScriptValue in MessageEvent
Instaed use wrapper tracing to refer a V8 value from Blink.
BUG=501866
Patch Set 1 #
Total comments: 5
Patch Set 2 : rebase #Patch Set 3 : Fix test #Patch Set 4 : Fix tests #Patch Set 5 : Fix tests #
Messages
Total messages: 51 (35 generated)
The CQ bit was checked by bashi@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
bashi@chromium.org changed reviewers: + haraken@chromium.org
PTAL
https://codereview.chromium.org/2849373002/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/events/MessageEvent.cpp (right): https://codereview.chromium.org/2849373002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/events/MessageEvent.cpp:241: return ScriptValue(script_state, data_as_v8_value_reference_.NewLocal( Do you know why we don't need to check the world before returning the v8 value? The message event may be created and accessed by different worlds.
https://codereview.chromium.org/2849373002/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/events/MessageEvent.cpp (right): https://codereview.chromium.org/2849373002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/events/MessageEvent.cpp:241: return ScriptValue(script_state, data_as_v8_value_reference_.NewLocal( On 2017/05/02 07:24:41, haraken wrote: > > Do you know why we don't need to check the world before returning the v8 value? > The message event may be created and accessed by different worlds. > In V8MessageEvent::dataAttributeGetterCustom() we clone the v8 value when world is different. V8ValueFor(script_state) does the job. If we remove custom getter we need to check here, but removing the custom getter doesn't seem to be trivial change.
LGTM https://codereview.chromium.org/2849373002/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/events/MessageEvent.h (right): https://codereview.chromium.org/2849373002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/events/MessageEvent.h:204: TraceWrapperV8Reference<v8::Value> data_as_v8_value_reference_; In a follow-up CL, I'd prefer introducing: WorldAwareV8Reference<v8::Value> ...; which takes care of all the complexity about the per-world wrappers. Then we can remove ScriptValue.V8ValueFor().
https://codereview.chromium.org/2849373002/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/events/MessageEvent.h (right): https://codereview.chromium.org/2849373002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/events/MessageEvent.h:204: TraceWrapperV8Reference<v8::Value> data_as_v8_value_reference_; On 2017/05/02 08:10:14, haraken wrote: > > In a follow-up CL, I'd prefer introducing: > > WorldAwareV8Reference<v8::Value> ...; > > which takes care of all the complexity about the per-world wrappers. Then we can > remove ScriptValue.V8ValueFor(). Agreed that we should have such class or mechanism. I'll try to think about it next week.
The CQ bit was checked by bashi@chromium.org
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by bashi@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by bashi@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by bashi@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by bashi@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by bashi@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Please take another look? This CL has changed since you reviewed. https://codereview.chromium.org/2849373002/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/events/MessageEvent.cpp (right): https://codereview.chromium.org/2849373002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/events/MessageEvent.cpp:241: return ScriptValue(script_state, data_as_v8_value_reference_.NewLocal( On 2017/05/02 07:38:12, bashi wrote: > On 2017/05/02 07:24:41, haraken wrote: > > > > Do you know why we don't need to check the world before returning the v8 > value? > > The message event may be created and accessed by different worlds. > > > > In V8MessageEvent::dataAttributeGetterCustom() we clone the v8 value when world > is different. V8ValueFor(script_state) does the job. > > If we remove custom getter we need to check here, but removing the custom getter > doesn't seem to be trivial change. Sorry I was wrong. We need to check world here. Changed this CL to use almost the same approach as CustomEvent. In a follow-up CL I'll try to reduce the dup (maybe by introducing WorldAwareReference).
LGTM
yukishiino@chromium.org changed reviewers: + yukishiino@chromium.org
LGTM. FYI, you can determine the world from v8::Value itself. IIUC, you can tell the world as v8::Object::CreationContext() => ScriptState => DOMWrapperWorld. If it's not a v8::Object, the v8::Value should be a primitive value and it shouldn't depend on a context, i.e. the world doesn't matter.
On 2017/05/09 11:48:03, Yuki wrote: > LGTM. > > FYI, you can determine the world from v8::Value itself. IIUC, you can tell the > world as v8::Object::CreationContext() => ScriptState => DOMWrapperWorld. If > it's not a v8::Object, the v8::Value should be a primitive value and it > shouldn't depend on a context, i.e. the world doesn't matter. Thanks for the info! I'd like to discuss how we should handle worlds in general in a follow-up CL :)
The CQ bit was checked by bashi@chromium.org
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by bashi@chromium.org
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
We've realized that using TraceWrapperV8Reference in Events requires proper reference chains from the root set and it seems we don't have those chain yet. Let me put this CL off until we have a solution for the issue. Details: https://groups.google.com/a/chromium.org/d/msg/platform-architecture-dev/1nLw...
The CQ bit was checked by bashi@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from ========== Stop storing ScriptValue in MessageEvent Instaed use wrapper tracing to refer a V8 value from Blink. BUG=501866 ========== to ========== Stop storing ScriptValue in MessageEvent Instaed use wrapper tracing to refer a V8 value from Blink. BUG=501866 ========== |