bindings: Use V8 MaybeLocal<> APIs in Dictionary class

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 11 matching lines @@
  * (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE OF
  * THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
  */
 
 #ifndef Dictionary_h
 #define Dictionary_h
 
 #include "bindings/core/v8/ExceptionMessages.h"
 #include "bindings/core/v8/ExceptionState.h"
 #include "bindings/core/v8/Nullable.h"
+#include "bindings/core/v8/ScriptState.h"
 #include "bindings/core/v8/ScriptValue.h"
 #include "bindings/core/v8/V8Binding.h"
 #include "bindings/core/v8/V8BindingMacros.h"
 #include "wtf/HashMap.h"
 #include "wtf/HashSet.h"
 #include "wtf/Vector.h"
 #include "wtf/text/AtomicString.h"
 #include "wtf/text/WTFString.h"
 #include <v8.h>
 
 namespace blink {
 
 // Dictionary class provides ways to retrieve property values as C++ objects
 // from a V8 object. Instances of this class must not outlive V8's handle scope
 // because they hold a V8 value without putting it on persistent handles.
-class Dictionary {
+class Dictionary final : public ScriptState::Observer {

[haraken, 2015/03/03 05:12:15]

ScriptState::Observer is just intended to be used for some inspector-specific
reason, and it's highly discouraged (because the lifetime of ScriptState is
already complicated and we don't want to introduce something more complicated
like observers).

I don't think you need the observer. See my comment below.

[bashi, 2015/03/03 05:56:28]

I see.

     ALLOW_ONLY_INLINE_ALLOCATION();
 public:
     Dictionary();
     Dictionary(const v8::Handle<v8::Value>& options, v8::Isolate*, ExceptionState&);
-    ~Dictionary();
+    virtual ~Dictionary();
 
     Dictionary& operator=(const Dictionary&);
 
     // This is different from the default constructor:
     //   * isObject() is true when using createEmpty().
     //   * isUndefinedOrNull() is true when using default constructor.
     static Dictionary createEmpty(v8::Isolate*);
 
     bool isObject() const;
     bool isUndefinedOrNull() const;
@@ skipping 50 matching lines @@
     };
 
     bool convert(ConversionContext&, const String&, Dictionary&) const;
 
     bool getOwnPropertiesAsStringHashMap(HashMap<String, String>&) const;
     bool getPropertyNames(Vector<String>&) const;
 
     bool hasProperty(const String&) const;
 
     v8::Isolate* isolate() const { return m_isolate; }
+    // Used by DictionaryHelper. This context could be an empty handle and
+    // should be used only for V8 APIs which return MaybeLocal<T>.
+    v8::Local<v8::Context> context() const { return m_context; }
 
     bool getKey(const String& key, v8::Local<v8::Value>&) const;
 
 private:
+    // ScriptState::Observer implementation
+    virtual void willDisposeScriptState(ScriptState*) override;
+
+    void subscribeScriptStateDisposal();
+    void unsubscribeScriptStateDisposal();
+
     v8::Handle<v8::Value> m_options;
+    v8::Local<v8::Context> m_context;

[bashi, 2015/03/03 04:25:05]

Should we hold ScriptState instead of v8::Local<v8::Context> ?

[haraken, 2015/03/03 05:12:15]

A big YES. If a DOM object needs to keep some script state (including Context),
the DOM object should hold RefPtr<ScriptState>. It is scary to hold a Context
and I've replaced almost all of them with RefPtr<ScriptState> before :)

However, in this particular case, Dictionary is guaranteed to be used on stack,
which means that we're guaranteed to be in a correct ScriptState already when
Dictionary is used. Thus what we should do here is:

- Guarantee that Dictionary is used only on stack. I think this is already
guaranteed in practice but we have no way to verify the fact.

- Guarantee that we're always in a correct ScriptState. I hope this is already
guaranteed. (As discussed offline, this is hard to verify.)

- Just use m_isolate->GetCurrentContext() in Dictionary methods.

[bashi, 2015/03/03 05:56:28]

Yeah, relying on RefPtr looks much better, and will follow your suggestion. But
I have one concern: Won't we want to stop using GetCurrentContext() in the near
future? If we want to stop using it, I guess that we would need this kind of
weak reference (or make Dictionary take ScriptState each time get() called).

One of the reasons why I choose Dictionary as the first instance was to make
sure that this kind of our assumption is reasonable for the time being:)

[haraken, 2015/03/03 06:12:14]

I'm sorry for not being clear.

Using GetCurrentContext() is completely fine (Note: We want to remove
Isolate::GetCurrent(), but that's a totally different issue than
Isolate::GetCurrentContext()). What we must make sure is that we are always in a
correct context by entering a correct ScriptState::Scope. Being in a correct
context is a mandatory requirement to make the system work correctly. And once
it's guaranteed (I hope it's already guaranteed but it's very hard to guarantee
the fact), GetCurrentContext() is guaranteed to return a correct context
whenever it's called, so using GetCurrentContext() is totally fine.

In short, what we should do is not to avoid GetCurrentContext but to guarantee
that we're always in a correct context (by entering a correct
ScriptState::Scope).

     v8::Isolate* m_isolate;
     ExceptionState* m_exceptionState;
 };
 
 template<>
 struct NativeValueTraits<Dictionary> {
     static inline Dictionary nativeValue(const v8::Handle<v8::Value>& value, v8::Isolate* isolate, ExceptionState& exceptionState)
     {
         return Dictionary(value, isolate, exceptionState);
     }
@@ skipping 22 matching lines @@
     static bool convert(const Dictionary&, Dictionary::ConversionContext&, const String& key, T& value);
     template <typename T>
     static bool convert(const Dictionary&, Dictionary::ConversionContext&, const String& key, Nullable<T>& value);
     template <template <typename> class PointerType, typename T>
     static bool convert(const Dictionary&, Dictionary::ConversionContext&, const String& key, PointerType<T>& value);
 };
 
 }
 
 #endif // Dictionary_h