Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2519403002:
binding: Lets Dictionary::getPropertyNames, etc. rethrow an exception. (Closed)
|
Created:
4 years, 1 month ago by Yuki Modified:
4 years ago CC:
chromium-reviews, shans, rjwright, rouslan+payments_chromium.org, blink-reviews-animation_chromium.org, tommyw+watchlist_chromium.org, blink-reviews-bindings_chromium.org, darktears, blink-reviews, sebsg+paymentswatch_chromium.org, mcasas+watch+mediastream_chromium.org, Eric Willigers Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Descriptionbinding: Lets Dictionary::getPropertyNames, etc. rethrow an exception.
Lets Dictionary::getPropertyNames and getOwnPropertiesAsStringHashMap
rethrow an exception up to the call sites.
BUG=666661
Committed: https://crrev.com/541073fb340487f9e29f0eefa10ce4f7548f49c4
Cr-Commit-Position: refs/heads/master@{#434516}
Patch Set 1 #Patch Set 2 : Fixed DictionaryTest. #
Total comments: 28
Patch Set 3 : Addressed review comments. #Patch Set 4 : Synced. #Patch Set 5 : Synced. #Patch Set 6 : Addressed review comments. #Messages
Total messages: 43 (25 generated)
The CQ bit was checked by yukishiino@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 yukishiino@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.
yukishiino@chromium.org changed reviewers: + bashi@chromium.org, haraken@chromium.org
Could you guys review this CL?
peria@chromium.org changed reviewers: + peria@chromium.org
https://codereview.chromium.org/2519403002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/bindings/core/v8/V8StringResource.h (right): https://codereview.chromium.org/2519403002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/bindings/core/v8/V8StringResource.h:208: if (prepareFast()) Can't we make this like prepare(isoalte, state)? return prepareFast() || prepareSlow(v8::Isolate::GetCurrent(), exceptionState); https://codereview.chromium.org/2519403002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/bindings/core/v8/V8StringResource.h:238: bool prepareSlow(v8::Isolate* isolate, ExceptionState& exceptionState) { nit: put an empty line here
https://codereview.chromium.org/2519403002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/bindings/core/v8/Dictionary.cpp (right): https://codereview.chromium.org/2519403002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/bindings/core/v8/Dictionary.cpp:121: // lets an exception be thrown. It's the caller's duty to catch the exception. Then would it be better to make the method return a MaybeLocal handle? https://codereview.chromium.org/2519403002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/bindings/core/v8/Dictionary.h (right): https://codereview.chromium.org/2519403002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/bindings/core/v8/Dictionary.h:85: ExceptionState&) const; Is it guaranteed that this method returns false if and only if the exception state is set? If yes, can we just ask the caller to check exceptionState.hadException after calling this method? That's what we're doing in other methods that take an ExceptionState parameter. https://codereview.chromium.org/2519403002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/bindings/core/v8/V8StringResource.h (right): https://codereview.chromium.org/2519403002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/bindings/core/v8/V8StringResource.h:190: bool prepare() { This should be deprecated as well, right? https://codereview.chromium.org/2519403002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/bindings/core/v8/V8StringResource.h:199: return m_v8Object->ToString(v8::Isolate::GetCurrent()->GetCurrentContext()) Not related to your CL but why don't you need to have a TryCatch block before calling ToString? https://codereview.chromium.org/2519403002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/mediastream/MediaConstraintsImpl.cpp (right): https://codereview.chromium.org/2519403002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/mediastream/MediaConstraintsImpl.cpp:155: TrackExceptionState exceptionState; We should avoid using TrackExceptionState in production code. Shall we add a TODO to fix it? https://codereview.chromium.org/2519403002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/payments/PaymentRequest.cpp (right): https://codereview.chromium.org/2519403002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/payments/PaymentRequest.cpp:365: TrackExceptionState trackExceptionState; Why can't you use exceptionState here?
https://codereview.chromium.org/2519403002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/bindings/core/v8/V8StringResource.h (right): https://codereview.chromium.org/2519403002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/bindings/core/v8/V8StringResource.h:196: // TODO(bashi): Pass an exceptionState to this function and handle an Why me? :)
A more meta question is: How hard would it be to replace Dictionaries with IDL dictionaries (instead of fixing bugs of Dictionaries)?
The CQ bit was checked by yukishiino@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...
Could you guys take another look?
On 2016/11/24 07:35:55, haraken wrote:
> A more meta question is: How hard would it be to replace Dictionaries with IDL
> dictionaries (instead of fixing bugs of Dictionaries)?
I think V8FooDictionary is (or should be, or looks much similar to) IDL
dictionary.
IDL dictionary is something like:
dictionary Foo {
DOMString requiredStr;
DOMString? optionalStr;
};
where a) the dictionary has its name "Foo", and b) the dictionary defines its
dictionary members with type, name, optionality, etc.
Dictionary class in C++ has no name, no member definition, etc. So, it's a kind
of utility/helper to implement V8FooDictionary, which looks like an IDL
dictionary (somewhat more at least).
In order to make V8FooDictionary conformant to the IDL dictionary, we have to
make Dictionary (partially) conformant to the spec, too, I think.
https://codereview.chromium.org/2519403002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/bindings/core/v8/Dictionary.cpp (right):
https://codereview.chromium.org/2519403002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/core/v8/Dictionary.cpp:121: // lets an
exception be thrown. It's the caller's duty to catch the exception.
On 2016/11/24 07:22:30, haraken wrote:
>
> Then would it be better to make the method return a MaybeLocal handle?
Done.
https://codereview.chromium.org/2519403002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/bindings/core/v8/Dictionary.h (right):
https://codereview.chromium.org/2519403002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/core/v8/Dictionary.h:85: ExceptionState&)
const;
On 2016/11/24 07:22:30, haraken wrote:
>
> Is it guaranteed that this method returns false if and only if the exception
> state is set?
>
> If yes, can we just ask the caller to check exceptionState.hadException after
> calling this method? That's what we're doing in other methods that take an
> ExceptionState parameter.
I like the rule to guarantee that "false in the return value" == "exception was
thrown".
We can make this rule true by replacing
if (m_dictionaryObject.IsEmpty())
return false;
with
DCHECK(!m_dictionaryObject.IsEmpty());
and I like this idea, though I'm not sure how easy this change will be.
Orthogonal to this idea, I think the return value is useful for the call sites
even with the above rule.
T* T::method(ExceptionState& exceptionState) {
ValueType1 value1;
ValueType2 value2;
if (!dict.get("key1", value1, exceptionState) ||
!dict.get("key2", value2, exceptionState)) {
return nullptr;
}
// do something with value1 and value2.
}
If we made the return type of the function void, then, the code looks like:
dict.get("key1", value1, exceptionState);
if (exceptionState.hadException())
return nullptr;
dict.get("key2", value2, exceptionState);
if (exceptionState.hadException())
return nullptr;
It looks for me inconvenient for the call sites.
https://codereview.chromium.org/2519403002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/bindings/core/v8/V8StringResource.h (right):
https://codereview.chromium.org/2519403002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/core/v8/V8StringResource.h:190: bool
prepare() {
On 2016/11/24 07:22:30, haraken wrote:
>
> This should be deprecated as well, right?
Done.
https://codereview.chromium.org/2519403002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/core/v8/V8StringResource.h:196: //
TODO(bashi): Pass an exceptionState to this function and handle an
On 2016/11/24 07:25:53, bashi1 wrote:
> Why me? :)
Done. X(
Removed TODO comments entirely because I made this version of prepare()
DEPRECATED.
https://codereview.chromium.org/2519403002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/core/v8/V8StringResource.h:199: return
m_v8Object->ToString(v8::Isolate::GetCurrent()->GetCurrentContext())
On 2016/11/24 07:22:30, haraken wrote:
>
> Not related to your CL but why don't you need to have a TryCatch block before
> calling ToString?
If you were talking about "the best coding practice", then we do need a TryCatch
block, or we should use ToLocalChecked().
The reason why this works so far (at least for most cases) is
https://cs.chromium.org/chromium/src/out/Debug/gen/blink/bindings/core/v8/V8A...
As soon as prepare() returned false, we return to V8, so letting an exception be
thrown is okay.
Probably, we have some cases that don't work well, but I'm not sure...
https://codereview.chromium.org/2519403002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/core/v8/V8StringResource.h:208: if
(prepareFast())
On 2016/11/24 07:21:23, peria wrote:
> Can't we make this like prepare(isoalte, state)?
>
> return prepareFast() || prepareSlow(v8::Isolate::GetCurrent(),
exceptionState);
Done.
https://codereview.chromium.org/2519403002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/core/v8/V8StringResource.h:238: bool
prepareSlow(v8::Isolate* isolate, ExceptionState& exceptionState) {
On 2016/11/24 07:21:23, peria wrote:
> nit: put an empty line here
Done.
https://codereview.chromium.org/2519403002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/mediastream/MediaConstraintsImpl.cpp
(right):
https://codereview.chromium.org/2519403002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/mediastream/MediaConstraintsImpl.cpp:155:
TrackExceptionState exceptionState;
On 2016/11/24 07:22:31, haraken wrote:
>
> We should avoid using TrackExceptionState in production code. Shall we add a
> TODO to fix it?
MediaConstraintsImpl looks a little bit different. See the call sites of this
function and APIs in MediaConstraintsImpl.
MediaContraintsImpl is intentionally using MediaErrorState, which looks similar
to ExceptionState but different.
https://cs.chromium.org/chromium/src/third_party/WebKit/Source/modules/medias...
MediaContraintsImpl was designed to use MediaErrorState to delay an error
handling. If parseMandatoryConstraintsDictionary() returned false, then the
call sites create an appropriate MediaErrorState. So we don't need to have an
ExceptionState here (at least with this approach).
Thus, it seems okay for me to just use TrackExceptionState.
https://codereview.chromium.org/2519403002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/payments/PaymentRequest.cpp (right):
https://codereview.chromium.org/2519403002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/payments/PaymentRequest.cpp:365:
TrackExceptionState trackExceptionState;
On 2016/11/24 07:22:31, haraken wrote:
>
> Why can't you use exceptionState here?
Reading the call site of this function, as "maybe" in the function name implies,
this operation is totally "optional" and okay to fail. The call site doesn't
want this function to throw. So, the original code intentionally has used
TrackExceptionState to swallow a possible exception.
I just keep the same behavior as before.
The reason why I let getPropertyNames() on line 410 throw an exception is
because it's unusual. getPropertyNames() rarely fails and it's not unlike a
simple type mismatch. So I think we don't want to ignore this kind of
exceptions.
lgtm from my side
lgtm on my side too.
> In order to make V8FooDictionary conformant to the IDL dictionary, we have to
> make Dictionary (partially) conformant to the spec, too, I think.
I guess that haraken-san's question was that can we remove "Dictionary" from
*.idl files and define "dictionary D { ... }" as needed.
I don't think it's an option for now. It requires a bunch of refactoring.
Specifically core/animation heavily uses Dictionary. Personally I want to remove
Dictionary class though :(
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: blimp_linux_dbg on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) cast_shell_linux on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) chromeos_daisy_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) chromeos_x86-generic_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) chromium_presubmit on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) linux_chromium_chromeos_compile_dbg_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) linux_chromium_clobber_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) linux_chromium_compile_dbg_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL)
LGTM % the first comment below. https://codereview.chromium.org/2519403002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/bindings/core/v8/Dictionary.h (right): https://codereview.chromium.org/2519403002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/bindings/core/v8/Dictionary.h:85: ExceptionState&) const; On 2016/11/24 10:19:32, Yuki wrote: > On 2016/11/24 07:22:30, haraken wrote: > > > > Is it guaranteed that this method returns false if and only if the exception > > state is set? > > > > If yes, can we just ask the caller to check exceptionState.hadException after > > calling this method? That's what we're doing in other methods that take an > > ExceptionState parameter. > > I like the rule to guarantee that "false in the return value" == "exception was > thrown". > > We can make this rule true by replacing > if (m_dictionaryObject.IsEmpty()) > return false; > with > DCHECK(!m_dictionaryObject.IsEmpty()); > and I like this idea, though I'm not sure how easy this change will be. > > Orthogonal to this idea, I think the return value is useful for the call sites > even with the above rule. > T* T::method(ExceptionState& exceptionState) { > ValueType1 value1; > ValueType2 value2; > if (!dict.get("key1", value1, exceptionState) || > !dict.get("key2", value2, exceptionState)) { > return nullptr; > } > // do something with value1 and value2. > } > If we made the return type of the function void, then, the code looks like: > dict.get("key1", value1, exceptionState); > if (exceptionState.hadException()) > return nullptr; > dict.get("key2", value2, exceptionState); > if (exceptionState.hadException()) > return nullptr; > It looks for me inconvenient for the call sites. I understand your point but other places in the code base follow the exceptionState.hadException rule. So I want to follow the existing rule until we decide to change the rule. https://codereview.chromium.org/2519403002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/mediastream/MediaConstraintsImpl.cpp (right): https://codereview.chromium.org/2519403002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/mediastream/MediaConstraintsImpl.cpp:155: TrackExceptionState exceptionState; On 2016/11/24 10:19:33, Yuki wrote: > On 2016/11/24 07:22:31, haraken wrote: > > > > We should avoid using TrackExceptionState in production code. Shall we add a > > TODO to fix it? > > MediaConstraintsImpl looks a little bit different. See the call sites of this > function and APIs in MediaConstraintsImpl. > > MediaContraintsImpl is intentionally using MediaErrorState, which looks similar > to ExceptionState but different. > https://cs.chromium.org/chromium/src/third_party/WebKit/Source/modules/medias... > > MediaContraintsImpl was designed to use MediaErrorState to delay an error > handling. If parseMandatoryConstraintsDictionary() returned false, then the > call sites create an appropriate MediaErrorState. So we don't need to have an > ExceptionState here (at least with this approach). > > Thus, it seems okay for me to just use TrackExceptionState. Okay. However, in long term I think we should replace MediaErrorState with ExceptionState and remove TrackExceptionState though. https://codereview.chromium.org/2519403002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/payments/PaymentRequest.cpp (right): https://codereview.chromium.org/2519403002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/payments/PaymentRequest.cpp:365: TrackExceptionState trackExceptionState; On 2016/11/24 10:19:33, Yuki wrote: > On 2016/11/24 07:22:31, haraken wrote: > > > > Why can't you use exceptionState here? > > Reading the call site of this function, as "maybe" in the function name implies, > this operation is totally "optional" and okay to fail. The call site doesn't > want this function to throw. So, the original code intentionally has used > TrackExceptionState to swallow a possible exception. > > I just keep the same behavior as before. > > The reason why I let getPropertyNames() on line 410 throw an exception is > because it's unusual. getPropertyNames() rarely fails and it's not unlike a > simple type mismatch. So I think we don't want to ignore this kind of > exceptions. That sounds too arbitrary to me though. To keep the existing behavior, I'm okay with this CL, but in long term I think we should try to remove TrackExceptionState from production code. In limited cases where we really want to ignore a thrown exception, we can explicitly call exceptionState.clearException.
The CQ bit was checked by yukishiino@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...
https://codereview.chromium.org/2519403002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/bindings/core/v8/Dictionary.h (right): https://codereview.chromium.org/2519403002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/bindings/core/v8/Dictionary.h:85: ExceptionState&) const; On 2016/11/24 12:39:12, haraken wrote: > On 2016/11/24 10:19:32, Yuki wrote: > > On 2016/11/24 07:22:30, haraken wrote: > > > > > > Is it guaranteed that this method returns false if and only if the exception > > > state is set? > > > > > > If yes, can we just ask the caller to check exceptionState.hadException > after > > > calling this method? That's what we're doing in other methods that take an > > > ExceptionState parameter. > > > > I like the rule to guarantee that "false in the return value" == "exception > was > > thrown". > > > > We can make this rule true by replacing > > if (m_dictionaryObject.IsEmpty()) > > return false; > > with > > DCHECK(!m_dictionaryObject.IsEmpty()); > > and I like this idea, though I'm not sure how easy this change will be. > > > > Orthogonal to this idea, I think the return value is useful for the call sites > > even with the above rule. > > T* T::method(ExceptionState& exceptionState) { > > ValueType1 value1; > > ValueType2 value2; > > if (!dict.get("key1", value1, exceptionState) || > > !dict.get("key2", value2, exceptionState)) { > > return nullptr; > > } > > // do something with value1 and value2. > > } > > If we made the return type of the function void, then, the code looks like: > > dict.get("key1", value1, exceptionState); > > if (exceptionState.hadException()) > > return nullptr; > > dict.get("key2", value2, exceptionState); > > if (exceptionState.hadException()) > > return nullptr; > > It looks for me inconvenient for the call sites. > > I understand your point but other places in the code base follow the > exceptionState.hadException rule. So I want to follow the existing rule until we > decide to change the rule. Can we change that rule right away? It's a bit error-prone. For example, the original version of this function returns a bool value (without taking an ExceptionState), but some of the call sites just ignore the return value. So, I've added WARN_UNUSED_RESULT to avoid this kind of error. If this function returns void, we cannot detect such an error. If the style/rule is only the problem, then can we change the style/rule right now? https://codereview.chromium.org/2519403002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/mediastream/MediaConstraintsImpl.cpp (right): https://codereview.chromium.org/2519403002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/mediastream/MediaConstraintsImpl.cpp:155: TrackExceptionState exceptionState; On 2016/11/24 12:39:13, haraken wrote: > On 2016/11/24 10:19:33, Yuki wrote: > > On 2016/11/24 07:22:31, haraken wrote: > > > > > > We should avoid using TrackExceptionState in production code. Shall we add a > > > TODO to fix it? > > > > MediaConstraintsImpl looks a little bit different. See the call sites of this > > function and APIs in MediaConstraintsImpl. > > > > MediaContraintsImpl is intentionally using MediaErrorState, which looks > similar > > to ExceptionState but different. > > > https://cs.chromium.org/chromium/src/third_party/WebKit/Source/modules/medias... > > > > MediaContraintsImpl was designed to use MediaErrorState to delay an error > > handling. If parseMandatoryConstraintsDictionary() returned false, then the > > call sites create an appropriate MediaErrorState. So we don't need to have an > > ExceptionState here (at least with this approach). > > > > Thus, it seems okay for me to just use TrackExceptionState. > > Okay. However, in long term I think we should replace MediaErrorState with > ExceptionState and remove TrackExceptionState though. Acknowledged. https://codereview.chromium.org/2519403002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/payments/PaymentRequest.cpp (right): https://codereview.chromium.org/2519403002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/payments/PaymentRequest.cpp:365: TrackExceptionState trackExceptionState; On 2016/11/24 12:39:13, haraken wrote: > On 2016/11/24 10:19:33, Yuki wrote: > > On 2016/11/24 07:22:31, haraken wrote: > > > > > > Why can't you use exceptionState here? > > > > Reading the call site of this function, as "maybe" in the function name > implies, > > this operation is totally "optional" and okay to fail. The call site doesn't > > want this function to throw. So, the original code intentionally has used > > TrackExceptionState to swallow a possible exception. > > > > I just keep the same behavior as before. > > > > The reason why I let getPropertyNames() on line 410 throw an exception is > > because it's unusual. getPropertyNames() rarely fails and it's not unlike a > > simple type mismatch. So I think we don't want to ignore this kind of > > exceptions. > > That sounds too arbitrary to me though. To keep the existing behavior, I'm okay > with this CL, but in long term I think we should try to remove > TrackExceptionState from production code. In limited cases where we really want > to ignore a thrown exception, we can explicitly call > exceptionState.clearException. > Acknowledged.
https://codereview.chromium.org/2519403002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/bindings/core/v8/Dictionary.h (right): https://codereview.chromium.org/2519403002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/bindings/core/v8/Dictionary.h:85: ExceptionState&) const; On 2016/11/25 08:31:04, Yuki wrote: > On 2016/11/24 12:39:12, haraken wrote: > > On 2016/11/24 10:19:32, Yuki wrote: > > > On 2016/11/24 07:22:30, haraken wrote: > > > > > > > > Is it guaranteed that this method returns false if and only if the > exception > > > > state is set? > > > > > > > > If yes, can we just ask the caller to check exceptionState.hadException > > after > > > > calling this method? That's what we're doing in other methods that take an > > > > ExceptionState parameter. > > > > > > I like the rule to guarantee that "false in the return value" == "exception > > was > > > thrown". > > > > > > We can make this rule true by replacing > > > if (m_dictionaryObject.IsEmpty()) > > > return false; > > > with > > > DCHECK(!m_dictionaryObject.IsEmpty()); > > > and I like this idea, though I'm not sure how easy this change will be. > > > > > > Orthogonal to this idea, I think the return value is useful for the call > sites > > > even with the above rule. > > > T* T::method(ExceptionState& exceptionState) { > > > ValueType1 value1; > > > ValueType2 value2; > > > if (!dict.get("key1", value1, exceptionState) || > > > !dict.get("key2", value2, exceptionState)) { > > > return nullptr; > > > } > > > // do something with value1 and value2. > > > } > > > If we made the return type of the function void, then, the code looks like: > > > dict.get("key1", value1, exceptionState); > > > if (exceptionState.hadException()) > > > return nullptr; > > > dict.get("key2", value2, exceptionState); > > > if (exceptionState.hadException()) > > > return nullptr; > > > It looks for me inconvenient for the call sites. > > > > I understand your point but other places in the code base follow the > > exceptionState.hadException rule. So I want to follow the existing rule until > we > > decide to change the rule. > > Can we change that rule right away? It's a bit error-prone. > > For example, the original version of this function returns a bool value (without > taking an ExceptionState), but some of the call sites just ignore the return > value. So, I've added WARN_UNUSED_RESULT to avoid this kind of error. If this > function returns void, we cannot detect such an error. > > If the style/rule is only the problem, then can we change the style/rule right > now? However, then we need to update the entire code base... For example, you need to rewrite: String foo(..., ExceptionState& exceptionState); to: bool foo(..., String& outValue, ExceptionState& exceptionState);
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2519403002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/bindings/core/v8/Dictionary.h (right): https://codereview.chromium.org/2519403002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/bindings/core/v8/Dictionary.h:85: ExceptionState&) const; On 2016/11/25 09:14:51, haraken wrote: > On 2016/11/25 08:31:04, Yuki wrote: > > On 2016/11/24 12:39:12, haraken wrote: > > > On 2016/11/24 10:19:32, Yuki wrote: > > > > On 2016/11/24 07:22:30, haraken wrote: > > > > > > > > > > Is it guaranteed that this method returns false if and only if the > > exception > > > > > state is set? > > > > > > > > > > If yes, can we just ask the caller to check exceptionState.hadException > > > after > > > > > calling this method? That's what we're doing in other methods that take > an > > > > > ExceptionState parameter. > > > > > > > > I like the rule to guarantee that "false in the return value" == > "exception > > > was > > > > thrown". > > > > > > > > We can make this rule true by replacing > > > > if (m_dictionaryObject.IsEmpty()) > > > > return false; > > > > with > > > > DCHECK(!m_dictionaryObject.IsEmpty()); > > > > and I like this idea, though I'm not sure how easy this change will be. > > > > > > > > Orthogonal to this idea, I think the return value is useful for the call > > sites > > > > even with the above rule. > > > > T* T::method(ExceptionState& exceptionState) { > > > > ValueType1 value1; > > > > ValueType2 value2; > > > > if (!dict.get("key1", value1, exceptionState) || > > > > !dict.get("key2", value2, exceptionState)) { > > > > return nullptr; > > > > } > > > > // do something with value1 and value2. > > > > } > > > > If we made the return type of the function void, then, the code looks > like: > > > > dict.get("key1", value1, exceptionState); > > > > if (exceptionState.hadException()) > > > > return nullptr; > > > > dict.get("key2", value2, exceptionState); > > > > if (exceptionState.hadException()) > > > > return nullptr; > > > > It looks for me inconvenient for the call sites. > > > > > > I understand your point but other places in the code base follow the > > > exceptionState.hadException rule. So I want to follow the existing rule > until > > we > > > decide to change the rule. > > > > Can we change that rule right away? It's a bit error-prone. > > > > For example, the original version of this function returns a bool value > (without > > taking an ExceptionState), but some of the call sites just ignore the return > > value. So, I've added WARN_UNUSED_RESULT to avoid this kind of error. If > this > > function returns void, we cannot detect such an error. > > > > If the style/rule is only the problem, then can we change the style/rule right > > now? > > However, then we need to update the entire code base... For example, you need to > rewrite: > > String foo(..., ExceptionState& exceptionState); > > to: > > bool foo(..., String& outValue, ExceptionState& exceptionState); Ah, I thought you wanted to return void. You actually meant: Vector<String> getPropertyNames(ExceptionState&); right? This makes sense (though the same problem lies; it's possible that the call sites don't check a possible exception). The original problem is that Dictionary class did not handle an exception correctly, and then Blink crashed. V8 introduced Maybe in order to force the call sites to check a possible error. Then, what do you think of the problem? I'm fine with the above signature for the time being, but it means that we don't solve anything; the call sites wouldn't handle an exception and Blink may crash somewhere else, and it's really hard to debug such crashes because the crashing point has nothing wrong, the problem (ignoring an exception) had already happened somewhere else we really don't know. Usually, there is no clue about who forgot to check an exception. WARN_UNUSED_RESULT is somewhat similar to v8::Maybe and helpful to catch such errors, I think. If we want, we can use it for new APIs at least (even when we don't change the existing APIs), and also it's possible that we stick to the old style. It seems we're facing to the same problem that V8 faced to, and we need a solution, I guess?
https://codereview.chromium.org/2519403002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/bindings/core/v8/Dictionary.h (right): https://codereview.chromium.org/2519403002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/bindings/core/v8/Dictionary.h:85: ExceptionState&) const; On 2016/11/25 09:58:54, Yuki wrote: > On 2016/11/25 09:14:51, haraken wrote: > > On 2016/11/25 08:31:04, Yuki wrote: > > > On 2016/11/24 12:39:12, haraken wrote: > > > > On 2016/11/24 10:19:32, Yuki wrote: > > > > > On 2016/11/24 07:22:30, haraken wrote: > > > > > > > > > > > > Is it guaranteed that this method returns false if and only if the > > > exception > > > > > > state is set? > > > > > > > > > > > > If yes, can we just ask the caller to check > exceptionState.hadException > > > > after > > > > > > calling this method? That's what we're doing in other methods that > take > > an > > > > > > ExceptionState parameter. > > > > > > > > > > I like the rule to guarantee that "false in the return value" == > > "exception > > > > was > > > > > thrown". > > > > > > > > > > We can make this rule true by replacing > > > > > if (m_dictionaryObject.IsEmpty()) > > > > > return false; > > > > > with > > > > > DCHECK(!m_dictionaryObject.IsEmpty()); > > > > > and I like this idea, though I'm not sure how easy this change will be. > > > > > > > > > > Orthogonal to this idea, I think the return value is useful for the call > > > sites > > > > > even with the above rule. > > > > > T* T::method(ExceptionState& exceptionState) { > > > > > ValueType1 value1; > > > > > ValueType2 value2; > > > > > if (!dict.get("key1", value1, exceptionState) || > > > > > !dict.get("key2", value2, exceptionState)) { > > > > > return nullptr; > > > > > } > > > > > // do something with value1 and value2. > > > > > } > > > > > If we made the return type of the function void, then, the code looks > > like: > > > > > dict.get("key1", value1, exceptionState); > > > > > if (exceptionState.hadException()) > > > > > return nullptr; > > > > > dict.get("key2", value2, exceptionState); > > > > > if (exceptionState.hadException()) > > > > > return nullptr; > > > > > It looks for me inconvenient for the call sites. > > > > > > > > I understand your point but other places in the code base follow the > > > > exceptionState.hadException rule. So I want to follow the existing rule > > until > > > we > > > > decide to change the rule. > > > > > > Can we change that rule right away? It's a bit error-prone. > > > > > > For example, the original version of this function returns a bool value > > (without > > > taking an ExceptionState), but some of the call sites just ignore the return > > > value. So, I've added WARN_UNUSED_RESULT to avoid this kind of error. If > > this > > > function returns void, we cannot detect such an error. > > > > > > If the style/rule is only the problem, then can we change the style/rule > right > > > now? > > > > However, then we need to update the entire code base... For example, you need > to > > rewrite: > > > > String foo(..., ExceptionState& exceptionState); > > > > to: > > > > bool foo(..., String& outValue, ExceptionState& exceptionState); > > Ah, I thought you wanted to return void. You actually meant: > Vector<String> getPropertyNames(ExceptionState&); > right? This makes sense (though the same problem lies; it's possible that the > call sites don't check a possible exception). Yes. However, my point is that the caller of a method that takes an ExceptionState should check exeptionState.hadException(). Then you don't need to return true/false from the method. Then you can just make the method return Vector<String>. > > The original problem is that Dictionary class did not handle an exception > correctly, and then Blink crashed. V8 introduced Maybe in order to force the > call sites to check a possible error. Then, what do you think of the problem? > I'm fine with the above signature for the time being, but it means that we don't > solve anything; the call sites wouldn't handle an exception and Blink may crash > somewhere else, and it's really hard to debug such crashes because the crashing > point has nothing wrong, the problem (ignoring an exception) had already > happened somewhere else we really don't know. Usually, there is no clue about > who forgot to check an exception. > > WARN_UNUSED_RESULT is somewhat similar to v8::Maybe and helpful to catch such > errors, I think. If we want, we can use it for new APIs at least (even when we > don't change the existing APIs), and also it's possible that we stick to the old > style. > > It seems we're facing to the same problem that V8 faced to, and we need a > solution, I guess?
The CQ bit was checked by yukishiino@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...
Could you take another look? Updated the function signatures. https://codereview.chromium.org/2519403002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/bindings/core/v8/Dictionary.h (right): https://codereview.chromium.org/2519403002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/bindings/core/v8/Dictionary.h:85: ExceptionState&) const; Per offline discussion, we're going to let the coding style have priority over error proof.
LGTM
The CQ bit was unchecked by yukishiino@chromium.org
The CQ bit was checked by yukishiino@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from peria@chromium.org, bashi@chromium.org Link to the patchset: https://codereview.chromium.org/2519403002/#ps100001 (title: "Addressed review comments.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 100001, "attempt_start_ts": 1480085760878000,
"parent_rev": "3bbf38073174ee82e0b33568d534a2a7de14d97a", "commit_rev":
"0f0aa8f6cab6bf736d14f3066d0537314a87ec38"}
Message was sent while issue was closed.
Committed patchset #6 (id:100001)
Message was sent while issue was closed.
Description was changed from ========== binding: Lets Dictionary::getPropertyNames, etc. rethrow an exception. Lets Dictionary::getPropertyNames and getOwnPropertiesAsStringHashMap rethrow an exception up to the call sites. BUG=666661 ========== to ========== binding: Lets Dictionary::getPropertyNames, etc. rethrow an exception. Lets Dictionary::getPropertyNames and getOwnPropertiesAsStringHashMap rethrow an exception up to the call sites. BUG=666661 Committed: https://crrev.com/541073fb340487f9e29f0eefa10ce4f7548f49c4 Cr-Commit-Position: refs/heads/master@{#434516} ==========
Message was sent while issue was closed.
Patchset 6 (id:??) landed as https://crrev.com/541073fb340487f9e29f0eefa10ce4f7548f49c4 Cr-Commit-Position: refs/heads/master@{#434516} |
