Adding NOTREACHED in Read() in Event StructTraits to add clarity to the code.

Adding NOTREACHED in Read() in Event StructTraits to add clarity to the code.

BUG=NONE

[mfomitchev, 2017-02-28 19:50:52]

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

[commit-bot: I haz the power, 2017-02-28 19:51:34]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-02-28 20:14:42]

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

[commit-bot: I haz the power, 2017-02-28 20:14:43]

Dry run: Try jobs failed on following builders:
  android_compile_dbg on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...)

[mfomitchev, 2017-02-28 21:17:48]

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

[commit-bot: I haz the power, 2017-02-28 21:18:20]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-02-28 21:18:20]

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

[commit-bot: I haz the power, 2017-02-28 21:18:21]

Dry run: CLs for remote refs other than refs/heads/master must contain
NOTRY=true and NOPRESUBMIT=true in order for the CQ to process them

[mfomitchev, 2017-02-28 21:33:53]

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

[commit-bot: I haz the power, 2017-02-28 21:34:29]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-02-28 21:34:30]

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

[commit-bot: I haz the power, 2017-02-28 21:34:31]

Dry run: CLs for remote refs other than refs/heads/master must contain
NOTRY=true and NOPRESUBMIT=true in order for the CQ to process them

[mfomitchev, 2017-02-28 21:38:44]

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

[commit-bot: I haz the power, 2017-02-28 21:39:32]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-02-28 22:35:21]

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

[commit-bot: I haz the power, 2017-02-28 22:35:22]

Dry run: This issue passed the CQ dry run.

[mfomitchev, 2017-03-01 22:09:06]

Description was changed from

==========
Tiny cleanup in Event StructTraits.

BUG=NONE
==========

to

==========
Adding NOTREACHED in Read() in Event StructTraits to add clarity to the code.

BUG=NONE
==========

[mfomitchev, 2017-03-01 22:09:27]

mfomitchev@chromium.org changed reviewers:
+ sadrul@chromium.org

[sadrul, 2017-03-02 00:00:39]

https://codereview.chromium.org/2719223004/diff/40001/ui/events/mojo/event_st...
File ui/events/mojo/event_struct_traits.cc (right):

https://codereview.chromium.org/2719223004/diff/40001/ui/events/mojo/event_st...
ui/events/mojo/event_struct_traits.cc:311: NOTREACHED();
I am not sure if NOTREACHED() is good here. Because this means a sender can
trigger a crash to a remote app by sending it an invalid event, which I think is
something we should avoid.

[mfomitchev, 2017-03-02 00:03:41]

https://codereview.chromium.org/2719223004/diff/40001/ui/events/mojo/event_st...
File ui/events/mojo/event_struct_traits.cc (right):

https://codereview.chromium.org/2719223004/diff/40001/ui/events/mojo/event_st...
ui/events/mojo/event_struct_traits.cc:311: NOTREACHED();
On 2017/03/02 00:00:38, sadrul wrote:
> I am not sure if NOTREACHED() is good here. Because this means a sender can
> trigger a crash to a remote app by sending it an invalid event, which I think
is
> something we should avoid.

If the event is invalid, it's type will be UNKNOWN, and we will return false,
no?

[sadrul, 2017-03-02 02:22:32]

https://codereview.chromium.org/2719223004/diff/40001/ui/events/mojo/event_st...
File ui/events/mojo/event_struct_traits.cc (right):

https://codereview.chromium.org/2719223004/diff/40001/ui/events/mojo/event_st...
ui/events/mojo/event_struct_traits.cc:311: NOTREACHED();
On 2017/03/02 00:03:41, mfomitchev wrote:
> On 2017/03/02 00:00:38, sadrul wrote:
> > I am not sure if NOTREACHED() is good here. Because this means a sender can
> > trigger a crash to a remote app by sending it an invalid event, which I
think
> is
> > something we should avoid.
> 
> If the event is invalid, it's type will be UNKNOWN, and we will return false,
> no?

I think it's possible for the sender to send an event with an undefined action
(i.e. none of the valid ui::mojom::EventTypes)?

[mfomitchev, 2017-03-02 03:05:45]

https://codereview.chromium.org/2719223004/diff/40001/ui/events/mojo/event_st...
File ui/events/mojo/event_struct_traits.cc (right):

https://codereview.chromium.org/2719223004/diff/40001/ui/events/mojo/event_st...
ui/events/mojo/event_struct_traits.cc:311: NOTREACHED();
On 2017/03/02 02:22:31, sadrul wrote:
> On 2017/03/02 00:03:41, mfomitchev wrote:
> > On 2017/03/02 00:00:38, sadrul wrote:
> > > I am not sure if NOTREACHED() is good here. Because this means a sender
can
> > > trigger a crash to a remote app by sending it an invalid event, which I
> think
> > is
> > > something we should avoid.
> > 
> > If the event is invalid, it's type will be UNKNOWN, and we will return
false,
> > no?
> 
> I think it's possible for the sender to send an event with an undefined action
> (i.e. none of the valid ui::mojom::EventTypes)?

Hmm.. at this point we have EventDataView object, and not just some random blob
of binary data. EventDataView::action() is returning a value of type
::ui::mojom::EventType, which I think means that it should be one of the valid
EventType values. If we somehow received an invalid serialized value, I think it
would be handled deeper in the guts of Mojo? Here we have a typed value
already..

As you may have guessed, I've put this up merely as a precursor/sanity check for
https://codereview.chromium.org/2728883002/. If we indeed have a case where we
can legitimately fall through to this return, we should probably structure the
code in that CL a little differently.

[sadrul, 2017-03-02 17:57:44]

On 2017/03/02 03:05:45, mfomitchev wrote:
>
https://codereview.chromium.org/2719223004/diff/40001/ui/events/mojo/event_st...
> File ui/events/mojo/event_struct_traits.cc (right):
> 
>
https://codereview.chromium.org/2719223004/diff/40001/ui/events/mojo/event_st...
> ui/events/mojo/event_struct_traits.cc:311: NOTREACHED();
> On 2017/03/02 02:22:31, sadrul wrote:
> > On 2017/03/02 00:03:41, mfomitchev wrote:
> > > On 2017/03/02 00:00:38, sadrul wrote:
> > > > I am not sure if NOTREACHED() is good here. Because this means a sender
> can
> > > > trigger a crash to a remote app by sending it an invalid event, which I
> > think
> > > is
> > > > something we should avoid.
> > > 
> > > If the event is invalid, it's type will be UNKNOWN, and we will return
> false,
> > > no?
> > 
> > I think it's possible for the sender to send an event with an undefined
action
> > (i.e. none of the valid ui::mojom::EventTypes)?
> 
> Hmm.. at this point we have EventDataView object, and not just some random
blob
> of binary data. EventDataView::action() is returning a value of type
> ::ui::mojom::EventType, which I think means that it should be one of the valid
> EventType values. If we somehow received an invalid serialized value, I think
it
> would be handled deeper in the guts of Mojo? Here we have a typed value
> already..
> 
> As you may have guessed, I've put this up merely as a precursor/sanity check
for
> https://codereview.chromium.org/2728883002/. If we indeed have a case where we
> can legitimately fall through to this return, we should probably structure the
> code in that CL a little differently.

Let's get advice from tsepez@

[mfomitchev, 2017-03-02 18:50:20]

mfomitchev@chromium.org changed reviewers:
+ tsepez@chromium.org

[mfomitchev, 2017-03-02 18:50:21]

+tsepez - can you please advise on the discussion we are having in the comments?

[Tom Sepez, 2017-03-02 18:54:37]

https://codereview.chromium.org/2719223004/diff/40001/ui/events/mojo/event_st...
File ui/events/mojo/event_struct_traits.cc (right):

https://codereview.chromium.org/2719223004/diff/40001/ui/events/mojo/event_st...
ui/events/mojo/event_struct_traits.cc:311: NOTREACHED();
Doesn't NOTREACHED() only fire in the debug code?  If so, its fine to leave it
as the release build avoids it, and we don't mind crashing in debug.

[mfomitchev, 2017-03-02 20:29:06]

Ok, it sounds like we can get to the end of the method when there is invalid
input, so I have adjusted CL https://codereview.chromium.org/2728883002/ to take
care of that. Can you please take another look at it? I will just get rid of
this CL.