Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2496533002:
binding: Makes Dictionary handle a possible exception in [[Get]]. (Closed)
Descriptionbinding: Makes Dictionary handle a possible exception in [[Get]].
Dictionary retrieves a dictionary member through the [[Get]] internal
method of the dictionary object. Since the properties can be accessor
properties, arbitrary script may run in [[Get]], so we have to handle
a possible exception in [[Get]].
The right fix is to propagate the exception up to the caller,
however, there are so many callers and the issue is urgent.
So simply swallowing all the exceptions in this CL, and fix it
re-throws in a follow-up CL.
BUG=644237
Committed: https://crrev.com/5f971b7bafe1c9a8574e66525b593644e4b9d658
Cr-Commit-Position: refs/heads/master@{#431848}
Patch Set 1 #Patch Set 2 : Added a layout test. #
Total comments: 15
Patch Set 3 : Synced. #Patch Set 4 : Added TODO comments + updated a test expectation. #
Total comments: 5
Patch Set 5 : Synced. #Patch Set 6 : Addressed review comments. #Patch Set 7 : Really added a short comment. (forgotten in the previous patch) #Patch Set 8 : Fixed a coding style. #
Messages
Total messages: 55 (31 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 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...
bashi@chromium.org changed reviewers: + bashi@chromium.org
lgtm, thank you for fixing bugs in Dictionary. https://codereview.chromium.org/2496533002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/bindings/core/v8/DictionaryIterator.cpp (left): https://codereview.chromium.org/2496533002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/bindings/core/v8/DictionaryIterator.cpp:32: // TODO(alancutter): Support callable objects as well as functions. Why did you remove this TODO? https://codereview.chromium.org/2496533002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/bindings/core/v8/DictionaryIterator.cpp (right): https://codereview.chromium.org/2496533002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/bindings/core/v8/DictionaryIterator.cpp:34: if (!v8Call(m_iterator->Get(m_isolate->GetCurrentContext(), m_nextKey), nit: Can we have two if blocks? if (!v8Call(...)) { exceptionState.rethrowV8Exception(tryCatch.Exception()); } else if (!next->IsFunction()) { exceptionState.throwTypeError("..."); }
haraken@chromium.org changed reviewers: + haraken@chromium.org
https://codereview.chromium.org/2496533002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/bindings/core/v8/DictionaryIterator.cpp (right): https://codereview.chromium.org/2496533002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/bindings/core/v8/DictionaryIterator.cpp:66: if (v8Call(resultObject->Get(m_isolate->GetCurrentContext(), m_doneKey), Isn't it possible that this Get fails if the above Get at line 63 has failed? However, then we need to check an exception every time we call Get, which is not really realistic...
https://codereview.chromium.org/2496533002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/bindings/core/v8/DictionaryIterator.cpp (right): https://codereview.chromium.org/2496533002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/bindings/core/v8/DictionaryIterator.cpp:66: if (v8Call(resultObject->Get(m_isolate->GetCurrentContext(), m_doneKey), On 2016/11/10 13:32:48, haraken wrote: > > Isn't it possible that this Get fails if the above Get at line 63 has failed? I think we get different properties ? ("value" and "done") > > However, then we need to check an exception every time we call Get, which is not > really realistic...
https://codereview.chromium.org/2496533002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/bindings/core/v8/DictionaryIterator.cpp (right): https://codereview.chromium.org/2496533002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/bindings/core/v8/DictionaryIterator.cpp:66: if (v8Call(resultObject->Get(m_isolate->GetCurrentContext(), m_doneKey), On 2016/11/10 13:35:00, bashi1 wrote: > On 2016/11/10 13:32:48, haraken wrote: > > > > Isn't it possible that this Get fails if the above Get at line 63 has failed? > I think we get different properties ? ("value" and "done") Sorry, I misunderstand your point. Not sure if it fails when line 63 has failed, but I think we might need to check Get() results every time... (toImplSequence() does) > > > > However, then we need to check an exception every time we call Get, which is > not > > really realistic... >
Hmm. As discussed in this bug (https://bugs.chromium.org/p/chromium/issues/detail?id=629070#c10), a right fix would be to replace Dictionary::get(...) with Dictionary::get(..., ExceptionState&) and let the callers handle the exception. - However, it will be a lot of work. We need some short-term fix. - On the other hand, I'm not sure if this CL is doing a right thing. We're suppressing an exception that should be handled by the callers. Any thoughts? https://codereview.chromium.org/2496533002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/bindings/core/v8/DictionaryIterator.cpp (right): https://codereview.chromium.org/2496533002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/bindings/core/v8/DictionaryIterator.cpp:66: if (v8Call(resultObject->Get(m_isolate->GetCurrentContext(), m_doneKey), On 2016/11/10 13:35:00, bashi1 wrote: > On 2016/11/10 13:32:48, haraken wrote: > > > > Isn't it possible that this Get fails if the above Get at line 63 has failed? > I think we get different properties ? ("value" and "done") > > > > However, then we need to check an exception every time we call Get, which is > not > > really realistic... > If V8 throws an exception, any subsequent V8 call will fail. So if the first Get has failed, the second Get will fail. At the moment, can we add v8Call() to the first Get?
> - On the other hand, I'm not sure if this CL is doing a right thing. We're suppressing an exception that should be handled by the callers. I'm not quite sure at this point, and the spec seems a little ambiguous (or unfriendly to readers). https://heycam.github.io/webidl/#es-dictionary 3.2.21. Dictionary types The spec does not mention to any abrupt completion case. My understanding is, if we simply followed the steps, and if the value is undefined in the case of abrupt completion, then we should ignore an exception and treat as if there is no such dictionary member. https://codereview.chromium.org/2496533002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/bindings/core/v8/DictionaryIterator.cpp (left): https://codereview.chromium.org/2496533002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/bindings/core/v8/DictionaryIterator.cpp:32: // TODO(alancutter): Support callable objects as well as functions. On 2016/11/10 13:32:42, bashi1 wrote: > Why did you remove this TODO? Forgot to mention it. obj->IsFunction() returns true if obj is callable, i.e. callable objects are functions by definition of ECMAScript. V8 is following this definition. https://codereview.chromium.org/2496533002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/bindings/core/v8/DictionaryIterator.cpp (right): https://codereview.chromium.org/2496533002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/bindings/core/v8/DictionaryIterator.cpp:34: if (!v8Call(m_iterator->Get(m_isolate->GetCurrentContext(), m_nextKey), On 2016/11/10 13:32:42, bashi1 wrote: > nit: Can we have two if blocks? > > if (!v8Call(...)) { > exceptionState.rethrowV8Exception(tryCatch.Exception()); > } else if (!next->IsFunction()) { > exceptionState.throwTypeError("..."); > } It fails if "next" key doesn't exist, too, even if there is no exception. !v8Call() doesn't guarantee an exception. https://codereview.chromium.org/2496533002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/bindings/core/v8/DictionaryIterator.cpp:66: if (v8Call(resultObject->Get(m_isolate->GetCurrentContext(), m_doneKey), On 2016/11/10 13:35:00, bashi1 wrote: > On 2016/11/10 13:32:48, haraken wrote: > > > > Isn't it possible that this Get fails if the above Get at line 63 has failed? > I think we get different properties ? ("value" and "done") > > > > However, then we need to check an exception every time we call Get, which is > not > > really realistic... > We're retrieving "value" and "done". If either of them threw an exception, we're rethrowing it on line 77. I think "any subsequent V8 call will fail." doesn't matter in this case.
On 2016/11/10 13:58:09, Yuki wrote: > > - On the other hand, I'm not sure if this CL is doing a right thing. We're > suppressing an exception that should be handled by the callers. > > I'm not quite sure at this point, and the spec seems a little ambiguous (or > unfriendly to readers). > > https://heycam.github.io/webidl/#es-dictionary > 3.2.21. Dictionary types > > The spec does not mention to any abrupt completion case. My understanding is, > if we simply followed the steps, and if the value is undefined in the case of > abrupt completion, then we should ignore an exception and treat as if there is > no such dictionary member. Yeah, thanks for the link. Shall we check with domenic@?
On 2016/11/10 13:46:03, haraken wrote: > Hmm. > > As discussed in this bug > (https://bugs.chromium.org/p/chromium/issues/detail?id=629070#c10), a right fix > would be to replace Dictionary::get(...) with Dictionary::get(..., > ExceptionState&) and let the callers handle the exception. > > - However, it will be a lot of work. We need some short-term fix. > > - On the other hand, I'm not sure if this CL is doing a right thing. We're > suppressing an exception that should be handled by the callers. > > Any thoughts? In my ideal world, Dictionary should be gone :) (use IDL dictionaries instead) We need a short-term fix and this fix would be an option as Dictionary::get() is fundamentally broken at this point... > > https://codereview.chromium.org/2496533002/diff/20001/third_party/WebKit/Sour... > File third_party/WebKit/Source/bindings/core/v8/DictionaryIterator.cpp (right): > > https://codereview.chromium.org/2496533002/diff/20001/third_party/WebKit/Sour... > third_party/WebKit/Source/bindings/core/v8/DictionaryIterator.cpp:66: if > (v8Call(resultObject->Get(m_isolate->GetCurrentContext(), m_doneKey), > On 2016/11/10 13:35:00, bashi1 wrote: > > On 2016/11/10 13:32:48, haraken wrote: > > > > > > Isn't it possible that this Get fails if the above Get at line 63 has > failed? > > I think we get different properties ? ("value" and "done") > > > > > > However, then we need to check an exception every time we call Get, which is > > not > > > really realistic... > > > > If V8 throws an exception, any subsequent V8 call will fail. So if the first Get > has failed, the second Get will fail. > > At the moment, can we add v8Call() to the first Get?
https://codereview.chromium.org/2496533002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/bindings/core/v8/DictionaryIterator.cpp (left): https://codereview.chromium.org/2496533002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/bindings/core/v8/DictionaryIterator.cpp:32: // TODO(alancutter): Support callable objects as well as functions. On 2016/11/10 13:58:09, Yuki wrote: > On 2016/11/10 13:32:42, bashi1 wrote: > > Why did you remove this TODO? > > Forgot to mention it. > > obj->IsFunction() returns true if obj is callable, i.e. callable objects are > functions by definition of ECMAScript. V8 is following this definition. Just to confirm: obj->IsCallable() is the same as obj->IsFunction() ? https://codereview.chromium.org/2496533002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/bindings/core/v8/DictionaryIterator.cpp (right): https://codereview.chromium.org/2496533002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/bindings/core/v8/DictionaryIterator.cpp:34: if (!v8Call(m_iterator->Get(m_isolate->GetCurrentContext(), m_nextKey), On 2016/11/10 13:58:09, Yuki wrote: > On 2016/11/10 13:32:42, bashi1 wrote: > > nit: Can we have two if blocks? > > > > if (!v8Call(...)) { > > exceptionState.rethrowV8Exception(tryCatch.Exception()); > > } else if (!next->IsFunction()) { > > exceptionState.throwTypeError("..."); > > } > > It fails if "next" key doesn't exist, too, even if there is no exception. > !v8Call() doesn't guarantee an exception. Then can we use ToLocal() ?
yukishiino@chromium.org changed reviewers: + domenic@chromium.org
Hi Domenic, Could you give us advice? We're considering what should happen if a dictionary member's [[Get]] throws. Dictionary is a simple ES object, and dictionary member is a property, and it can be an accessor property whose [[Get]] throws. https://heycam.github.io/webidl/#es-dictionary 3.2.21. Dictionary types The spec says we should use "the result of calling the [[Get]] internal method", however, what should we do if [[Get]] throws? The spec says nothing about abrupt completion case. I vaguely guess that we should ignore the exception and treat the dictionary member as if it doesn't exist (as value=undefined case), but not quite sure. What do you think?
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: 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_...)
You should rethrow the exception:). There are several other places in that algorithm that cause exceptions as well (e.g. conversions). We will make this clearer by changing the spec to read something like "value is ? Get(V, key)" where "?" is https://tc39.github.io/ecma262/#sec-algorithm-conventions (i.e. it means rethrow). On Thu, Nov 10, 2016 at 9:10 AM <yukishiino@chromium.org> wrote: > Hi Domenic, > > Could you give us advice? We're considering what should happen if a > dictionary > member's [[Get]] throws. > > Dictionary is a simple ES object, and dictionary member is a property, and > it > can be an accessor property whose [[Get]] throws. > > https://heycam.github.io/webidl/#es-dictionary > 3.2.21. Dictionary types > > The spec says we should use "the result of calling the [[Get]] internal > method", > however, what should we do if [[Get]] throws? The spec says nothing about > abrupt completion case. > > I vaguely guess that we should ignore the exception and treat the > dictionary > member as if it doesn't exist (as value=undefined case), but not quite > sure. > > What do you think? > > > https://codereview.chromium.org/2496533002/ > -- You received this message because you are subscribed to the Google Groups "Blink Reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to blink-reviews+unsubscribe@chromium.org.
You should rethrow the exception:). There are several other places in that algorithm that cause exceptions as well (e.g. conversions). We will make this clearer by changing the spec to read something like "value is ? Get(V, key)" where "?" is https://tc39.github.io/ecma262/#sec-algorithm-conventions (i.e. it means rethrow). On Thu, Nov 10, 2016 at 9:10 AM <yukishiino@chromium.org> wrote: > Hi Domenic, > > Could you give us advice? We're considering what should happen if a > dictionary > member's [[Get]] throws. > > Dictionary is a simple ES object, and dictionary member is a property, and > it > can be an accessor property whose [[Get]] throws. > > https://heycam.github.io/webidl/#es-dictionary > 3.2.21. Dictionary types > > The spec says we should use "the result of calling the [[Get]] internal > method", > however, what should we do if [[Get]] throws? The spec says nothing about > abrupt completion case. > > I vaguely guess that we should ignore the exception and treat the > dictionary > member as if it doesn't exist (as value=undefined case), but not quite > sure. > > What do you think? > > > https://codereview.chromium.org/2496533002/ > -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
Posted https://github.com/heycam/webidl/pull/234 to make this clearer. On Thu, Nov 10, 2016 at 1:44 PM Domenic Denicola <domenic@google.com> wrote: > You should rethrow the exception:). There are several other places in that > algorithm that cause exceptions as well (e.g. conversions). > > We will make this clearer by changing the spec to read something like > "value is ? Get(V, key)" where "?" is > https://tc39.github.io/ecma262/#sec-algorithm-conventions (i.e. it means > rethrow). > > On Thu, Nov 10, 2016 at 9:10 AM <yukishiino@chromium.org> wrote: > > Hi Domenic, > > Could you give us advice? We're considering what should happen if a > dictionary > member's [[Get]] throws. > > Dictionary is a simple ES object, and dictionary member is a property, and > it > can be an accessor property whose [[Get]] throws. > > https://heycam.github.io/webidl/#es-dictionary > 3.2.21. Dictionary types > > The spec says we should use "the result of calling the [[Get]] internal > method", > however, what should we do if [[Get]] throws? The spec says nothing about > abrupt completion case. > > I vaguely guess that we should ignore the exception and treat the > dictionary > member as if it doesn't exist (as value=undefined case), but not quite > sure. > > What do you think? > > > https://codereview.chromium.org/2496533002/ > > -- You received this message because you are subscribed to the Google Groups "Blink Reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to blink-reviews+unsubscribe@chromium.org.
Posted https://github.com/heycam/webidl/pull/234 to make this clearer. On Thu, Nov 10, 2016 at 1:44 PM Domenic Denicola <domenic@google.com> wrote: > You should rethrow the exception:). There are several other places in that > algorithm that cause exceptions as well (e.g. conversions). > > We will make this clearer by changing the spec to read something like > "value is ? Get(V, key)" where "?" is > https://tc39.github.io/ecma262/#sec-algorithm-conventions (i.e. it means > rethrow). > > On Thu, Nov 10, 2016 at 9:10 AM <yukishiino@chromium.org> wrote: > > Hi Domenic, > > Could you give us advice? We're considering what should happen if a > dictionary > member's [[Get]] throws. > > Dictionary is a simple ES object, and dictionary member is a property, and > it > can be an accessor property whose [[Get]] throws. > > https://heycam.github.io/webidl/#es-dictionary > 3.2.21. Dictionary types > > The spec says we should use "the result of calling the [[Get]] internal > method", > however, what should we do if [[Get]] throws? The spec says nothing about > abrupt completion case. > > I vaguely guess that we should ignore the exception and treat the > dictionary > member as if it doesn't exist (as value=undefined case), but not quite > sure. > > What do you think? > > > https://codereview.chromium.org/2496533002/ > > -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
Thanks Domenic. I'll update the CL to rethrow the exception.
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...
Description was changed from ========== binding: Makes Dictionary handle a possible exception in [[Get]]. Dictionary retrieves a dictionary member through the [[Get]] internal method of the dictionary object. Since the properties can be accessor properties, arbitrary script may run in [[Get]], so we have to handle a possible exception in [[Get]]. BUG=644237 ========== to ========== binding: Makes Dictionary handle a possible exception in [[Get]]. Dictionary retrieves a dictionary member through the [[Get]] internal method of the dictionary object. Since the properties can be accessor properties, arbitrary script may run in [[Get]], so we have to handle a possible exception in [[Get]]. The right fix is to propagate the exception up to the caller, however, there are so many callers and the issue is urgent. So simply swallowing all the exceptions in this CL, and fix it re-throws in a follow-up CL. BUG=644237 ==========
It turned out that it wouldn't be easy to fix all the callers to pass an ExceptionState, and this is a ReleaseBlock-Beta/Dev issue, so let me go with this CL with TODO comments for the time being. We'll implement the right fix later. https://codereview.chromium.org/2496533002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/bindings/core/v8/DictionaryIterator.cpp (left): https://codereview.chromium.org/2496533002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/bindings/core/v8/DictionaryIterator.cpp:32: // TODO(alancutter): Support callable objects as well as functions. On 2016/11/10 14:08:30, bashi1 wrote: > On 2016/11/10 13:58:09, Yuki wrote: > > On 2016/11/10 13:32:42, bashi1 wrote: > > > Why did you remove this TODO? > > > > Forgot to mention it. > > > > obj->IsFunction() returns true if obj is callable, i.e. callable objects are > > functions by definition of ECMAScript. V8 is following this definition. > > Just to confirm: obj->IsCallable() is the same as obj->IsFunction() ? I think so. https://cs.chromium.org/chromium/src/v8/src/api.cc?q=Value::IsFunction+file:v... https://codereview.chromium.org/2496533002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/bindings/core/v8/DictionaryIterator.cpp (right): https://codereview.chromium.org/2496533002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/bindings/core/v8/DictionaryIterator.cpp:34: if (!v8Call(m_iterator->Get(m_isolate->GetCurrentContext(), m_nextKey), On 2016/11/10 14:08:30, bashi1 wrote: > On 2016/11/10 13:58:09, Yuki wrote: > > On 2016/11/10 13:32:42, bashi1 wrote: > > > nit: Can we have two if blocks? > > > > > > if (!v8Call(...)) { > > > exceptionState.rethrowV8Exception(tryCatch.Exception()); > > > } else if (!next->IsFunction()) { > > > exceptionState.throwTypeError("..."); > > > } > > > > It fails if "next" key doesn't exist, too, even if there is no exception. > > !v8Call() doesn't guarantee an exception. > > Then can we use ToLocal() ? I don't understand how ToLocal() helps this case. In general, I think it's risky to assume how Get fails. MaybeLocal could be empty for a variety of reasons depending on V8 implementation, and it's possible that no exception was thrown.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2496533002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/bindings/core/v8/DictionaryIterator.cpp (right): https://codereview.chromium.org/2496533002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/bindings/core/v8/DictionaryIterator.cpp:34: if (!v8Call(m_iterator->Get(m_isolate->GetCurrentContext(), m_nextKey), On 2016/11/11 15:14:29, Yuki wrote: > On 2016/11/10 14:08:30, bashi1 wrote: > > On 2016/11/10 13:58:09, Yuki wrote: > > > On 2016/11/10 13:32:42, bashi1 wrote: > > > > nit: Can we have two if blocks? > > > > > > > > if (!v8Call(...)) { > > > > exceptionState.rethrowV8Exception(tryCatch.Exception()); > > > > } else if (!next->IsFunction()) { > > > > exceptionState.throwTypeError("..."); > > > > } > > > > > > It fails if "next" key doesn't exist, too, even if there is no exception. > > > !v8Call() doesn't guarantee an exception. > > > > Then can we use ToLocal() ? > > I don't understand how ToLocal() helps this case. In general, I think it's > risky to assume how Get fails. MaybeLocal could be empty for a variety of > reasons depending on V8 implementation, and it's possible that no exception was > thrown. My point is we should handle the failure reason appropriately. ToLocal() fail means there is an exception. On the other hand, IsFunction() == false is a semantics failure. The former we should just rethrow the exeption but the latter we should throw a type error (as you did). I wanted to form the code blocks to express the intent explicitly.
LGTM as a short-term fix. https://codereview.chromium.org/2496533002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/bindings/core/v8/Dictionary.cpp (right): https://codereview.chromium.org/2496533002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/bindings/core/v8/Dictionary.cpp:140: if (!v8CallBoolean(object->Has(v8Context(), key))) Just to confirm: The line 138 and 140 never throws an exception, right? https://codereview.chromium.org/2496533002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/bindings/core/v8/DictionaryIterator.cpp (right): https://codereview.chromium.org/2496533002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/bindings/core/v8/DictionaryIterator.cpp:63: m_value = resultObject->Get(m_isolate->GetCurrentContext(), m_valueKey); Nit: I'd prefer forming the code so that it checks an exception every time we call Get.
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/2496533002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/bindings/core/v8/DictionaryIterator.cpp (right): https://codereview.chromium.org/2496533002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/bindings/core/v8/DictionaryIterator.cpp:34: if (!v8Call(m_iterator->Get(m_isolate->GetCurrentContext(), m_nextKey), On 2016/11/12 01:16:59, bashi1 wrote: > On 2016/11/11 15:14:29, Yuki wrote: > > On 2016/11/10 14:08:30, bashi1 wrote: > > > On 2016/11/10 13:58:09, Yuki wrote: > > > > On 2016/11/10 13:32:42, bashi1 wrote: > > > > > nit: Can we have two if blocks? > > > > > > > > > > if (!v8Call(...)) { > > > > > exceptionState.rethrowV8Exception(tryCatch.Exception()); > > > > > } else if (!next->IsFunction()) { > > > > > exceptionState.throwTypeError("..."); > > > > > } > > > > > > > > It fails if "next" key doesn't exist, too, even if there is no exception. > > > > !v8Call() doesn't guarantee an exception. > > > > > > Then can we use ToLocal() ? > > > > I don't understand how ToLocal() helps this case. In general, I think it's > > risky to assume how Get fails. MaybeLocal could be empty for a variety of > > reasons depending on V8 implementation, and it's possible that no exception > was > > thrown. > > My point is we should handle the failure reason appropriately. ToLocal() fail > means there is an exception. On the other hand, IsFunction() == false is a > semantics failure. The former we should just rethrow the exeption but the latter > we should throw a type error (as you did). I wanted to form the code blocks to > express the intent explicitly. I've completely forgotten the rule that, whenever a Maybe is empty, an exception must be thrown. Now I understand your point. https://codereview.chromium.org/2496533002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/bindings/core/v8/Dictionary.cpp (right): https://codereview.chromium.org/2496533002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/bindings/core/v8/Dictionary.cpp:140: if (!v8CallBoolean(object->Has(v8Context(), key))) On 2016/11/12 07:45:13, haraken wrote: > > Just to confirm: The line 138 and 140 never throws an exception, right? I expected so, but after talking with bashi@, I recognized it could throw. IMHO, the right way to go is that a) Stop using v8Call macros, and use ToLocal or To directly. b) If a Maybe or MaybeLocal is empty, there must be an exception. Then, we can correctly detect a possible exception. Added another (short) comment in TODO. Actually there should be no case that propertyKey() throws, so don't worry too much for now. https://codereview.chromium.org/2496533002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/bindings/core/v8/DictionaryIterator.cpp (right): https://codereview.chromium.org/2496533002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/bindings/core/v8/DictionaryIterator.cpp:63: m_value = resultObject->Get(m_isolate->GetCurrentContext(), m_valueKey); On 2016/11/12 07:45:13, haraken wrote: > > Nit: I'd prefer forming the code so that it checks an exception every time we > call Get. Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
https://codereview.chromium.org/2496533002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/bindings/core/v8/Dictionary.cpp (right): https://codereview.chromium.org/2496533002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/bindings/core/v8/Dictionary.cpp:140: if (!v8CallBoolean(object->Has(v8Context(), key))) On 2016/11/14 06:57:25, Yuki wrote: > On 2016/11/12 07:45:13, haraken wrote: > > > > Just to confirm: The line 138 and 140 never throws an exception, right? > > I expected so, but after talking with bashi@, I recognized it could throw. > > IMHO, the right way to go is that > a) Stop using v8Call macros, and use ToLocal or To directly. > b) If a Maybe or MaybeLocal is empty, there must be an exception. > Then, we can correctly detect a possible exception. > > Added another (short) comment in TODO. > > Actually there should be no case that propertyKey() throws, so don't worry too > much for now. Sounds reasonable. Our plan is to deprecate v8Call() and use To/ToLocal instead.
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...
still lgtm
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: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
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 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 haraken@chromium.org, bashi@chromium.org Link to the patchset: https://codereview.chromium.org/2496533002/#ps140001 (title: "Fixed a coding style.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== binding: Makes Dictionary handle a possible exception in [[Get]]. Dictionary retrieves a dictionary member through the [[Get]] internal method of the dictionary object. Since the properties can be accessor properties, arbitrary script may run in [[Get]], so we have to handle a possible exception in [[Get]]. The right fix is to propagate the exception up to the caller, however, there are so many callers and the issue is urgent. So simply swallowing all the exceptions in this CL, and fix it re-throws in a follow-up CL. BUG=644237 ========== to ========== binding: Makes Dictionary handle a possible exception in [[Get]]. Dictionary retrieves a dictionary member through the [[Get]] internal method of the dictionary object. Since the properties can be accessor properties, arbitrary script may run in [[Get]], so we have to handle a possible exception in [[Get]]. The right fix is to propagate the exception up to the caller, however, there are so many callers and the issue is urgent. So simply swallowing all the exceptions in this CL, and fix it re-throws in a follow-up CL. BUG=644237 ==========
Message was sent while issue was closed.
Committed patchset #8 (id:140001)
Message was sent while issue was closed.
Description was changed from ========== binding: Makes Dictionary handle a possible exception in [[Get]]. Dictionary retrieves a dictionary member through the [[Get]] internal method of the dictionary object. Since the properties can be accessor properties, arbitrary script may run in [[Get]], so we have to handle a possible exception in [[Get]]. The right fix is to propagate the exception up to the caller, however, there are so many callers and the issue is urgent. So simply swallowing all the exceptions in this CL, and fix it re-throws in a follow-up CL. BUG=644237 ========== to ========== binding: Makes Dictionary handle a possible exception in [[Get]]. Dictionary retrieves a dictionary member through the [[Get]] internal method of the dictionary object. Since the properties can be accessor properties, arbitrary script may run in [[Get]], so we have to handle a possible exception in [[Get]]. The right fix is to propagate the exception up to the caller, however, there are so many callers and the issue is urgent. So simply swallowing all the exceptions in this CL, and fix it re-throws in a follow-up CL. BUG=644237 Committed: https://crrev.com/5f971b7bafe1c9a8574e66525b593644e4b9d658 Cr-Commit-Position: refs/heads/master@{#431848} ==========
Message was sent while issue was closed.
Patchset 8 (id:??) landed as https://crrev.com/5f971b7bafe1c9a8574e66525b593644e4b9d658 Cr-Commit-Position: refs/heads/master@{#431848} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
