Crashed at ImageEventListener::cast & ConditionEventListener::cast

Crashed at ImageEventListener::cast & ConditionEventListener::cast.
Add null checking in EventTarget::removeEventListener,
because we shouldn't be passing in a null listener reference to the ==()operator.

BUG=377425
R=sigbjornf@opera.com

Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=181514

[sof, 2014-09-03 10:49:59]

Do you think you can put together tests for the problematic condition?

(Then we can better understand why there are empty references in the
EventListenerMap vector.)

[Miyoung Shin(g), 2014-09-03 12:46:46]

On 2014/09/03 10:49:59, sof wrote:
> Do you think you can put together tests for the problematic condition?
> 
> (Then we can better understand why there are empty references in the
> EventListenerMap vector.)

Because ImageDocument is created when loading directly image url like
http://xxxxx.png, I have no idea.
Moreover ImageDocument's listener which has the problem is added only in case of
the main frame,
and I used the console window in inspector to add javascipt code.
I don't know how to inject javascript code for window.addEventListener and
window.removeEventListener in ImageDocument.

[Miyoung Shin(g), 2014-09-03 12:59:17]

When calling removeEventListener several times by JS,
from the second time, we get null from EventListenerMap inside V8 binding code
for removeEventListener.
Other listener class is OK because operation== is very simple like "this ==
listener".
But both of imagedocument listener and svg listener try to access the null point
for type casting and the crash occurs.

[kouhei (in TOK), 2014-09-03 13:41:20]

On 2014/09/03 12:46:46, Miyoung Shin(g) wrote:
> On 2014/09/03 10:49:59, sof wrote:
> > Do you think you can put together tests for the problematic condition?
> > 
> > (Then we can better understand why there are empty references in the
> > EventListenerMap vector.)
> 
> Because ImageDocument is created when loading directly image url like
> http://xxxxx.png, I have no idea.
> Moreover ImageDocument's listener which has the problem is added only in case
of
> the main frame,
> and I used the console window in inspector to add javascipt code.
> I don't know how to inject javascript code for window.addEventListener and
> window.removeEventListener in ImageDocument.

Would it be possible if you load image inside an iframe?

[Miyoung Shin(g), 2014-09-03 14:04:42]

On 2014/09/03 13:41:20, kouhei wrote:
> On 2014/09/03 12:46:46, Miyoung Shin(g) wrote:
> > On 2014/09/03 10:49:59, sof wrote:
> > > Do you think you can put together tests for the problematic condition?
> > > 
> > > (Then we can better understand why there are empty references in the
> > > EventListenerMap vector.)
> > 
> > Because ImageDocument is created when loading directly image url like
> > http://xxxxx.png, I have no idea.
> > Moreover ImageDocument's listener which has the problem is added only in
case
> of
> > the main frame,
> > and I used the console window in inspector to add javascipt code.
> > I don't know how to inject javascript code for window.addEventListener and
> > window.removeEventListener in ImageDocument.
> 
> Would it be possible if you load image inside an iframe?

I'd already tried to load image inside an iframe.
But It can't make the crash because listener of imagedocument is added only in
case of main frame.

[Miyoung Shin(g), 2014-09-05 14:56:06]

This is backtrace.

Received signal 11 SEGV_MAPERR 000000000018
#0 0x7f734e920f8e [7383:7383:0905/235427:INFO:CONSOLE(3)] "blur, before remove",
source:  (3)
base::debug::StackTrace::StackTrace()
#1 0x7f734e920ac0 base::debug::(anonymous namespace)::StackDumpSignalHandler()
#2 0x7f73470e9340 <unknown>
#3 0x7f73510b680c blink::EventListener::type()
#4 0x7f73510b5bc5 blink::ImageEventListener::cast()
#5 0x7f73510b55e1 blink::ImageEventListener::operator==()
#6 0x7f7350f1608e blink::operator==()
#7 0x7f7350f15fa4 WTF::Vector<>::find<>()
#8 0x7f7350f13986 blink::removeListenerFromVector()
#9 0x7f7350f138a9 blink::EventListenerMap::remove()
#10 0x7f7350f22787 blink::EventTarget::removeEventListener()
#11 0x7f735151d286 blink::LocalDOMWindow::removeEventListener()
#12 0x7f73521cfe59 blink::EventTargetV8Internal::removeEventListenerMethod()
#13 0x7f73521ce834
blink::EventTargetV8Internal::removeEventListenerMethodCallback()
#14 0x7f7350bfff80 v8::internal::FunctionCallbackArguments::Call()
#15 0x7f735076f0cd v8::internal::Builtin_HandleApiCall()
#16 0x2959f260754e <unknown>

[sof, 2014-09-05 16:27:29]

On 2014/09/05 14:56:06, Miyoung Shin(g) wrote:
> This is backtrace.
> 
> Received signal 11 SEGV_MAPERR 000000000018
> #0 0x7f734e920f8e [7383:7383:0905/235427:INFO:CONSOLE(3)] "blur, before
remove",
> source:  (3)
> base::debug::StackTrace::StackTrace()
> #1 0x7f734e920ac0 base::debug::(anonymous namespace)::StackDumpSignalHandler()
> #2 0x7f73470e9340 <unknown>
> #3 0x7f73510b680c blink::EventListener::type()
> #4 0x7f73510b5bc5 blink::ImageEventListener::cast()
> #5 0x7f73510b55e1 blink::ImageEventListener::operator==()
> #6 0x7f7350f1608e blink::operator==()
> #7 0x7f7350f15fa4 WTF::Vector<>::find<>()
> #8 0x7f7350f13986 blink::removeListenerFromVector()
> #9 0x7f7350f138a9 blink::EventListenerMap::remove()
> #10 0x7f7350f22787 blink::EventTarget::removeEventListener()
> #11 0x7f735151d286 blink::LocalDOMWindow::removeEventListener()
> #12 0x7f73521cfe59 blink::EventTargetV8Internal::removeEventListenerMethod()
> #13 0x7f73521ce834
> blink::EventTargetV8Internal::removeEventListenerMethodCallback()
> #14 0x7f7350bfff80 v8::internal::FunctionCallbackArguments::Call()
> #15 0x7f735076f0cd v8::internal::Builtin_HandleApiCall()
> #16 0x2959f260754e <unknown>

My suggestion is that you try to determine under what conditions a null listener
is being passed to LocalDOMWindow::removeEventListener().

The problem needs to be solved further out than where this CL inserts a null
check -- we shouldn't be passing in a null listener reference to the ==()
operator here; that's just broken.

[sof, 2014-09-06 06:26:50]

https://codereview.chromium.org/551473002/ has a testcase.

As to addressing the problem, have a look at http://crbug.com/249598's changes
to EventTarget::addEventListener() -- want to do a variation of that for
removeEventListener() also, I think.

[Miyoung Shin(g), 2014-09-06 11:01:30]

On 2014/09/06 06:26:50, sof wrote:
> https://codereview.chromium.org/551473002/ has a testcase.
> 
> As to addressing the problem, have a look at http://crbug.com/249598 changes
> to EventTarget::addEventListener() -- want to do a variation of that for
> removeEventListener() also, I think.

Thank you for your kind. 
I think also that this issue treats the same like http://crbug.com/249598
changes.
And your test case really helps me, I never think before. I'm keep it in my mind
for the future.
After changing, I tested your test cast and it's OK. (before changing, crash)

BTW, how to reflect your patch, do you think it should combine with my patch or
apply each patches?

[sof, 2014-09-06 13:22:42]

On 2014/09/06 11:01:30, Miyoung Shin(g) wrote:
> On 2014/09/06 06:26:50, sof wrote:
> > https://codereview.chromium.org/551473002/ has a testcase.
> > 
> > As to addressing the problem, have a look at http://crbug.com/249598 changes
> > to EventTarget::addEventListener() -- want to do a variation of that for
> > removeEventListener() also, I think.
> 
> Thank you for your kind. 
> I think also that this issue treats the same like http://crbug.com/249598
> changes.
> And your test case really helps me, I never think before. I'm keep it in my
mind
> for the future.
> After changing, I tested your test cast and it's OK. (before changing, crash)
> 
> BTW, how to reflect your patch, do you think it should combine with my patch
or
> apply each patches?

I would just include the test in this CL.

Could you also add a pair of asserts to the operator ==() for
RegisteredEventListener, so that we catch any future attempts to create a value
reference from null listener? i.e., add

  ASSERT(a.listener);
  ASSERT(b.listener);

before the 'return' ?

[Miyoung Shin(g), 2014-09-07 01:50:44]

On 2014/09/06 13:22:42, sof wrote:
> On 2014/09/06 11:01:30, Miyoung Shin(g) wrote:
> > On 2014/09/06 06:26:50, sof wrote:
> > > https://codereview.chromium.org/551473002/ has a testcase.
> > > 
> > > As to addressing the problem, have a look at http://crbug.com/249598
changes
> > > to EventTarget::addEventListener() -- want to do a variation of that for
> > > removeEventListener() also, I think.
> > 
> > Thank you for your kind. 
> > I think also that this issue treats the same like http://crbug.com/249598
> > changes.
> > And your test case really helps me, I never think before. I'm keep it in my
> mind
> > for the future.
> > After changing, I tested your test cast and it's OK. (before changing,
crash)
> > 
> > BTW, how to reflect your patch, do you think it should combine with my patch
> or
> > apply each patches?
> 
> I would just include the test in this CL.
> 
> Could you also add a pair of asserts to the operator ==() for
> RegisteredEventListener, so that we catch any future attempts to create a
value
> reference from null listener? i.e., add
> 
>   ASSERT(a.listener);
>   ASSERT(b.listener);

> before the 'return' ?

I've re-uploaded the patch to apply your comment and there isn't any crash
against adding ASSERT when testing LayoutTests/fast/* on Debug.

[sof, 2014-09-07 04:49:42]

lgtm, thanks for iterating on this one.

(no need to wait on approval for the test tidyup, just land with the nit
addressed.)

https://codereview.chromium.org/539453002/diff/60001/LayoutTests/fast/images/...
File LayoutTests/fast/images/image-document-remove-listener.html (right):

https://codereview.chromium.org/539453002/diff/60001/LayoutTests/fast/images/...
LayoutTests/fast/images/image-document-remove-listener.html:16: function
log(message)
Unused in the end, so let's just remove it.

[Miyoung Shin(g), 2014-09-07 05:34:47]

The CQ bit was checked by myid.o.shin@gmail.com

[commit-bot: I haz the power, 2014-09-07 05:35:51]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/myid.o.shin@gmail.com/539453002/80001

[commit-bot: I haz the power, 2014-09-07 08:34:14]

Committed patchset #5 (id:80001) as 181514