Fix event warning crash when a listener is removed while dispatching event

Fix event warning crash when a listener is removed while dispatching event

Also, expose TestRunner.setDumpConsoleMessages() to tests to control whether
console messages are included into the test output.

BUG=612377
Committed: https://crrev.com/cb2227837d5d0e6a59e73eb62e56d53534a69386
Cr-Commit-Position: refs/heads/master@{#395653}

[caseq, 2016-05-18 23:13:36]

caseq@chromium.org changed reviewers:
+ dtapuska@chromium.org, pfeldman@chromium.org

[dtapuska, 2016-05-18 23:44:17]

https://codereview.chromium.org/1989103006/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/events/EventTarget.cpp (left):

https://codereview.chromium.org/1989103006/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/events/EventTarget.cpp:574:
reportBlockedEvent(context, event, &registeredListener, now -
event->platformTimeStamp());
Eek, can we just not hold a reference but a copy of the registered event
listener on the stack?

[caseq, 2016-05-18 23:48:14]

On 2016/05/18 23:44:17, dtapuska wrote:
>
https://codereview.chromium.org/1989103006/diff/1/third_party/WebKit/Source/c...
> File third_party/WebKit/Source/core/events/EventTarget.cpp (left):
> 
>
https://codereview.chromium.org/1989103006/diff/1/third_party/WebKit/Source/c...
> third_party/WebKit/Source/core/events/EventTarget.cpp:574:
> reportBlockedEvent(context, event, &registeredListener, now -
> event->platformTimeStamp());
> Eek, can we just not hold a reference but a copy of the registered event
> listener on the stack?

Well, we still need original to set the blockedEventWarningEmitted flag there,
but we've just had a similar discussion with Pavel and I'm going to adjust the
way we check for whether RegisteredEventListener has moved to compare actual
even listener.

[caseq, 2016-05-19 00:13:14]

ptal

[dtapuska, 2016-05-19 15:41:21]

https://codereview.chromium.org/1989103006/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/events/EventTarget.cpp (right):

https://codereview.chromium.org/1989103006/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/events/EventTarget.cpp:577: if
(shouldReportBlockedEvent && i > 0 && entry[i - 1].listener() == listener
This doesn't look too correct to me. What happens when this is the only
listener? Doesn't entry[i-1] refer to invalid memory then?

[caseq, 2016-05-19 16:04:40]

On 2016/05/19 15:41:21, dtapuska wrote:
>
https://codereview.chromium.org/1989103006/diff/20001/third_party/WebKit/Sour...
> File third_party/WebKit/Source/core/events/EventTarget.cpp (right):
> 
>
https://codereview.chromium.org/1989103006/diff/20001/third_party/WebKit/Sour...
> third_party/WebKit/Source/core/events/EventTarget.cpp:577: if
> (shouldReportBlockedEvent && i > 0 && entry[i - 1].listener() == listener
> This doesn't look too correct to me. What happens when this is the only
> listener? Doesn't entry[i-1] refer to invalid memory then?

We've got i > 0 clause first, don't we?

[dtapuska, 2016-05-19 16:06:31]

On 2016/05/19 16:04:40, caseq wrote:
> On 2016/05/19 15:41:21, dtapuska wrote:
> >
>
https://codereview.chromium.org/1989103006/diff/20001/third_party/WebKit/Sour...
> > File third_party/WebKit/Source/core/events/EventTarget.cpp (right):
> > 
> >
>
https://codereview.chromium.org/1989103006/diff/20001/third_party/WebKit/Sour...
> > third_party/WebKit/Source/core/events/EventTarget.cpp:577: if
> > (shouldReportBlockedEvent && i > 0 && entry[i - 1].listener() == listener
> > This doesn't look too correct to me. What happens when this is the only
> > listener? Doesn't entry[i-1] refer to invalid memory then?
> 
> We've got i > 0 clause first, don't we?

Isn't the variable incremented inside the while loop here:
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...

Note I did not write this code; I am just trying to understand it :-)

[caseq, 2016-05-19 16:15:26]

On 2016/05/19 16:06:31, dtapuska wrote:
> On 2016/05/19 16:04:40, caseq wrote:
> > On 2016/05/19 15:41:21, dtapuska wrote:
> > >
> >
>
https://codereview.chromium.org/1989103006/diff/20001/third_party/WebKit/Sour...
> > > File third_party/WebKit/Source/core/events/EventTarget.cpp (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/1989103006/diff/20001/third_party/WebKit/Sour...
> > > third_party/WebKit/Source/core/events/EventTarget.cpp:577: if
> > > (shouldReportBlockedEvent && i > 0 && entry[i - 1].listener() == listener
> > > This doesn't look too correct to me. What happens when this is the only
> > > listener? Doesn't entry[i-1] refer to invalid memory then?
> > 
> > We've got i > 0 clause first, don't we?
> 
> Isn't the variable incremented inside the while loop here:
>
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...
> 

It is, and it's why we have to subtract one to refer to the original listener in
case nothing changes. Now the catch is that i (and size) may change while event
is being dispatched by listener->handleEvent() with the help of
FiringEventIterator as the listener is being removed.
This may cause either i to become negative, or entry[i - 1] to point to a
different previously dispatched listener -- so we check for these two
conditions.

[dtapuska, 2016-05-20 14:59:46]

lgtm

[dtapuska, 2016-05-20 14:59:47]

lgtm

[pfeldman, 2016-05-23 16:57:03]

lgtm

[caseq, 2016-05-23 16:58:14]

The CQ bit was checked by caseq@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-05-23 16:58:33]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1989103006/20001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1989103006/20001

[commit-bot: I haz the power, 2016-05-23 17:00:45]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-05-23 17:00:46]

Dry run: Try jobs failed on following builders:
  ios-device-gn on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-gn/bui...)
  ios-simulator on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
  ios-simulator-gn on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-gn/...)

[caseq, 2016-05-23 18:28:53]

The CQ bit was checked by caseq@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-05-23 18:29:24]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1989103006/40001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1989103006/40001

[commit-bot: I haz the power, 2016-05-23 19:42:06]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-05-23 19:42:07]

Dry run: Try jobs failed on following builders:
  mac_chromium_gn_rel on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_gn_r...)

[caseq, 2016-05-23 22:26:59]

The CQ bit was checked by caseq@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-05-23 22:27:35]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1989103006/60001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1989103006/60001

[commit-bot: I haz the power, 2016-05-23 23:54:45]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-05-23 23:54:46]

Dry run: Try jobs failed on following builders:
  mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[caseq, 2016-05-24 00:12:16]

The CQ bit was checked by caseq@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-05-24 00:12:47]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1989103006/80001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1989103006/80001

[commit-bot: I haz the power, 2016-05-24 05:01:41]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-05-24 05:01:43]

Dry run: Try jobs failed on following builders:
  win_chromium_rel_ng on tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[caseq, 2016-05-24 17:07:45]

The CQ bit was checked by caseq@chromium.org

[caseq, 2016-05-24 17:07:45]

The patchset sent to the CQ was uploaded after l-g-t-m from
dtapuska@chromium.org, pfeldman@chromium.org
Link to the patchset: https://codereview.chromium.org/1989103006/#ps80001
(title: "fixed AtomicString usage")

[commit-bot: I haz the power, 2016-05-24 17:08:18]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1989103006/80001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1989103006/80001

[commit-bot: I haz the power, 2016-05-24 18:52:22]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2016-05-24 18:53:06]

Description was changed from

==========
Fix event warning crash when a listener is removed while dispatching event

Also, expose TestRunner.setDumpConsoleMessages() to tests to control whether
console messages are included into the test output.

BUG=612377
==========

to

==========
Fix event warning crash when a listener is removed while dispatching event

Also, expose TestRunner.setDumpConsoleMessages() to tests to control whether
console messages are included into the test output.

BUG=612377

Committed: https://crrev.com/cb2227837d5d0e6a59e73eb62e56d53534a69386
Cr-Commit-Position: refs/heads/master@{#395653}
==========

[commit-bot: I haz the power, 2016-05-24 18:53:07]

Patchset 5 (id:??) landed as
https://crrev.com/cb2227837d5d0e6a59e73eb62e56d53534a69386
Cr-Commit-Position: refs/heads/master@{#395653}