Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2102413002:
[Refactoring] Remove toV8Object functions (Closed)
DescriptionRemove toV8Object functions.
This CL is a follow-up CL of https://codereview.chromium.org/2101953002/
BUG=614919
Committed: https://crrev.com/c1cf0eeba3a6d4541f8a2b7161cb0859a70a8493
Cr-Commit-Position: refs/heads/master@{#402778}
Patch Set 1 #
Messages
Total messages: 17 (5 generated)
Description was changed from ========== Remove toV8Object functions BUG=614919 ========== to ========== Remove toV8Object functions. This CL is a follow-up CL of https://codereview.chromium.org/2101953002/ BUG=614919 ==========
peria@chromium.org changed reviewers: + haraken@chromium.org, yukishiino@chromium.org
PTL
LGTM This CL changes existing behavior because after this CL ScriptValueSerializer starts crashing when toV8 returns an empty handle. However, thanks to nhiroki's work, it's now super rare that toV8 returns an empty handle. So I'm okay with the change.
LGTM.
On 2016/06/29 08:51:18, haraken wrote: > LGTM > > This CL changes existing behavior because after this CL ScriptValueSerializer > starts crashing when toV8 returns an empty handle. However, thanks to nhiroki's > work, it's now super rare that toV8 returns an empty handle. So I'm okay with > the change. Before this CL, toV8Object() seems to return v8::Local<v8::Object>(), and I think it is same with an empty handle v8::Local<v8::Value>().As<v8::Object>(), isn't it?
On 2016/06/29 09:06:01, peria wrote: > On 2016/06/29 08:51:18, haraken wrote: > > LGTM > > > > This CL changes existing behavior because after this CL ScriptValueSerializer > > starts crashing when toV8 returns an empty handle. However, thanks to > nhiroki's > > work, it's now super rare that toV8 returns an empty handle. So I'm okay with > > the change. > > Before this CL, toV8Object() seems to return v8::Local<v8::Object>(), > and I think it is same with an empty handle > v8::Local<v8::Value>().As<v8::Object>(), > isn't it? I guess v8::Local<v8::Value>().As<v8::Object>() will crash (though I've not yet checked).
On 2016/06/29 09:09:52, haraken wrote: > On 2016/06/29 09:06:01, peria wrote: > > On 2016/06/29 08:51:18, haraken wrote: > > > LGTM > > > > > > This CL changes existing behavior because after this CL > ScriptValueSerializer > > > starts crashing when toV8 returns an empty handle. However, thanks to > > nhiroki's > > > work, it's now super rare that toV8 returns an empty handle. So I'm okay > with > > > the change. > > > > Before this CL, toV8Object() seems to return v8::Local<v8::Object>(), > > and I think it is same with an empty handle > > v8::Local<v8::Value>().As<v8::Object>(), > > isn't it? > > I guess v8::Local<v8::Value>().As<v8::Object>() will crash (though I've not yet > checked). v8::Local<v8::Value>().As<v8::Object>() should be okay, IIUC. v8::Null(isolate).As<v8::Object>() is NOT okay, though. If |impl| is nullptr, then toV8(impl, creationContext, isolate) returns v8::Null(isolate). Then, we have a problem because we're doing v8::Null(isolate).As<v8::Object>() where a type mismatch occurs.
On 2016/06/29 09:14:52, Yuki wrote: > On 2016/06/29 09:09:52, haraken wrote: > > On 2016/06/29 09:06:01, peria wrote: > > > On 2016/06/29 08:51:18, haraken wrote: > > > > LGTM > > > > > > > > This CL changes existing behavior because after this CL > > ScriptValueSerializer > > > > starts crashing when toV8 returns an empty handle. However, thanks to > > > nhiroki's > > > > work, it's now super rare that toV8 returns an empty handle. So I'm okay > > with > > > > the change. > > > > > > Before this CL, toV8Object() seems to return v8::Local<v8::Object>(), > > > and I think it is same with an empty handle > > > v8::Local<v8::Value>().As<v8::Object>(), > > > isn't it? > > > > I guess v8::Local<v8::Value>().As<v8::Object>() will crash (though I've not > yet > > checked). > > v8::Local<v8::Value>().As<v8::Object>() should be okay, IIUC. > > v8::Null(isolate).As<v8::Object>() is NOT okay, though. > If |impl| is nullptr, then > toV8(impl, creationContext, isolate) > returns v8::Null(isolate). Then, we have a problem because we're doing > v8::Null(isolate).As<v8::Object>() > where a type mismatch occurs. Won't we crash when we run obj->IsJSReceiver() in CheckCast (when the type check is enabled)? https://cs.chromium.org/chromium/src/v8/src/api.cc?q=api.cc&sq=package:chromi...
On 2016/06/29 09:21:31, haraken wrote: > On 2016/06/29 09:14:52, Yuki wrote: > > On 2016/06/29 09:09:52, haraken wrote: > > > On 2016/06/29 09:06:01, peria wrote: > > > > On 2016/06/29 08:51:18, haraken wrote: > > > > > LGTM > > > > > > > > > > This CL changes existing behavior because after this CL > > > ScriptValueSerializer > > > > > starts crashing when toV8 returns an empty handle. However, thanks to > > > > nhiroki's > > > > > work, it's now super rare that toV8 returns an empty handle. So I'm okay > > > with > > > > > the change. > > > > > > > > Before this CL, toV8Object() seems to return v8::Local<v8::Object>(), > > > > and I think it is same with an empty handle > > > > v8::Local<v8::Value>().As<v8::Object>(), > > > > isn't it? > > > > > > I guess v8::Local<v8::Value>().As<v8::Object>() will crash (though I've not > > yet > > > checked). > > > > v8::Local<v8::Value>().As<v8::Object>() should be okay, IIUC. > > > > v8::Null(isolate).As<v8::Object>() is NOT okay, though. > > If |impl| is nullptr, then > > toV8(impl, creationContext, isolate) > > returns v8::Null(isolate). Then, we have a problem because we're doing > > v8::Null(isolate).As<v8::Object>() > > where a type mismatch occurs. > > Won't we crash when we run obj->IsJSReceiver() in CheckCast (when the type check > is enabled)? > > https://cs.chromium.org/chromium/src/v8/src/api.cc?q=api.cc&sq=package:chromi... Ah, right. It does. https://cs.chromium.org/chromium/src/v8/include/v8.h?sq=package:chromium&dr&r... Thank you for the clarification. Anyway, committing.
The CQ bit was checked by peria@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
On 2016/06/29 09:28:32, peria wrote: > On 2016/06/29 09:21:31, haraken wrote: > > On 2016/06/29 09:14:52, Yuki wrote: > > > On 2016/06/29 09:09:52, haraken wrote: > > > > On 2016/06/29 09:06:01, peria wrote: > > > > > On 2016/06/29 08:51:18, haraken wrote: > > > > > > LGTM > > > > > > > > > > > > This CL changes existing behavior because after this CL > > > > ScriptValueSerializer > > > > > > starts crashing when toV8 returns an empty handle. However, thanks to > > > > > nhiroki's > > > > > > work, it's now super rare that toV8 returns an empty handle. So I'm > okay > > > > with > > > > > > the change. > > > > > > > > > > Before this CL, toV8Object() seems to return v8::Local<v8::Object>(), > > > > > and I think it is same with an empty handle > > > > > v8::Local<v8::Value>().As<v8::Object>(), > > > > > isn't it? > > > > > > > > I guess v8::Local<v8::Value>().As<v8::Object>() will crash (though I've > not > > > yet > > > > checked). > > > > > > v8::Local<v8::Value>().As<v8::Object>() should be okay, IIUC. > > > > > > v8::Null(isolate).As<v8::Object>() is NOT okay, though. > > > If |impl| is nullptr, then > > > toV8(impl, creationContext, isolate) > > > returns v8::Null(isolate). Then, we have a problem because we're doing > > > v8::Null(isolate).As<v8::Object>() > > > where a type mismatch occurs. > > > > Won't we crash when we run obj->IsJSReceiver() in CheckCast (when the type > check > > is enabled)? > > > > > https://cs.chromium.org/chromium/src/v8/src/api.cc?q=api.cc&sq=package:chromi... > > Ah, right. It does. > https://cs.chromium.org/chromium/src/v8/include/v8.h?sq=package:chromium&dr&r... > > Thank you for the clarification. Anyway, committing. We won't crash for the empty handle, I think? https://cs.chromium.org/chromium/src/v8/include/v8.h?sq=package:chromium&dr&l... And we crash for v8::Null, I think.
Message was sent while issue was closed.
Description was changed from ========== Remove toV8Object functions. This CL is a follow-up CL of https://codereview.chromium.org/2101953002/ BUG=614919 ========== to ========== Remove toV8Object functions. This CL is a follow-up CL of https://codereview.chromium.org/2101953002/ BUG=614919 ==========
Message was sent while issue was closed.
Committed patchset #1 (id:1)
Message was sent while issue was closed.
Description was changed from ========== Remove toV8Object functions. This CL is a follow-up CL of https://codereview.chromium.org/2101953002/ BUG=614919 ========== to ========== Remove toV8Object functions. This CL is a follow-up CL of https://codereview.chromium.org/2101953002/ BUG=614919 Committed: https://crrev.com/c1cf0eeba3a6d4541f8a2b7161cb0859a70a8493 Cr-Commit-Position: refs/heads/master@{#402778} ==========
Message was sent while issue was closed.
Patchset 1 (id:??) landed as https://crrev.com/c1cf0eeba3a6d4541f8a2b7161cb0859a70a8493 Cr-Commit-Position: refs/heads/master@{#402778} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
