EventSender: Added checks for missing arguments

Added checks for missing arguments in event_sender.cc. Also simplified the uses of x & y in EventSender::GestureEvent.

BUG=415970
Committed: https://crrev.com/0fff7fc37d0fc782f10b9eef2d4047bd286d1ff7
Cr-Commit-Position: refs/heads/master@{#296505}

[mustaq, 2014-09-23 13:40:32]

mustaq@chromium.org changed reviewers:
+ dpranke@chromium.org

[Rick Byers, 2014-09-23 14:27:14]

rbyers@chromium.org changed reviewers:
+ rbyers@chromium.org

[Rick Byers, 2014-09-23 14:27:14]

https://codereview.chromium.org/584203003/diff/20001/content/shell/renderer/t...
File content/shell/renderer/test_runner/event_sender.cc (right):

https://codereview.chromium.org/584203003/diff/20001/content/shell/renderer/t...
content/shell/renderer/test_runner/event_sender.cc:1927:
DCHECK(args->GetNext(&x));
I think we should check to see if there are args and fail gracefully (with an
exception) if there isn't.  See the uses of args->PeekNext().IsEmpty()
throughout this file.  The main reason to do this even though this is only test
code is that we do fuzz testing that creates semi-random html/JavaScript pages
that may make use of EventSender (eg. see http://crbug.com/390623).  So any
crash you can trigger with EventSender is a potential clusterfuzz failure point
(which would ultimately be a false positive since it doesn't ship, but still
worth avoiding the noise).

[earthdok, 2014-09-23 14:33:08]

earthdok@chromium.org changed reviewers:
+ earthdok@chromium.org

[earthdok, 2014-09-23 14:33:08]

https://codereview.chromium.org/584203003/diff/20001/content/shell/renderer/t...
File content/shell/renderer/test_runner/event_sender.cc (right):

https://codereview.chromium.org/584203003/diff/20001/content/shell/renderer/t...
content/shell/renderer/test_runner/event_sender.cc:1927:
DCHECK(args->GetNext(&x));
DCHECKs aren't evaluated in Release builds, so this function call won't happen.
Worse, MSan can no longer catch this bug because you've pre-initialized the
variables (though in this particular case you'll get a compiler warning about
unused variables).

Why not change those to CHECKs, and remove explicit zero-initialization of x and
y?

[earthdok, 2014-09-23 14:34:05]

On 2014/09/23 14:33:08, earthdok wrote:
>
https://codereview.chromium.org/584203003/diff/20001/content/shell/renderer/t...
> File content/shell/renderer/test_runner/event_sender.cc (right):
> 
>
https://codereview.chromium.org/584203003/diff/20001/content/shell/renderer/t...
> content/shell/renderer/test_runner/event_sender.cc:1927:
> DCHECK(args->GetNext(&x));
> DCHECKs aren't evaluated in Release builds, so this function call won't
happen.
> Worse, MSan can no longer catch this bug because you've pre-initialized the
> variables (though in this particular case you'll get a compiler warning about
> unused variables).
s/unused/uninitialized/

[mustaq, 2014-09-23 14:53:40]

On 2014/09/23 14:34:05, earthdok wrote:
> On 2014/09/23 14:33:08, earthdok wrote:
> >
>
https://codereview.chromium.org/584203003/diff/20001/content/shell/renderer/t...
> > File content/shell/renderer/test_runner/event_sender.cc (right):
> > 
> >
>
https://codereview.chromium.org/584203003/diff/20001/content/shell/renderer/t...
> > content/shell/renderer/test_runner/event_sender.cc:1927:
> > DCHECK(args->GetNext(&x));
> > DCHECKs aren't evaluated in Release builds, so this function call won't
> happen.
> > Worse, MSan can no longer catch this bug because you've pre-initialized the
> > variables (though in this particular case you'll get a compiler warning
about
> > unused variables).
> s/unused/uninitialized/

- I will follow Rick's suggestion here instead of using CHECKs.
- Apparently there are quite a few unchecked GetNext calls here, I'll try to fix
the obvious ones.
- Re uninitialized vars: linux_chromium_gn_rel test package failed in my first
patch above when I had no pre-initialization.

[earthdok, 2014-09-23 15:16:10]

On 2014/09/23 14:53:40, mustaq wrote:
> On 2014/09/23 14:34:05, earthdok wrote:
> > On 2014/09/23 14:33:08, earthdok wrote:
> > >
> >
>
https://codereview.chromium.org/584203003/diff/20001/content/shell/renderer/t...
> > > File content/shell/renderer/test_runner/event_sender.cc (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/584203003/diff/20001/content/shell/renderer/t...
> > > content/shell/renderer/test_runner/event_sender.cc:1927:
> > > DCHECK(args->GetNext(&x));
> > > DCHECKs aren't evaluated in Release builds, so this function call won't
> > happen.
> > > Worse, MSan can no longer catch this bug because you've pre-initialized
the
> > > variables (though in this particular case you'll get a compiler warning
> about
> > > unused variables).
> > s/unused/uninitialized/
> 
> - I will follow Rick's suggestion here instead of using CHECKs.
> - Apparently there are quite a few unchecked GetNext calls here, I'll try to
fix
> the obvious ones.
> - Re uninitialized vars: linux_chromium_gn_rel test package failed in my first
> patch above when I had no pre-initialization.

It failed precisely because the GetNext() calls were inside a DCHECK and were
skipped in the Release build. Pre-initializing the variables just masked the
real bug.

[mustaq, 2014-09-23 15:34:17]

On 2014/09/23 15:16:10, earthdok wrote:
> On 2014/09/23 14:53:40, mustaq wrote:
> > On 2014/09/23 14:34:05, earthdok wrote:
> > > On 2014/09/23 14:33:08, earthdok wrote:
> > > >
> > >
> >
>
https://codereview.chromium.org/584203003/diff/20001/content/shell/renderer/t...
> > > > File content/shell/renderer/test_runner/event_sender.cc (right):
> > > > 
> > > >
> > >
> >
>
https://codereview.chromium.org/584203003/diff/20001/content/shell/renderer/t...
> > > > content/shell/renderer/test_runner/event_sender.cc:1927:
> > > > DCHECK(args->GetNext(&x));
> > > > DCHECKs aren't evaluated in Release builds, so this function call won't
> > > happen.
> > > > Worse, MSan can no longer catch this bug because you've pre-initialized
> the
> > > > variables (though in this particular case you'll get a compiler warning
> > about
> > > > unused variables).
> > > s/unused/uninitialized/
> > 
> > - I will follow Rick's suggestion here instead of using CHECKs.
> > - Apparently there are quite a few unchecked GetNext calls here, I'll try to
> fix
> > the obvious ones.
> > - Re uninitialized vars: linux_chromium_gn_rel test package failed in my
first
> > patch above when I had no pre-initialization.
> 
> It failed precisely because the GetNext() calls were inside a DCHECK and were
> skipped in the Release build. Pre-initializing the variables just masked the
> real bug.

Done. PTAL.

[earthdok, 2014-09-23 15:44:47]

lgtm with nit

https://codereview.chromium.org/584203003/diff/40001/content/shell/renderer/t...
File content/shell/renderer/test_runner/event_sender.cc (right):

https://codereview.chromium.org/584203003/diff/40001/content/shell/renderer/t...
content/shell/renderer/test_runner/event_sender.cc:1648: args->ThrowError();
Why not merge the two if statements? (Unless of course the exception includes a
stack trace pointing to the specific ThrowError() call.)

[mustaq, 2014-09-23 15:56:16]

PTAL.

https://codereview.chromium.org/584203003/diff/40001/content/shell/renderer/t...
File content/shell/renderer/test_runner/event_sender.cc (right):

https://codereview.chromium.org/584203003/diff/40001/content/shell/renderer/t...
content/shell/renderer/test_runner/event_sender.cc:1648: args->ThrowError();
On 2014/09/23 15:44:46, earthdok wrote:
> Why not merge the two if statements? (Unless of course the exception includes
a
> stack trace pointing to the specific ThrowError() call.)

Good point. The merging doesn't obscure the error message very much because
ThrowError points to the "position" of the failed argument:
https://code.google.com/p/chromium/codesearch#chromium/src/gin/arguments.cc&l=40

[earthdok, 2014-09-23 16:00:09]

On 2014/09/23 15:56:16, mustaq wrote:
> PTAL.
> 
>
https://codereview.chromium.org/584203003/diff/40001/content/shell/renderer/t...
> File content/shell/renderer/test_runner/event_sender.cc (right):
> 
>
https://codereview.chromium.org/584203003/diff/40001/content/shell/renderer/t...
> content/shell/renderer/test_runner/event_sender.cc:1648: args->ThrowError();
> On 2014/09/23 15:44:46, earthdok wrote:
> > Why not merge the two if statements? (Unless of course the exception
includes
> a
> > stack trace pointing to the specific ThrowError() call.)
> 
> Good point. The merging doesn't obscure the error message very much because
> ThrowError points to the "position" of the failed argument:
>
https://code.google.com/p/chromium/codesearch#chromium/src/gin/arguments.cc&l=40

still lgtm

[mustaq, 2014-09-23 16:01:16]

The CQ bit was checked by mustaq@chromium.org

[mustaq, 2014-09-23 16:01:16]

The CQ bit was unchecked by mustaq@chromium.org

[Rick Byers, 2014-09-23 16:01:58]

Thanks, LGTM

[Rick Byers, 2014-09-23 16:02:38]

On 2014/09/23 16:01:58, Rick Byers wrote:
> Thanks, LGTM

Oh, but be sure to update your CL title and description to match the new change.

[Dirk Pranke, 2014-09-23 20:00:49]

lgtm

[mustaq, 2014-09-23 20:09:35]

The CQ bit was checked by mustaq@chromium.org

[commit-bot: I haz the power, 2014-09-23 20:31:40]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/584203003/60001

[commit-bot: I haz the power, 2014-09-23 21:36:04]

Committed patchset #4 (id:60001) as 6a933bb92126c2746c701ae7706f8b8a27e88db3

[commit-bot: I haz the power, 2014-09-23 21:36:43]

Patchset 4 (id:??) landed as
https://crrev.com/302907463636d2ddfe25c5381bd1f2b64016d1c6
Cr-Commit-Position: refs/heads/master@{#296256}

[Mads Ager (chromium), 2014-09-24 07:02:31]

A revert of this CL (patchset #4 id:60001) has been created in
https://codereview.chromium.org/600733002/ by ager@chromium.org.

The reason for reverting is: This makes the fast/events/hit-test-counts.html
layout test fail. Reverting for now.

http://test-results.appspot.com/dashboards/flakiness_dashboard.html#tests=fas....

[mustaq, 2014-09-24 19:10:35]

On 2014/09/24 07:02:31, Mads Ager (chromium) wrote:
> A revert of this CL (patchset #4 id:60001) has been created in
> https://codereview.chromium.org/600733002/ by mailto:ager@chromium.org.
> 
> The reason for reverting is: This makes the fast/events/hit-test-counts.html
> layout test fail. Reverting for now.
> 
>
http://test-results.appspot.com/dashboards/flakiness_dashboard.html#tests=fas....

Landed a fix for hit-test-count (https://codereview.chromium.org/603593002) few
hours ago. I'll force a few blink tests here now, then commit this change as is.

[mustaq, 2014-09-24 20:19:38]

The CQ bit was checked by mustaq@chromium.org

[commit-bot: I haz the power, 2014-09-24 20:20:53]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/584203003/60001

[commit-bot: I haz the power, 2014-09-24 20:21:50]

Committed patchset #4 (id:60001) as dc2709d99f96c0ab255a1992de45c0c768642b8c

[commit-bot: I haz the power, 2014-09-24 20:22:32]

Patchset 4 (id:??) landed as
https://crrev.com/0fff7fc37d0fc782f10b9eef2d4047bd286d1ff7
Cr-Commit-Position: refs/heads/master@{#296505}