binding: Lets Dictionary::getPropertyNames, etc. rethrow an exception.

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

 /*
  * Copyright (C) 2010 Google Inc. All rights reserved.
  *
  * Redistribution and use in source and binary forms, with or without
  * modification, are permitted provided that the following conditions
  * are met:
  *
  * 1.  Redistributions of source code must retain the above copyright
  *     notice, this list of conditions and the following disclaimer.
  * 2.  Redistributions in binary form must reproduce the above copyright
@@ skipping 60 matching lines @@
         NOTREACHED();
         return v8::Local<v8::Value>();
     }
   }
 
   bool get(const StringView& key, v8::Local<v8::Value>& value) const {
     return m_isolate && getInternal(v8String(m_isolate, key), value);
   }
   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 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.

[Yuki, 2016/11/24 10:19:32]

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.

[haraken, 2016/11/24 12:39:12]

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.

[Yuki, 2016/11/25 08:31:04]

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 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 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 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 coding style have priority over
error proof.

 
   bool hasProperty(const StringView&) const;
 
   v8::Isolate* isolate() const { return m_isolate; }
   v8::Local<v8::Context> v8Context() const {
     ASSERT(m_isolate);
     return m_isolate->GetCurrentContext();
   }
 
   DictionaryIterator getIterator(ExecutionContext*) const;
@@ skipping 50 matching lines @@
   static bool get(const Dictionary&,
                   const StringView& key,
                   PointerType<T>& value);
   template <typename T>
   static bool get(const Dictionary&, const StringView& key, Nullable<T>& value);
 };
 
 }  // namespace blink
 
 #endif  // Dictionary_h