Remove leak of objects between isolated worlds on custom events.

Source/bindings/v8/custom/V8CustomEventCustom.cpp

Index: Source/bindings/v8/custom/V8CustomEventCustom.cpp
diff --git a/Source/bindings/v8/custom/V8CustomEventCustom.cpp b/Source/bindings/v8/custom/V8CustomEventCustom.cpp
index ff0f9b355c75a404d34f7125047a40ec5070b14d..44d3daea5d5a6d7a41c68478544cd4408db7da59 100644
--- a/Source/bindings/v8/custom/V8CustomEventCustom.cpp
+++ b/Source/bindings/v8/custom/V8CustomEventCustom.cpp
@@ -45,6 +45,32 @@
 
 namespace WebCore {
 
+void V8CustomEvent::constructorCustom(const v8::FunctionCallbackInfo<v8::Value>& args)

[haraken, 2013/06/22 13:16:46]

We don't want to increase custom bindings, which are likely to be buggy. Would
it be possible to auto-generator the code? I think you can improve the generated
code for ConstructorTemplate=Event so that SetHiddenValue() is generated for
attributes of type 'any'.

[jww, 2013/06/25 01:50:29]

Good idea. I'v generalized the constructor to take care of this. This has the
advantage of also addressing http://crbug.com/251970 as well.

+{
+    if (args.Length() < 1) {
+        throwNotEnoughArgumentsError(args.GetIsolate());
+        return;
+    }
+
+    String type = toWebCoreString(args[0]);
+    CustomEventInit eventInit;
+
+    if (args.Length() >= 2) {
+        Dictionary options = Dictionary(args[1], args.GetIsolate());

[adamk, 2013/06/22 01:00:04]

This syntax looks odd to me, can you get rid of the extra "Dictionary" and just
say options(...) instead?

[jww, 2013/06/25 01:50:29]

Done.

+        if (!fillEventInit(eventInit, options))
+            return;
+        v8::Local<v8::Value> detail;
+        options.getKey("detail", detail);
+        args.Holder()->SetHiddenValue(V8HiddenPropertyName::detail(), detail);

[adamk, 2013/06/22 01:00:04]

Guard this with an if() check on the above? Or at least that detail is
non-empty?

[jww, 2013/06/25 01:50:29]

I wrapped the "SetHiddenValue" with an "if" check for "!detail.IsEmpty()". Is
that what you meant?

+    }
+
+    RefPtr<CustomEvent> event = CustomEvent::create(type, eventInit);
+
+    v8::Handle<v8::Object> wrapper = args.Holder();
+    V8DOMWrapper::associateObjectWithWrapper(event.release(), &V8CustomEvent::info, wrapper, args.GetIsolate(), WrapperConfiguration::Dependent);
+    args.GetReturnValue().Set(wrapper);

[haraken, 2013/06/22 13:16:46]

Nit: You can use v8SetRuturnValue().

[jww, 2013/06/25 01:50:29]

Done.

+}
+
 void V8CustomEvent::detailAttrGetterCustom(v8::Local<v8::String> name, const v8::PropertyCallbackInfo<v8::Value>& info)
 {
     CustomEvent* imp = V8CustomEvent::toNative(info.Holder());
@@ -58,7 +84,19 @@ void V8CustomEvent::detailAttrGetterCustom(v8::Local<v8::String> name, const v8:
         v8SetReturnValue(info, value);
         return;
     }
-    v8SetReturnValue(info, imp->detail().v8Value());
+    v8SetReturnValue(info, info.Holder()->GetHiddenValue(V8HiddenPropertyName::detail()));
+}
+
+void V8CustomEvent::initCustomEventMethodCustom(const v8::FunctionCallbackInfo<v8::Value>& args)

[haraken, 2013/06/22 13:16:46]

Ditto. I hope you can improve the code generator and auto-generate the code.

[jww, 2013/06/25 01:50:29]

adamk@ and I discussed this, and while we agree that the constructor should be
auto generated (and that should be done now), we don't believe this method
should be auto generated. Given that MessageEvent would still need to have a
custom method, and PopState doesn't event have an init, an auto generated method
would end up just being the implementation of this method directly, which, we
think, would just make it more complicated.

[haraken, 2013/06/25 02:06:58]

Hmm, I'm neutral about this. I agree that the generated code will be used only
here as of today, but it will be used in other events in the future. In that
sense, I might want to improve the code generator at this point:)

[adamk, 2013/06/25 19:10:04]

My argument is that we'd have to special case event init methods, and I don't
think there will be any new event init methods in the future (since modern
events just use constructors). So unless we can un-customize initMessageEvent(),
this would really be the only place it's ever used.

[haraken, 2013/06/25 23:55:30]

Good point. It makes sense to keep the code in custom bindings. Thanks for the
clarification.

+{
+    CustomEvent* event = V8CustomEvent::toNative(args.Holder());

[adamk, 2013/06/22 01:00:04]

Can you add an ASSERT() that this event doesn't have a serializedValue()?

[jww, 2013/06/25 01:50:29]

Done.

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

[haraken, 2013/06/22 13:16:46]

SetHiddenValue affects the lifetime of 'detail'. Is there any existing test
about the lifetime?

[jww, 2013/06/25 01:50:29]

I think the answer is 'no,' but honestly, I'm not sure what you mean. Could you
explain what type of test about the lifetime you would expect?

[haraken, 2013/06/25 02:06:58]

I expect a test that fails if we miss SetHiddenValue() but passes if we add
SetHiddenValue(). The test will look like:

  var detail = "foo";
  var e = document.createEvent("CustomEvent");
  e.initCustomEvent(..., detail);
  detail = null;
  gc();
  shouldBeEqualToString('e.detail', "foo");

I'd hope you add similar tests for PopStateEvent and MessageEvent as well. I'm
sorry I'm asking you too much.

[adamk, 2013/06/25 19:10:04]

The trouble with the old code was that it was leaky, not that it was allowing
the detail to drop off too soon, so I don't think there's such a test (unless
there's some way to test for leaks).

[haraken, 2013/06/25 23:55:30]

Regarding GC issues, ideally we should have the following tests:

[1] A test that verifies that an object is not garbage-collected prematurely.
[2] A test that verifies that an object does not leak.
[3] A test that verifies that an object is not exposed to unexpected isolated
worlds.

Regarding [1], we already have a lot of tests for various DOM objects. So I hope
you can add tests for [1] for {Message,PopState,Custom}Event in this CL.

Regarding [2], we already have several tests, although the test coverage is not
good. One way to test a leak is to compare window.internals.numberOfLiveNodes
between before and after gc().

Regarding [3], you might be the first one to add a test. Thank you:)

+    event->initCustomEvent(typeArg, canBubbleArg, cancelableArg);

[adamk, 2013/06/22 01:00:04]

Can you just call initEvent() here and get rid of the initCustomEvent overload?

[jww, 2013/06/25 01:50:29]

Done.

 }
 
 } // namespace WebCore