third_party/WebKit/Source/bindings/core/v8/Dictionary.h - Issue 2519403002: binding: Lets Dictionary::getPropertyNames, etc. rethrow an exception.

Unified Diff: third_party/WebKit/Source/bindings/core/v8/Dictionary.h

Issue 2519403002: binding: Lets Dictionary::getPropertyNames, etc. rethrow an exception. (Closed)

Patch Set: Fixed DictionaryTest. Created 4 years, 1 month ago

Use n/p to move between diff chunks; N/P to move between comments. Draft comments are only viewable by you.

Jump to:

View side-by-side diff with in-line comments

Index: third_party/WebKit/Source/bindings/core/v8/Dictionary.h

diff --git a/third_party/WebKit/Source/bindings/core/v8/Dictionary.h b/third_party/WebKit/Source/bindings/core/v8/Dictionary.h

index d280c7065be2aad800ddccfce7c951d9d4cd10e9..313681f5241961f777f272a6e03736d02ca67182 100644

--- a/third_party/WebKit/Source/bindings/core/v8/Dictionary.h

+++ b/third_party/WebKit/Source/bindings/core/v8/Dictionary.h

@@ -78,8 +78,11 @@ class CORE_EXPORT Dictionary final {

}

bool get(const StringView& key, Dictionary&) const;

- bool getOwnPropertiesAsStringHashMap(HashMap<String, String>&) const;

- bool getPropertyNames(Vector<String>&) const;

+ WARN_UNUSED_RESULT bool getOwnPropertiesAsStringHashMap(

+ HashMap<String, String>&,

+ ExceptionState&) const;

+ WARN_UNUSED_RESULT bool getPropertyNames(Vector<String>&,

+ ExceptionState&) const;

haraken 2016/11/24 07:22:30 Is it guaranteed that this method returns false if

Yuki 2016/11/24 10:19:32 I like the rule to guarantee that "false in the re

haraken 2016/11/24 12:39:12 I understand your point but other places in the co

Yuki 2016/11/25 08:31:04 Can we change that rule right away? It's a bit er

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?

haraken 2016/11/25 09:14:51 However, then we need to update the entire code ba

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);

Yuki 2016/11/25 09:58:54 Ah, I thought you wanted to return void. You actu

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?

haraken 2016/11/25 10:14:37 Yes. However, my point is that the caller of a me

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>.

Yuki 2016/11/25 14:50:25 Per offline discussion, we're going to let the cod

bool hasProperty(const StringView&) const;

« no previous file with comments | « no previous file | third_party/WebKit/Source/bindings/core/v8/Dictionary.cpp » ('j') | third_party/WebKit/Source/bindings/core/v8/Dictionary.cpp » ('J')