Issue 17063016: Remove leak of objects between isolated worlds on custom events.

Issue 17063016: Remove leak of objects between isolated worlds on custom events. (Closed)

Created:
7 years, 6 months ago by jww

Modified:
7 years, 5 months ago

Reviewers:
haraken, adamk

CC:
blink-reviews, Nils Barth (inactive), jsbell+bindings_chromium.org, eae+blinkwatch, abarth-chromium, marja+watch_chromium.org, dglazkov+blink, adamk+blink_chromium.org, Nate Chapin, do-not-use

Base URL:
https://chromium.googlesource.com/chromium/blink.git@master

Visibility:
Public.

More Reviews

Description

Remove leak of objects between isolated worlds on custom events, message events, and pop state events. This stops both a security leak between isolated worlds as well as a memory leak. This includes updates to the code generator as well to generalize this fix to the three different event types. BUG=85158 Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=153701

Patch Set 1 #

Total comments: 30

Patch Set 2 : Large set of fixes from adamk and haraken. #

Total comments: 11

Patch Set 3 : Rebase on LKGR #

Patch Set 4 : Fixed some test failures, fixed some nits, and generalized the new events test. #

Total comments: 7

Patch Set 5 : Minor test improvements #

Patch Set 6 : Added GC test #

Total comments: 16

Patch Set 7 : Removed unnecessary gc() definition from test. #

Patch Set 8 : Reverted to version that expects GetHiddenValue to not return an empty Handle for undefined #

Total comments: 28

Patch Set 9 : Fixed several nits #

Patch Set 10 : Rebase updates on top of V8's GetHiddenValue fix. #

Patch Set 11 : Minor rebase nits. #

Created: 7 years, 5 months ago

Download [raw] [tar.bz2]

	Unified diffs	Side-by-side diffs	Delta from patch set	Stats (+257 lines, -85 lines)			Patch
M	LayoutTests/fast/events/constructors/custom-event-constructor.html	View	1 2 3	1 chunk	+6 lines, -0 lines	0 comments	Download
M	LayoutTests/fast/events/constructors/custom-event-constructor-expected.txt	View	1 2 3 4 5 6 7	1 chunk	+4 lines, -2 lines	0 comments	Download
A	LayoutTests/fast/events/event-isolated-world.html	View	1 2 3 4 5 6 7 8 9	1 chunk	+71 lines, -0 lines	0 comments	Download
A	LayoutTests/fast/events/event-isolated-world-expected.txt	View	1 2 3 4 5 6 7 8 9	1 chunk	+15 lines, -0 lines	0 comments	Download
A	LayoutTests/fast/events/event-properties-gc.html	View	1 2 3 4 5 6	1 chunk	+64 lines, -0 lines	0 comments	Download
A	LayoutTests/fast/events/event-properties-gc-expected.txt	View	1 2 3 4 5	1 chunk	+12 lines, -0 lines	0 comments	Download
M	Source/bindings/scripts/CodeGeneratorV8.pm	View	1 2 3 4 5 6 7 8 9 10	2 chunks	+19 lines, -2 lines	0 comments	Download
M	Source/bindings/tests/results/V8TestEventConstructor.cpp	View	1 2 3 4 5 6 7 8 9 10	1 chunk	+1 line, -1 line	0 comments	Download
M	Source/bindings/v8/Dictionary.h	View	1 2 3 4 5 6 7 8 9	1 chunk	+1 line, -0 lines	0 comments	Download
M	Source/bindings/v8/Dictionary.cpp	View	1 2 3 4 5 6 7 8 9	1 chunk	+5 lines, -0 lines	0 comments	Download
M	Source/bindings/v8/V8HiddenPropertyName.h	View	1 2 3 4 5 6 7 8 9 10	1 chunk	+1 line, -0 lines	0 comments	Download
M	Source/bindings/v8/custom/V8CustomEventCustom.cpp	View	1 2 3 4 5 6 7 8 9	1 chunk	+35 lines, -9 lines	0 comments	Download
M	Source/bindings/v8/custom/V8MessageEventCustom.cpp	View	1 2 3 4 5 6 7 8 9 10	4 chunks	+11 lines, -11 lines	0 comments	Download
M	Source/bindings/v8/custom/V8PopStateEventCustom.cpp	View	1 2 3 4 5 6 7 8 9	1 chunk	+0 lines, -5 lines	0 comments	Download
M	Source/core/dom/CustomEvent.h	View	1 4 5 6 7	2 chunks	+1 line, -8 lines	0 comments	Download
M	Source/core/dom/CustomEvent.cpp	View	1 4 5 6 7	3 chunks	+0 lines, -17 lines	0 comments	Download
M	Source/core/dom/CustomEvent.idl	View	1	1 chunk	+4 lines, -4 lines	0 comments	Download
M	Source/core/dom/MessageEvent.h	View	1 4 5 6 7	7 chunks	+4 lines, -8 lines	0 comments	Download
M	Source/core/dom/MessageEvent.cpp	View	1 4 5 6 7	4 chunks	+2 lines, -5 lines	0 comments	Download
M	Source/core/dom/PopStateEvent.h	View	1 2 3 4 5 6 7	3 chunks	+1 line, -8 lines	0 comments	Download
M	Source/core/dom/PopStateEvent.cpp	View	1 2 3 4 5 6 7	2 chunks	+0 lines, -5 lines	0 comments	Download

Messages

Total messages: 31 (0 generated)

Expand Messages | Collapse Messages

adamk

Looking good. https://codereview.chromium.org/17063016/diff/1/Source/bindings/v8/custom/V8CustomEventCustom.cpp File Source/bindings/v8/custom/V8CustomEventCustom.cpp (right): https://codereview.chromium.org/17063016/diff/1/Source/bindings/v8/custom/V8CustomEventCustom.cpp#newcode59 Source/bindings/v8/custom/V8CustomEventCustom.cpp:59: Dictionary options = Dictionary(args[1], args.GetIsolate()); This syntax ...

7 years, 6 months ago (2013-06-22 01:00:04 UTC) #2

haraken

https://codereview.chromium.org/17063016/diff/1/LayoutTests/http/tests/security/isolatedWorld/custom-event.html File LayoutTests/http/tests/security/isolatedWorld/custom-event.html (right): https://codereview.chromium.org/17063016/diff/1/LayoutTests/http/tests/security/isolatedWorld/custom-event.html#newcode2 LayoutTests/http/tests/security/isolatedWorld/custom-event.html:2: <html> You can write the test more simply by ...

7 years, 6 months ago (2013-06-22 13:16:45 UTC) #3

adamk

Another point about the test: it needn't live in http/tests/ (there's no need for it ...

7 years, 6 months ago (2013-06-24 18:07:32 UTC) #4

jww

Here are the fixes that I think address all of your issues. Note that I ...

7 years, 6 months ago (2013-06-25 01:50:28 UTC) #5

Here are the fixes that I think address all of your issues. Note that I haven't
added tests yet for the MessageEvent or PopState code; I will do that soon, but
I thought I'd make sure the changes fit your vision first.

https://codereview.chromium.org/17063016/diff/1/LayoutTests/http/tests/securi...
File LayoutTests/http/tests/security/isolatedWorld/custom-event.html (right):

https://codereview.chromium.org/17063016/diff/1/LayoutTests/http/tests/securi...
LayoutTests/http/tests/security/isolatedWorld/custom-event.html:2: <html>
On 2013/06/22 13:16:46, haraken wrote:
> 
> You can write the test more simply by including js-test-pre.js. Please use
> shouldBe() instead of hand-written innerHTMLs.

Given that I'm moving the test from http/tests to fast/events, I don't believe I
am able to use js-test-pre.js anymore, so I am now replicating shouldBe() as is
done in the other fast/events tests.

https://codereview.chromium.org/17063016/diff/1/LayoutTests/http/tests/securi...
LayoutTests/http/tests/security/isolatedWorld/custom-event.html:7: if
(window.testRunner && window.eventSender) {
On 2013/06/22 13:16:46, haraken wrote:
> 
> Nit: window.eventSender is not needed.

Done.

https://codereview.chromium.org/17063016/diff/1/Source/bindings/v8/Dictionary.h
File Source/bindings/v8/Dictionary.h (right):

https://codereview.chromium.org/17063016/diff/1/Source/bindings/v8/Dictionary...
Source/bindings/v8/Dictionary.h:106: private:
On 2013/06/22 13:16:46, haraken wrote:
> 
> Instead of making getKey() public, would you define get(const String&,
> v8::Local<v8::Value>&) just like other get() methods?

Done.

https://codereview.chromium.org/17063016/diff/1/Source/bindings/v8/custom/V8C...
File Source/bindings/v8/custom/V8CustomEventCustom.cpp (right):

https://codereview.chromium.org/17063016/diff/1/Source/bindings/v8/custom/V8C...
Source/bindings/v8/custom/V8CustomEventCustom.cpp:48: void
V8CustomEvent::constructorCustom(const v8::FunctionCallbackInfo<v8::Value>&
args)
On 2013/06/22 13:16:46, haraken wrote:
> 
> 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'.
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.

https://codereview.chromium.org/17063016/diff/1/Source/bindings/v8/custom/V8C...
Source/bindings/v8/custom/V8CustomEventCustom.cpp:59: Dictionary options =
Dictionary(args[1], args.GetIsolate());
On 2013/06/22 01:00:04, adamk wrote:
> This syntax looks odd to me, can you get rid of the extra "Dictionary" and
just
> say options(...) instead?

Done.

https://codereview.chromium.org/17063016/diff/1/Source/bindings/v8/custom/V8C...
Source/bindings/v8/custom/V8CustomEventCustom.cpp:64:
args.Holder()->SetHiddenValue(V8HiddenPropertyName::detail(), detail);
On 2013/06/22 01:00:04, adamk wrote:
> Guard this with an if() check on the above? Or at least that detail is
> non-empty?
I wrapped the "SetHiddenValue" with an "if" check for "!detail.IsEmpty()". Is
that what you meant?

https://codereview.chromium.org/17063016/diff/1/Source/bindings/v8/custom/V8C...
Source/bindings/v8/custom/V8CustomEventCustom.cpp:71:
args.GetReturnValue().Set(wrapper);
On 2013/06/22 13:16:46, haraken wrote:
> 
> Nit: You can use v8SetRuturnValue().

Done.

https://codereview.chromium.org/17063016/diff/1/Source/bindings/v8/custom/V8C...
Source/bindings/v8/custom/V8CustomEventCustom.cpp:90: void
V8CustomEvent::initCustomEventMethodCustom(const
v8::FunctionCallbackInfo<v8::Value>& args)
On 2013/06/22 13:16:46, haraken wrote:
> 
> Ditto. I hope you can improve the code generator and auto-generate the code.
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.

https://codereview.chromium.org/17063016/diff/1/Source/bindings/v8/custom/V8C...
Source/bindings/v8/custom/V8CustomEventCustom.cpp:92: CustomEvent* event =
V8CustomEvent::toNative(args.Holder());
On 2013/06/22 01:00:04, adamk wrote:
> Can you add an ASSERT() that this event doesn't have a serializedValue()?

Done.

https://codereview.chromium.org/17063016/diff/1/Source/bindings/v8/custom/V8C...
Source/bindings/v8/custom/V8CustomEventCustom.cpp:98:
args.Holder()->SetHiddenValue(V8HiddenPropertyName::detail(), detailsArg);
On 2013/06/22 13:16:46, haraken wrote:
> 
> SetHiddenValue affects the lifetime of 'detail'. Is there any existing test
> about the lifetime?
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?

https://codereview.chromium.org/17063016/diff/1/Source/bindings/v8/custom/V8C...
Source/bindings/v8/custom/V8CustomEventCustom.cpp:99:
event->initCustomEvent(typeArg, canBubbleArg, cancelableArg);
On 2013/06/22 01:00:04, adamk wrote:
> Can you just call initEvent() here and get rid of the initCustomEvent
overload?

Done.

https://codereview.chromium.org/17063016/diff/1/Source/core/dom/CustomEvent.h
File Source/core/dom/CustomEvent.h (right):

https://codereview.chromium.org/17063016/diff/1/Source/core/dom/CustomEvent.h...
Source/core/dom/CustomEvent.h:35: struct CustomEventInit : public EventInit {
On 2013/06/22 01:00:04, adamk wrote:
> This can probably just be
> 
> typedef EventInit CustomEventInit;

Done.

haraken

This is a great patch. https://codereview.chromium.org/17063016/diff/1/Source/bindings/v8/custom/V8CustomEventCustom.cpp File Source/bindings/v8/custom/V8CustomEventCustom.cpp (right): https://codereview.chromium.org/17063016/diff/1/Source/bindings/v8/custom/V8CustomEventCustom.cpp#newcode90 Source/bindings/v8/custom/V8CustomEventCustom.cpp:90: void V8CustomEvent::initCustomEventMethodCustom(const v8::FunctionCallbackInfo<v8::Value>& args) ...

7 years, 6 months ago (2013-06-25 02:06:58 UTC) #6

jww

On 2013/06/25 02:06:58, haraken wrote: > This is a great patch. > > https://codereview.chromium.org/17063016/diff/1/Source/bindings/v8/custom/V8CustomEventCustom.cpp > ...

7 years, 6 months ago (2013-06-25 02:57:10 UTC) #7

adamk

https://codereview.chromium.org/17063016/diff/1/Source/bindings/v8/custom/V8CustomEventCustom.cpp File Source/bindings/v8/custom/V8CustomEventCustom.cpp (right): https://codereview.chromium.org/17063016/diff/1/Source/bindings/v8/custom/V8CustomEventCustom.cpp#newcode90 Source/bindings/v8/custom/V8CustomEventCustom.cpp:90: void V8CustomEvent::initCustomEventMethodCustom(const v8::FunctionCallbackInfo<v8::Value>& args) On 2013/06/25 02:06:58, haraken wrote: ...

7 years, 6 months ago (2013-06-25 19:10:04 UTC) #8

haraken

7 years, 6 months ago (2013-06-25 23:55:29 UTC) #9

jww

haraken@, I'll take a look at your test suggestions in the morning. For now, it ...

7 years, 6 months ago (2013-06-26 02:27:06 UTC) #10

haraken

The change looks good. Would you add GC tests as I mentioned above? You can ...

7 years, 6 months ago (2013-06-26 04:02:31 UTC) #11

haraken

https://codereview.chromium.org/17063016/diff/21001/Source/bindings/v8/custom/V8CustomEventCustom.cpp File Source/bindings/v8/custom/V8CustomEventCustom.cpp (right): https://codereview.chromium.org/17063016/diff/21001/Source/bindings/v8/custom/V8CustomEventCustom.cpp#newcode77 Source/bindings/v8/custom/V8CustomEventCustom.cpp:77: // V8MessageEventCustom.cpp and V8PopStateEventCustom.cpp as well. I don't think ...

7 years, 6 months ago (2013-06-26 04:40:20 UTC) #12

jww

On 2013/06/26 04:40:20, haraken wrote: > https://codereview.chromium.org/17063016/diff/21001/Source/bindings/v8/custom/V8CustomEventCustom.cpp > File Source/bindings/v8/custom/V8CustomEventCustom.cpp (right): > > https://codereview.chromium.org/17063016/diff/21001/Source/bindings/v8/custom/V8CustomEventCustom.cpp#newcode77 > ...

7 years, 6 months ago (2013-06-26 17:17:41 UTC) #13

On 2013/06/26 04:40:20, haraken wrote:
>
https://codereview.chromium.org/17063016/diff/21001/Source/bindings/v8/custom...
> File Source/bindings/v8/custom/V8CustomEventCustom.cpp (right):
> 
>
https://codereview.chromium.org/17063016/diff/21001/Source/bindings/v8/custom...
> Source/bindings/v8/custom/V8CustomEventCustom.cpp:77: //
> V8MessageEventCustom.cpp and V8PopStateEventCustom.cpp as well.
> 
> 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?

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.

jww

I'll work on your GC test suggestion and get back to you soon with it. ...

7 years, 6 months ago (2013-06-26 17:19:08 UTC) #14

I'll work on your GC test suggestion and get back to you soon with it.

https://codereview.chromium.org/17063016/diff/21001/LayoutTests/fast/events/c...
File LayoutTests/fast/events/constructors/custom-event-constructor-expected.txt
(right):

https://codereview.chromium.org/17063016/diff/21001/LayoutTests/fast/events/c...
LayoutTests/fast/events/constructors/custom-event-constructor-expected.txt:17:
PASS new CustomEvent('eventType', { detail: null }).detail is null
On 2013/06/26 04:02:31, haraken wrote:
> 
> Would you add similar test cases for PopStateEvent and MessageEvent? Maybe do
> they already have the test cases?
They already have similar test cases; I was just adding in the equivalent for
CustomEvent.

https://codereview.chromium.org/17063016/diff/21001/LayoutTests/fast/events/e...
File LayoutTests/fast/events/event-isolated-world.html (right):

https://codereview.chromium.org/17063016/diff/21001/LayoutTests/fast/events/e...
LayoutTests/fast/events/event-isolated-world.html:47:
document.body.appendChild(script);
On 2013/06/26 04:02:31, haraken wrote:
> 
> I don't think this is needed. I guess that you can just statically write
<script
> src="../js/resources/js-test-post.js"></script> at the end of this html and
call
> testRunner.notifyDone() here.

Done.

https://codereview.chromium.org/17063016/diff/21001/Source/bindings/v8/custom...
File Source/bindings/v8/custom/V8CustomEventCustom.cpp (right):

https://codereview.chromium.org/17063016/diff/21001/Source/bindings/v8/custom...
Source/bindings/v8/custom/V8CustomEventCustom.cpp:77: //
V8MessageEventCustom.cpp and V8PopStateEventCustom.cpp as well.
On 2013/06/26 04:40:20, haraken wrote:
> 
> 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?

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

7 years, 6 months ago (2013-06-26 17:43:37 UTC) #15

https://codereview.chromium.org/17063016/diff/21001/Source/bindings/v8/custom...
File Source/bindings/v8/custom/V8CustomEventCustom.cpp (right):

https://codereview.chromium.org/17063016/diff/21001/Source/bindings/v8/custom...
Source/bindings/v8/custom/V8CustomEventCustom.cpp:77: //
V8MessageEventCustom.cpp and V8PopStateEventCustom.cpp as well.
On 2013/06/26 17:19:08, jww wrote:
> On 2013/06/26 04:40:20, haraken wrote:
> > 
> > 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?
> 
> 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.

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

jww

I've added in a GC test, as per haraken's request. I, too, am all for ...

7 years, 6 months ago (2013-06-26 21:41:02 UTC) #16

haraken

Thanks for the tests! Looks good. Regarding the GetHiddenValue(), I'm sorry I was misunderstanding. The ...

7 years, 6 months ago (2013-06-26 23:53:04 UTC) #17

adamk

Please update the CL description to more accurately capture your change, which is now broader ...

7 years, 6 months ago (2013-06-27 00:15:04 UTC) #18

jww

https://codereview.chromium.org/17063016/diff/38001/LayoutTests/fast/events/event-properties-gc.html File LayoutTests/fast/events/event-properties-gc.html (right): https://codereview.chromium.org/17063016/diff/38001/LayoutTests/fast/events/event-properties-gc.html#newcode11 LayoutTests/fast/events/event-properties-gc.html:11: function gc() { On 2013/06/26 23:53:04, haraken wrote: > ...

7 years, 6 months ago (2013-06-27 00:16:18 UTC) #19

haraken

On 2013/06/27 00:16:18, jww wrote: > https://codereview.chromium.org/17063016/diff/38001/LayoutTests/fast/events/event-properties-gc.html > File LayoutTests/fast/events/event-properties-gc.html (right): > > https://codereview.chromium.org/17063016/diff/38001/LayoutTests/fast/events/event-properties-gc.html#newcode11 > ...

7 years, 6 months ago (2013-06-27 00:20:18 UTC) #20

adamk

On 2013/06/27 00:20:18, haraken wrote: > On 2013/06/27 00:16:18, jww wrote: > > > https://codereview.chromium.org/17063016/diff/38001/LayoutTests/fast/events/event-properties-gc.html ...

7 years, 6 months ago (2013-06-27 00:24:30 UTC) #21

jww

I've reverted the files to a version where they expect GetHiddenValue to return v8::Undefined() not ...

7 years, 6 months ago (2013-06-27 04:35:35 UTC) #22

haraken

LGTM from my side. Please wait for adamk's comment before landing :) https://codereview.chromium.org/17063016/diff/54001/LayoutTests/fast/events/event-isolated-world.html File LayoutTests/fast/events/event-isolated-world.html ...

7 years, 6 months ago (2013-06-27 04:50:27 UTC) #23

adamk

https://codereview.chromium.org/17063016/diff/54001/Source/core/dom/CustomEvent.h File Source/core/dom/CustomEvent.h (right): https://codereview.chromium.org/17063016/diff/54001/Source/core/dom/CustomEvent.h#newcode35 Source/core/dom/CustomEvent.h:35: typedef EventInit CustomEventInit; On 2013/06/27 04:50:27, haraken wrote: > ...

7 years, 5 months ago (2013-06-27 17:11:45 UTC) #24

jww

https://codereview.chromium.org/17063016/diff/54001/LayoutTests/fast/events/event-isolated-world.html File LayoutTests/fast/events/event-isolated-world.html (right): https://codereview.chromium.org/17063016/diff/54001/LayoutTests/fast/events/event-isolated-world.html#newcode16 LayoutTests/fast/events/event-isolated-world.html:16: shouldBe("documentObject", "undefined"); On 2013/06/27 04:50:27, haraken wrote: > > ...

7 years, 5 months ago (2013-06-27 17:44:42 UTC) #25

haraken

LGTM https://codereview.chromium.org/17063016/diff/54001/Source/bindings/v8/custom/V8CustomEventCustom.cpp File Source/bindings/v8/custom/V8CustomEventCustom.cpp (right): https://codereview.chromium.org/17063016/diff/54001/Source/bindings/v8/custom/V8CustomEventCustom.cpp#newcode55 Source/bindings/v8/custom/V8CustomEventCustom.cpp:55: void V8CustomEvent::detailAttrGetterCustom(v8::Local<v8::String> name, const v8::PropertyCallbackInfo<v8::Value>& info) I'd guess ...

7 years, 5 months ago (2013-06-28 00:08:00 UTC) #26

commit-bot: I haz the power

CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/jww@chromium.org/17063016/67001

7 years, 5 months ago (2013-07-08 18:55:08 UTC) #28

commit-bot: I haz the power

Failed to apply patch for Source/bindings/v8/V8HiddenPropertyName.h: While running patch -p1 --forward --force --no-backup-if-mismatch; patching file ...

7 years, 5 months ago (2013-07-08 18:55:24 UTC) #29

commit-bot: I haz the power

CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/jww@chromium.org/17063016/75001

7 years, 5 months ago (2013-07-08 19:06:49 UTC) #30

Message was sent while issue was closed.

Change committed as 153701

Expand Messages | Collapse Messages