Remove leak of objects between isolated worlds on custom events.

Source/bindings/v8/custom/V8CustomEventCustom.cpp

 /*
  * Copyright (C) 2012 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:
  *
  *     * Redistributions of source code must retain the above copyright
  * notice, this list of conditions and the following disclaimer.
  *     * Redistributions in binary form must reproduce the above
@@ skipping 27 matching lines @@
 #include "bindings/v8/V8Binding.h"
 #include "bindings/v8/V8DOMWrapper.h"
 #include "bindings/v8/V8HiddenPropertyName.h"
 #include "core/dom/ContextFeatures.h"
 #include "core/dom/ExceptionCode.h"
 #include "core/page/Frame.h"
 #include "RuntimeEnabledFeatures.h"
 
 namespace WebCore {
 
+// Save the detail value to a hidden attribute in the V8PCustomEvent, and return it, for convenience.
+static v8::Handle<v8::Value> cacheState(v8::Handle<v8::Object> customEvent, v8::Handle<v8::Value> detail)
+{
+    customEvent->SetHiddenValue(V8HiddenPropertyName::detail(), detail);
+    return detail;
+}
+
+
 void V8CustomEvent::detailAttrGetterCustom(v8::Local<v8::String> name, const v8::PropertyCallbackInfo<v8::Value>& info)
 {
-    CustomEvent* imp = V8CustomEvent::toNative(info.Holder());
-    RefPtr<SerializedScriptValue> serialized = imp->serializedScriptValue();
-    if (serialized) {
-        v8::Handle<v8::Value> value = info.Holder()->GetHiddenValue(V8HiddenPropertyName::detail());
-        if (value.IsEmpty()) {
-            value = serialized->deserialize();
-            info.Holder()->SetHiddenValue(V8HiddenPropertyName::detail(), value);
-        }
-        v8SetReturnValue(info, value);
+    CustomEvent* event = V8CustomEvent::toNative(info.Holder());
+    ASSERT(!event->serializedScriptValue().get());
+
+    v8::Handle<v8::Value> result = info.Holder()->GetHiddenValue(V8HiddenPropertyName::detail());
+
+    if (!result.IsEmpty()) {
+        v8SetReturnValue(info, result);
         return;
     }
-    v8SetReturnValue(info, imp->detail().v8Value());
+
+    // This is necessary because of the V8 bug 2746. V8 returns an empty
+    // handler when a hidden value is v8::Undefined. Thus, it is necessary to
+    // keep extra state around in the event about whether the value was set
+    // in the first place. That is, if the detail was set, and V8 returns an
+    // empty handler for the value, we know that the value must actually be a
+    // v8::Undefined(), so we explicitly set that here. Oy!
+    //
+    // Once bug 2746 is addressed, the following 'if' should become dead code
+    // and should be removable. Please see the related comments in
+    // V8MessageEventCustom.cpp and V8PopStateEventCustom.cpp as well.

[haraken, 2013/06/26 04:40:20]

I don't think this is a V8 bug nor you need to handle the case specially in the
binding layer.

I think you are talking about the following test cases:

  shouldBe("new CustomEvent('eventType', { detail: undefined }).detail",
"undefined");
  shouldBe("new CustomEvent('eventType', { detail: null }).detail", "null");
  shouldBe("new CustomEvent('eventType', { }).detail", "undefined");

To accomplish the above behavior, I guess you don't need to do any special
handling. Specifically:

- If v8::GetHiddenValue() returns an empty handle, then we can treat it as
undefined.
- Otherwise, we can treat the value returned by v8::GetHiddenValue() as is.

Am I missing something?

[jww, 2013/06/26 17:19:08]

If GetHiddenValue returns an empty v8::Handle, there are two possibilities:
(1) The value is v8::Undefined(), in which case we want to cache and return
v8::Undefined()
(2) The value was never set at all, in which case we want to check to see if a
serialized value was set, and if that was not the case either, than we want to
cache and return null (not undefined).

Thus, in the case that GetHiddenValue returns an empty v8::Handle *and* no
serialized script was set, we are unsure about whether to return v8::Undefined()
or null. The later should only be the case if it was explicitly set to null, but
right now we can't distinguish between the two cases without the logic I've
added.

Thus, I do believe this is a bug; currently, v8 does not allow us to distinguish
between two different but valid values that we can set a hidden property to,
undefined and null.

[adamk, 2013/06/26 17:43:38]

Note that this bug has now been fixed in
https://code.google.com/p/v8/source/detail?r=15329, so if we just wait for that
to roll into Chromium this change can be simplified (I'm fine with landing it
first and then simplifying later).

+    if (event->isDetailSet()) {
+        v8SetReturnValue(info, v8::Undefined());
+        return;
+    }
+
+    RefPtr<SerializedScriptValue> serialized = event->serializedScriptValue();
+    if (serialized) {
+        result = serialized->deserialize();
+        v8SetReturnValue(info, cacheState(info.Holder(), result));
+        return;
+    }
+
+    v8SetReturnValue(info, cacheState(info.Holder(), v8Null(info.GetIsolate())));
+}
+
+void V8CustomEvent::initCustomEventMethodCustom(const v8::FunctionCallbackInfo<v8::Value>& args)
+{
+    CustomEvent* event = V8CustomEvent::toNative(args.Holder());
+    String typeArg = toWebCoreString(args[0]);
+    bool canBubbleArg = args[1]->BooleanValue();
+    bool cancelableArg = args[2]->BooleanValue();
+    v8::Handle<v8::Value> detailsArg = args[3];
+
+    args.Holder()->SetHiddenValue(V8HiddenPropertyName::detail(), detailsArg);
+    event->initEvent(typeArg, canBubbleArg, cancelableArg);
 }
 
 } // namespace WebCore