Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
606603002:
Conversion of Promise is trivial (Closed)
|
Created:
6 years, 2 months ago by Jens Widell Modified:
6 years, 2 months ago CC:
blink-reviews, blink-reviews-bindings_chromium.org, arv+blink, abarth-chromium Base URL:
https://chromium.googlesource.com/chromium/blink.git@master Project:
blink Visibility:
Public. |
DescriptionConversion of Promise is trivial
Promise values are converted using ScriptPromise::cast(), which is trivial
insofar as it can't cause an exception to be thrown.
BUG=413257
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=182676
Patch Set 1 #
Messages
Total messages: 12 (2 generated)
jl@opera.com changed reviewers: + haraken@chromium.org, yhirano@chromium.org
PTAL Please confirm my reasoning here: ScriptPromise::cast() (via ScriptPromise::InternalResolver::resolve()) calls into V8 using an API that certainly looks like it might throw exceptions. My assumption here is that what ScriptPromise::cast() does is a "trivial" packaging-up of a value in an already-resolved promise, but I won't claim to fully understand this code.
On 2014/09/25 09:31:33, Jens Widell wrote: > PTAL > > Please confirm my reasoning here: ScriptPromise::cast() (via > ScriptPromise::InternalResolver::resolve()) calls into V8 using an API that > certainly looks like it might throw exceptions. > > My assumption here is that what ScriptPromise::cast() does is a "trivial" > packaging-up of a value in an already-resolved promise, but I won't claim to > fully understand this code. Hmm, it looks like that this is the case currently, but I'm not sure if it's reasonable to put the assumption for the ScriptPromise::cast(). hirano-san?
On 2014/09/25 09:35:55, haraken wrote: > On 2014/09/25 09:31:33, Jens Widell wrote: > > PTAL > > > > Please confirm my reasoning here: ScriptPromise::cast() (via > > ScriptPromise::InternalResolver::resolve()) calls into V8 using an API that > > certainly looks like it might throw exceptions. > > > > My assumption here is that what ScriptPromise::cast() does is a "trivial" > > packaging-up of a value in an already-resolved promise, but I won't claim to > > fully understand this code. > > Hmm, it looks like that this is the case currently, but I'm not sure if it's > reasonable to put the assumption for the ScriptPromise::cast(). > > hirano-san? Conceptually, Promise.resolve doesn't throw an exception. Strictly speaking, it can throw an exception in the case of stack overflow, but currently the case is not cared enough and I don't know if we should handle it. By the way, the predicate for the next block, "!promiseArg.isUndefinedOrNull() && !promiseArg.isObject()", is nonsense.
On 2014/09/25 10:57:56, yhirano wrote: > On 2014/09/25 09:35:55, haraken wrote: > > On 2014/09/25 09:31:33, Jens Widell wrote: > > > PTAL > > > > > > Please confirm my reasoning here: ScriptPromise::cast() (via > > > ScriptPromise::InternalResolver::resolve()) calls into V8 using an API that > > > certainly looks like it might throw exceptions. > > > > > > My assumption here is that what ScriptPromise::cast() does is a "trivial" > > > packaging-up of a value in an already-resolved promise, but I won't claim to > > > fully understand this code. > > > > Hmm, it looks like that this is the case currently, but I'm not sure if it's > > reasonable to put the assumption for the ScriptPromise::cast(). > > > > hirano-san? > > Conceptually, Promise.resolve doesn't throw an exception. Thanks. That means that this change should be correct. > Strictly speaking, it can throw an exception in the case of stack overflow, but > currently the case is not cared enough and I don't know if we should handle it. I suspect stack overflow exceptions are not generally well handled, when they occur in calls that otherwise normally wouldn't throw. > By the way, the predicate for the next block, "!promiseArg.isUndefinedOrNull() > && !promiseArg.isObject()", is nonsense. You mean it can never be true? I guess we should remove the methods.cpp:203-211 block then? (In a different CL.)
On 2014/09/25 11:04:16, Jens Widell wrote: > On 2014/09/25 10:57:56, yhirano wrote: > > On 2014/09/25 09:35:55, haraken wrote: > > > On 2014/09/25 09:31:33, Jens Widell wrote: > > > > PTAL > > > > > > > > Please confirm my reasoning here: ScriptPromise::cast() (via > > > > ScriptPromise::InternalResolver::resolve()) calls into V8 using an API > that > > > > certainly looks like it might throw exceptions. > > > > > > > > My assumption here is that what ScriptPromise::cast() does is a "trivial" > > > > packaging-up of a value in an already-resolved promise, but I won't claim > to > > > > fully understand this code. > > > > > > Hmm, it looks like that this is the case currently, but I'm not sure if it's > > > reasonable to put the assumption for the ScriptPromise::cast(). > > > > > > hirano-san? > > > > Conceptually, Promise.resolve doesn't throw an exception. > > Thanks. That means that this change should be correct. What happens if Promise.resolve throws an exception by mistake? Can we easily notice it and fix the code?
On 2014/09/25 11:10:45, haraken wrote:
> On 2014/09/25 11:04:16, Jens Widell wrote:
> > On 2014/09/25 10:57:56, yhirano wrote:
> > > On 2014/09/25 09:35:55, haraken wrote:
> > > > On 2014/09/25 09:31:33, Jens Widell wrote:
> > > > > PTAL
> > > > >
> > > > > Please confirm my reasoning here: ScriptPromise::cast() (via
> > > > > ScriptPromise::InternalResolver::resolve()) calls into V8 using an API
> > that
> > > > > certainly looks like it might throw exceptions.
> > > > >
> > > > > My assumption here is that what ScriptPromise::cast() does is a
> "trivial"
> > > > > packaging-up of a value in an already-resolved promise, but I won't
> claim
> > to
> > > > > fully understand this code.
> > > >
> > > > Hmm, it looks like that this is the case currently, but I'm not sure if
> it's
> > > > reasonable to put the assumption for the ScriptPromise::cast().
> > > >
> > > > hirano-san?
> > >
> > > Conceptually, Promise.resolve doesn't throw an exception.
> >
> > Thanks. That means that this change should be correct.
>
> What happens if Promise.resolve throws an exception by mistake? Can we easily
> notice it and fix the code?
It would be easy to build some debugging mechanism for detecting unexpected
exceptions in/from V8. Something like
#ifdef ENABLE(ASSERT)
class V8NoExceptionsScope {
v8::TryCatch block;
public:
~V8NoExceptionsScope() {
ASSERT(!block.HasCaught());
}
};
#else
class V8NoExceptionsScope {};
#endif
for instance (but with a few more bells and whistles, I'd imagine.)
On 2014/09/25 11:17:54, Jens Widell wrote:
> On 2014/09/25 11:10:45, haraken wrote:
> > On 2014/09/25 11:04:16, Jens Widell wrote:
> > > On 2014/09/25 10:57:56, yhirano wrote:
> > > > On 2014/09/25 09:35:55, haraken wrote:
> > > > > On 2014/09/25 09:31:33, Jens Widell wrote:
> > > > > > PTAL
> > > > > >
> > > > > > Please confirm my reasoning here: ScriptPromise::cast() (via
> > > > > > ScriptPromise::InternalResolver::resolve()) calls into V8 using an
API
> > > that
> > > > > > certainly looks like it might throw exceptions.
> > > > > >
> > > > > > My assumption here is that what ScriptPromise::cast() does is a
> > "trivial"
> > > > > > packaging-up of a value in an already-resolved promise, but I won't
> > claim
> > > to
> > > > > > fully understand this code.
> > > > >
> > > > > Hmm, it looks like that this is the case currently, but I'm not sure
if
> > it's
> > > > > reasonable to put the assumption for the ScriptPromise::cast().
> > > > >
> > > > > hirano-san?
> > > >
> > > > Conceptually, Promise.resolve doesn't throw an exception.
> > >
> > > Thanks. That means that this change should be correct.
> >
> > What happens if Promise.resolve throws an exception by mistake? Can we
easily
> > notice it and fix the code?
>
> It would be easy to build some debugging mechanism for detecting unexpected
> exceptions in/from V8. Something like
>
> #ifdef ENABLE(ASSERT)
> class V8NoExceptionsScope {
> v8::TryCatch block;
> public:
> ~V8NoExceptionsScope() {
> ASSERT(!block.HasCaught());
> }
> };
> #else
> class V8NoExceptionsScope {};
> #endif
>
> for instance (but with a few more bells and whistles, I'd imagine.)
Yeah, I'm not quite sure if it's worth introducing the mechanism (it adds a mess
to the generated code:-), but either way this CL LGTM.
The CQ bit was checked by jl@opera.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/606603002/1
On 2014/09/25 11:04:16, Jens Widell wrote: > On 2014/09/25 10:57:56, yhirano wrote: > > On 2014/09/25 09:35:55, haraken wrote: > > > On 2014/09/25 09:31:33, Jens Widell wrote: > > > > PTAL > > > > > > > > Please confirm my reasoning here: ScriptPromise::cast() (via > > > > ScriptPromise::InternalResolver::resolve()) calls into V8 using an API > that > > > > certainly looks like it might throw exceptions. > > > > > > > > My assumption here is that what ScriptPromise::cast() does is a "trivial" > > > > packaging-up of a value in an already-resolved promise, but I won't claim > to > > > > fully understand this code. > > > > > > Hmm, it looks like that this is the case currently, but I'm not sure if it's > > > reasonable to put the assumption for the ScriptPromise::cast(). > > > > > > hirano-san? > > > > Conceptually, Promise.resolve doesn't throw an exception. > > Thanks. That means that this change should be correct. > > > Strictly speaking, it can throw an exception in the case of stack overflow, > but > > currently the case is not cared enough and I don't know if we should handle > it. > > I suspect stack overflow exceptions are not generally well handled, when they > occur in calls that otherwise normally wouldn't throw. > > > By the way, the predicate for the next block, "!promiseArg.isUndefinedOrNull() > > && !promiseArg.isObject()", is nonsense. > > You mean it can never be true? I guess we should remove the methods.cpp:203-211 > block then? (In a different CL.) I think so. proiseArg is 1) a Promise object (in most cases) or 2) an empty value (stack overflow). If we don't care about (2), it will never be true.
Message was sent while issue was closed.
Committed patchset #1 (id:1) as 182676 |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
