Prevent browser crash resulting from misbehaving WebVR renderer requesting multiple VSyncs.

Prevent browser crash resulting from misbehaving WebVR renderer requesting multiple VSyncs.

This also fixes a logic error, where if we have multiple rAF loops running for the same VrDisplay, we mistakenly fire off a VSync request for each rAF loop, when we should be sending only one at a time.

BUG=695470
Review-Url: https://codereview.chromium.org/2711173002
Cr-Commit-Position: refs/heads/master@{#452711}
Committed: https://chromium.googlesource.com/chromium/src/+/45c6392a896fefd5bdaef09d80920a55e1ea2ac4

[mthiesse, 2017-02-23 16:46:24]

mthiesse@chromium.org changed reviewers:
+ bajones@chromium.org, dcheng@chromium.org

[mthiesse, 2017-02-23 16:46:25]

dcheng, please review mojom changes.
bajones, please review the rest.

[mthiesse, 2017-02-23 16:50:21]

Description was changed from

==========
Prevent browser crash resulting from misbehaving WebVR renderer.

BUG=695470
==========

to

==========
Prevent browser crash resulting from misbehaving WebVR renderer requesting
mutiple VSyncs.

BUG=695470
==========

[mthiesse, 2017-02-23 16:50:32]

Description was changed from

==========
Prevent browser crash resulting from misbehaving WebVR renderer requesting
mutiple VSyncs.

BUG=695470
==========

to

==========
Prevent browser crash resulting from misbehaving WebVR renderer requesting
multiple VSyncs.

BUG=695470
==========

[bajones, 2017-02-23 17:21:22]

https://codereview.chromium.org/2711173002/diff/1/chrome/browser/android/vr_s...
File chrome/browser/android/vr_shell/vr_shell_gl.cc (right):

https://codereview.chromium.org/2711173002/diff/1/chrome/browser/android/vr_s...
chrome/browser/android/vr_shell/vr_shell_gl.cc:180:
device::mojom::VRVSyncProvider::Error::ERROR_TRY_AGAIN);
Could you clarify this? If the VSyncProvider is being destroyed why are we
telling Blink to try again?

[mthiesse, 2017-02-23 18:11:59]

https://codereview.chromium.org/2711173002/diff/1/chrome/browser/android/vr_s...
File chrome/browser/android/vr_shell/vr_shell_gl.cc (right):

https://codereview.chromium.org/2711173002/diff/1/chrome/browser/android/vr_s...
chrome/browser/android/vr_shell/vr_shell_gl.cc:180:
device::mojom::VRVSyncProvider::Error::ERROR_TRY_AGAIN);
On 2017/02/23 17:21:22, bajones wrote:
> Could you clarify this? If the VSyncProvider is being destroyed why are we
> telling Blink to try again?

Added comments.

[bajones, 2017-02-23 18:35:56]

lgtm

[dcheng, 2017-02-23 19:36:59]

https://codereview.chromium.org/2711173002/diff/20001/device/vr/vr_service.mojom
File device/vr/vr_service.mojom (right):

https://codereview.chromium.org/2711173002/diff/20001/device/vr/vr_service.mo...
device/vr/vr_service.mojom:101: ERROR_NONE,
Nit: Error => Status { SUCCESS, TRY_AGAIN };

?

https://codereview.chromium.org/2711173002/diff/20001/device/vr/vr_service.mo...
device/vr/vr_service.mojom:103: ERROR_BAD_REQUEST,
I would just omit BAD_REQUEST from the enum completely.

https://codereview.chromium.org/2711173002/diff/20001/device/vr/vr_service.mo...
device/vr/vr_service.mojom:109: GetVSync() => (VRPose? pose,
mojo.common.mojom.TimeDelta time, int16 frameId, Error error);
Nit: 80 chars

[mthiesse, 2017-02-23 19:41:36]

https://codereview.chromium.org/2711173002/diff/20001/device/vr/vr_service.mojom
File device/vr/vr_service.mojom (right):

https://codereview.chromium.org/2711173002/diff/20001/device/vr/vr_service.mo...
device/vr/vr_service.mojom:103: ERROR_BAD_REQUEST,
On 2017/02/23 19:36:58, dcheng wrote:
> I would just omit BAD_REQUEST from the enum completely.

How would you recommend we handle the bad request case, where we call
mojo::ReportBadMessage, but still have to run the callback to prevent the
browser crashing?

[dcheng, 2017-02-23 19:45:27]

https://codereview.chromium.org/2711173002/diff/20001/device/vr/vr_service.mojom
File device/vr/vr_service.mojom (right):

https://codereview.chromium.org/2711173002/diff/20001/device/vr/vr_service.mo...
device/vr/vr_service.mojom:103: ERROR_BAD_REQUEST,
On 2017/02/23 19:41:35, mthiesse wrote:
> On 2017/02/23 19:36:58, dcheng wrote:
> > I would just omit BAD_REQUEST from the enum completely.
> 
> How would you recommend we handle the bad request case, where we call
> mojo::ReportBadMessage, but still have to run the callback to prevent the
> browser crashing?

We shouldn't have to invoke the callback on a bad message. If we do, that seems
like a bug in Mojo itself.

(I'm following up with some people about the crash report in the bug. The crash
is in DumpWithoutCrashing, which shouldn't crash, but it's showing up as a
renderer kill...)

[dcheng, 2017-02-23 19:47:18]

On 2017/02/23 19:45:27, dcheng wrote:
>
https://codereview.chromium.org/2711173002/diff/20001/device/vr/vr_service.mojom
> File device/vr/vr_service.mojom (right):
> 
>
https://codereview.chromium.org/2711173002/diff/20001/device/vr/vr_service.mo...
> device/vr/vr_service.mojom:103: ERROR_BAD_REQUEST,
> On 2017/02/23 19:41:35, mthiesse wrote:
> > On 2017/02/23 19:36:58, dcheng wrote:
> > > I would just omit BAD_REQUEST from the enum completely.
> > 
> > How would you recommend we handle the bad request case, where we call
> > mojo::ReportBadMessage, but still have to run the callback to prevent the
> > browser crashing?
> 
> We shouldn't have to invoke the callback on a bad message. If we do, that
seems
> like a bug in Mojo itself.
> 
> (I'm following up with some people about the crash report in the bug. The
crash
> is in DumpWithoutCrashing, which shouldn't crash, but it's showing up as a
> renderer kill...)

rsesek@ pointed out the obvious (that this is, in fact, a renderer kill, and
thus WAI)

[mthiesse, 2017-02-23 19:50:07]

On 2017/02/23 19:47:18, dcheng wrote:
> On 2017/02/23 19:45:27, dcheng wrote:
> >
>
https://codereview.chromium.org/2711173002/diff/20001/device/vr/vr_service.mojom
> > File device/vr/vr_service.mojom (right):
> > 
> >
>
https://codereview.chromium.org/2711173002/diff/20001/device/vr/vr_service.mo...
> > device/vr/vr_service.mojom:103: ERROR_BAD_REQUEST,
> > On 2017/02/23 19:41:35, mthiesse wrote:
> > > On 2017/02/23 19:36:58, dcheng wrote:
> > > > I would just omit BAD_REQUEST from the enum completely.
> > > 
> > > How would you recommend we handle the bad request case, where we call
> > > mojo::ReportBadMessage, but still have to run the callback to prevent the
> > > browser crashing?
> > 
> > We shouldn't have to invoke the callback on a bad message. If we do, that
> seems
> > like a bug in Mojo itself.
> > 
> > (I'm following up with some people about the crash report in the bug. The
> crash
> > is in DumpWithoutCrashing, which shouldn't crash, but it's showing up as a
> > renderer kill...)
> 
> rsesek@ pointed out the obvious (that this is, in fact, a renderer kill, and
> thus WAI)

Yes, the renderer kill is expected, but the browser crashes if we don't invoke
the callback. Is this being looked into?

[dcheng, 2017-02-23 19:51:22]

On 2017/02/23 19:50:07, mthiesse wrote:
> On 2017/02/23 19:47:18, dcheng wrote:
> > On 2017/02/23 19:45:27, dcheng wrote:
> > >
> >
>
https://codereview.chromium.org/2711173002/diff/20001/device/vr/vr_service.mojom
> > > File device/vr/vr_service.mojom (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/2711173002/diff/20001/device/vr/vr_service.mo...
> > > device/vr/vr_service.mojom:103: ERROR_BAD_REQUEST,
> > > On 2017/02/23 19:41:35, mthiesse wrote:
> > > > On 2017/02/23 19:36:58, dcheng wrote:
> > > > > I would just omit BAD_REQUEST from the enum completely.
> > > > 
> > > > How would you recommend we handle the bad request case, where we call
> > > > mojo::ReportBadMessage, but still have to run the callback to prevent
the
> > > > browser crashing?
> > > 
> > > We shouldn't have to invoke the callback on a bad message. If we do, that
> > seems
> > > like a bug in Mojo itself.
> > > 
> > > (I'm following up with some people about the crash report in the bug. The
> > crash
> > > is in DumpWithoutCrashing, which shouldn't crash, but it's showing up as a
> > > renderer kill...)
> > 
> > rsesek@ pointed out the obvious (that this is, in fact, a renderer kill, and
> > thus WAI)
> 
> Yes, the renderer kill is expected, but the browser crashes if we don't invoke
> the callback. Is this being looked into?

Is it actually a browser crash though? I don't see any indication that it is
from the crash report.

[dcheng, 2017-02-23 19:52:08]

On 2017/02/23 19:51:22, dcheng wrote:
> On 2017/02/23 19:50:07, mthiesse wrote:
> > On 2017/02/23 19:47:18, dcheng wrote:
> > > On 2017/02/23 19:45:27, dcheng wrote:
> > > >
> > >
> >
>
https://codereview.chromium.org/2711173002/diff/20001/device/vr/vr_service.mojom
> > > > File device/vr/vr_service.mojom (right):
> > > > 
> > > >
> > >
> >
>
https://codereview.chromium.org/2711173002/diff/20001/device/vr/vr_service.mo...
> > > > device/vr/vr_service.mojom:103: ERROR_BAD_REQUEST,
> > > > On 2017/02/23 19:41:35, mthiesse wrote:
> > > > > On 2017/02/23 19:36:58, dcheng wrote:
> > > > > > I would just omit BAD_REQUEST from the enum completely.
> > > > > 
> > > > > How would you recommend we handle the bad request case, where we call
> > > > > mojo::ReportBadMessage, but still have to run the callback to prevent
> the
> > > > > browser crashing?
> > > > 
> > > > We shouldn't have to invoke the callback on a bad message. If we do,
that
> > > seems
> > > > like a bug in Mojo itself.
> > > > 
> > > > (I'm following up with some people about the crash report in the bug.
The
> > > crash
> > > > is in DumpWithoutCrashing, which shouldn't crash, but it's showing up as
a
> > > > renderer kill...)
> > > 
> > > rsesek@ pointed out the obvious (that this is, in fact, a renderer kill,
and
> > > thus WAI)
> > 
> > Yes, the renderer kill is expected, but the browser crashes if we don't
invoke
> > the callback. Is this being looked into?
> 
> Is it actually a browser crash though? I don't see any indication that it is
> from the crash report.

(If it is a browser crash, then that's probably the wrong crash dump?)

[mthiesse, 2017-02-23 19:53:22]

On 2017/02/23 19:41:36, mthiesse wrote:
>
https://codereview.chromium.org/2711173002/diff/20001/device/vr/vr_service.mojom
> File device/vr/vr_service.mojom (right):
> 
>
https://codereview.chromium.org/2711173002/diff/20001/device/vr/vr_service.mo...
> device/vr/vr_service.mojom:103: ERROR_BAD_REQUEST,
> On 2017/02/23 19:36:58, dcheng wrote:
> > I would just omit BAD_REQUEST from the enum completely.
> 
> How would you recommend we handle the bad request case, where we call
> mojo::ReportBadMessage, but still have to run the callback to prevent the
> browser crashing?

https://paste.googleplex.com/5362279369408512?raw

[mthiesse, 2017-02-23 19:54:54]

The browser crash is from a DCHECK, so it won't be hit in release builds or
captured by the crash reporter. The stack is at
https://paste.googleplex.com/5362279369408512?raw

[dcheng, 2017-02-23 19:58:16]

On 2017/02/23 19:53:22, mthiesse wrote:
> On 2017/02/23 19:41:36, mthiesse wrote:
> >
>
https://codereview.chromium.org/2711173002/diff/20001/device/vr/vr_service.mojom
> > File device/vr/vr_service.mojom (right):
> > 
> >
>
https://codereview.chromium.org/2711173002/diff/20001/device/vr/vr_service.mo...
> > device/vr/vr_service.mojom:103: ERROR_BAD_REQUEST,
> > On 2017/02/23 19:36:58, dcheng wrote:
> > > I would just omit BAD_REQUEST from the enum completely.
> > 
> > How would you recommend we handle the bad request case, where we call
> > mojo::ReportBadMessage, but still have to run the callback to prevent the
> > browser crashing?
> 
> https://paste.googleplex.com/5362279369408512?raw

I filed a bug for this: https://crbug.com/695593

However, note that if this is something a non-compromised renderer can trigger,
this needs to be handled more gracefully anyway: the renderer side should be
rejecting these calls.

[mthiesse, 2017-02-23 20:05:03]

On 2017/02/23 19:58:16, dcheng wrote:
> On 2017/02/23 19:53:22, mthiesse wrote:
> > On 2017/02/23 19:41:36, mthiesse wrote:
> > >
> >
>
https://codereview.chromium.org/2711173002/diff/20001/device/vr/vr_service.mojom
> > > File device/vr/vr_service.mojom (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/2711173002/diff/20001/device/vr/vr_service.mo...
> > > device/vr/vr_service.mojom:103: ERROR_BAD_REQUEST,
> > > On 2017/02/23 19:36:58, dcheng wrote:
> > > > I would just omit BAD_REQUEST from the enum completely.
> > > 
> > > How would you recommend we handle the bad request case, where we call
> > > mojo::ReportBadMessage, but still have to run the callback to prevent the
> > > browser crashing?
> > 
> > https://paste.googleplex.com/5362279369408512?raw
> 
> I filed a bug for this: https://crbug.com/695593
> 
> However, note that if this is something a non-compromised renderer can
trigger,
> this needs to be handled more gracefully anyway: the renderer side should be
> rejecting these calls.

Right, it was a bug that originally started this investigation. A
non-compromised renderer will never trigger this, barring buggy implementation
;)

[dcheng, 2017-02-23 20:39:14]

On 2017/02/23 20:05:03, mthiesse wrote:
> On 2017/02/23 19:58:16, dcheng wrote:
> > On 2017/02/23 19:53:22, mthiesse wrote:
> > > On 2017/02/23 19:41:36, mthiesse wrote:
> > > >
> > >
> >
>
https://codereview.chromium.org/2711173002/diff/20001/device/vr/vr_service.mojom
> > > > File device/vr/vr_service.mojom (right):
> > > > 
> > > >
> > >
> >
>
https://codereview.chromium.org/2711173002/diff/20001/device/vr/vr_service.mo...
> > > > device/vr/vr_service.mojom:103: ERROR_BAD_REQUEST,
> > > > On 2017/02/23 19:36:58, dcheng wrote:
> > > > > I would just omit BAD_REQUEST from the enum completely.
> > > > 
> > > > How would you recommend we handle the bad request case, where we call
> > > > mojo::ReportBadMessage, but still have to run the callback to prevent
the
> > > > browser crashing?
> > > 
> > > https://paste.googleplex.com/5362279369408512?raw
> > 
> > I filed a bug for this: https://crbug.com/695593
> > 
> > However, note that if this is something a non-compromised renderer can
> trigger,
> > this needs to be handled more gracefully anyway: the renderer side should be
> > rejecting these calls.
> 
> Right, it was a bug that originally started this investigation. A
> non-compromised renderer will never trigger this, barring buggy implementation
> ;)

I don't follow: the example in the bug is just an ordinary page. The renderer is
not compromised; just executing normal JS shouldn't result in a renderer kill
situation.

[mthiesse, 2017-02-23 20:39:58]

https://codereview.chromium.org/2711173002/diff/20001/device/vr/vr_service.mojom
File device/vr/vr_service.mojom (right):

https://codereview.chromium.org/2711173002/diff/20001/device/vr/vr_service.mo...
device/vr/vr_service.mojom:101: ERROR_NONE,
On 2017/02/23 19:36:58, dcheng wrote:
> Nit: Error => Status { SUCCESS, TRY_AGAIN };
> 
> ?

Fun fact, I can't use TRY_AGAIN as an enum value, it seems there's a collision
with some Android headers somehow? I've changed it to RETRY.

Compile error:
gen/device/vr/vr_service.mojom-shared.h:137:3: error: expected identifier
  TRY_AGAIN,
  ^
../../third_party/android_tools/ndk/platforms/android-16/arch-arm/usr/include/netdb.h:132:19:
note: expanded from macro 'TRY_AGAIN'
#define TRY_AGAIN       2 /* Non-Authoritative Host not found, or SERVERFAIL */
                        ^
In file included from
../../content/browser/frame_host/render_frame_host_impl.cc:129:
In file included from ../../device/vr/vr_service_impl.h:12:
In file included from ../../device/vr/vr_device.h:11:
In file included from gen/device/vr/vr_service.mojom.h:32:
gen/device/vr/vr_service.mojom-shared.h:144:35: error: expected unqualified-id
    case VRVSyncProvider_Statuss::TRY_AGAIN:
                                  ^
../../third_party/android_tools/ndk/platforms/android-16/arch-arm/usr/include/netdb.h:132:19:
note: expanded from macro 'TRY_AGAIN'
#define TRY_AGAIN       2 /* Non-Authoritative Host not found, or SERVERFAIL */

https://codereview.chromium.org/2711173002/diff/20001/device/vr/vr_service.mo...
device/vr/vr_service.mojom:103: ERROR_BAD_REQUEST,
On 2017/02/23 19:45:27, dcheng wrote:
> On 2017/02/23 19:41:35, mthiesse wrote:
> > On 2017/02/23 19:36:58, dcheng wrote:
> > > I would just omit BAD_REQUEST from the enum completely.
> > 
> > How would you recommend we handle the bad request case, where we call
> > mojo::ReportBadMessage, but still have to run the callback to prevent the
> > browser crashing?
> 
> We shouldn't have to invoke the callback on a bad message. If we do, that
seems
> like a bug in Mojo itself.
> 
> (I'm following up with some people about the crash report in the bug. The
crash
> is in DumpWithoutCrashing, which shouldn't crash, but it's showing up as a
> renderer kill...)

Acknowledged.

https://codereview.chromium.org/2711173002/diff/20001/device/vr/vr_service.mo...
device/vr/vr_service.mojom:109: GetVSync() => (VRPose? pose,
mojo.common.mojom.TimeDelta time, int16 frameId, Error error);
On 2017/02/23 19:36:58, dcheng wrote:
> Nit: 80 chars

Done.

[mthiesse, 2017-02-23 20:41:26]

On 2017/02/23 20:39:14, dcheng wrote:
> On 2017/02/23 20:05:03, mthiesse wrote:
> > On 2017/02/23 19:58:16, dcheng wrote:
> > > On 2017/02/23 19:53:22, mthiesse wrote:
> > > > On 2017/02/23 19:41:36, mthiesse wrote:
> > > > >
> > > >
> > >
> >
>
https://codereview.chromium.org/2711173002/diff/20001/device/vr/vr_service.mojom
> > > > > File device/vr/vr_service.mojom (right):
> > > > > 
> > > > >
> > > >
> > >
> >
>
https://codereview.chromium.org/2711173002/diff/20001/device/vr/vr_service.mo...
> > > > > device/vr/vr_service.mojom:103: ERROR_BAD_REQUEST,
> > > > > On 2017/02/23 19:36:58, dcheng wrote:
> > > > > > I would just omit BAD_REQUEST from the enum completely.
> > > > > 
> > > > > How would you recommend we handle the bad request case, where we call
> > > > > mojo::ReportBadMessage, but still have to run the callback to prevent
> the
> > > > > browser crashing?
> > > > 
> > > > https://paste.googleplex.com/5362279369408512?raw
> > > 
> > > I filed a bug for this: https://crbug.com/695593
> > > 
> > > However, note that if this is something a non-compromised renderer can
> > trigger,
> > > this needs to be handled more gracefully anyway: the renderer side should
be
> > > rejecting these calls.
> > 
> > Right, it was a bug that originally started this investigation. A
> > non-compromised renderer will never trigger this, barring buggy
implementation
> > ;)
> 
> I don't follow: the example in the bug is just an ordinary page. The renderer
is
> not compromised; just executing normal JS shouldn't result in a renderer kill
> situation.

The bug was in VrDisplay.cpp, fixed in this change. We were mistakenly sending
mojo messages where we shouldn't have been.

The page in the bug not works as expected, unchanged.

[mthiesse, 2017-02-23 20:43:28]

On 2017/02/23 20:41:26, mthiesse wrote:
> On 2017/02/23 20:39:14, dcheng wrote:
> > On 2017/02/23 20:05:03, mthiesse wrote:
> > > On 2017/02/23 19:58:16, dcheng wrote:
> > > > On 2017/02/23 19:53:22, mthiesse wrote:
> > > > > On 2017/02/23 19:41:36, mthiesse wrote:
> > > > > >
> > > > >
> > > >
> > >
> >
>
https://codereview.chromium.org/2711173002/diff/20001/device/vr/vr_service.mojom
> > > > > > File device/vr/vr_service.mojom (right):
> > > > > > 
> > > > > >
> > > > >
> > > >
> > >
> >
>
https://codereview.chromium.org/2711173002/diff/20001/device/vr/vr_service.mo...
> > > > > > device/vr/vr_service.mojom:103: ERROR_BAD_REQUEST,
> > > > > > On 2017/02/23 19:36:58, dcheng wrote:
> > > > > > > I would just omit BAD_REQUEST from the enum completely.
> > > > > > 
> > > > > > How would you recommend we handle the bad request case, where we
call
> > > > > > mojo::ReportBadMessage, but still have to run the callback to
prevent
> > the
> > > > > > browser crashing?
> > > > > 
> > > > > https://paste.googleplex.com/5362279369408512?raw
> > > > 
> > > > I filed a bug for this: https://crbug.com/695593
> > > > 
> > > > However, note that if this is something a non-compromised renderer can
> > > trigger,
> > > > this needs to be handled more gracefully anyway: the renderer side
should
> be
> > > > rejecting these calls.
> > > 
> > > Right, it was a bug that originally started this investigation. A
> > > non-compromised renderer will never trigger this, barring buggy
> implementation
> > > ;)
> > 
> > I don't follow: the example in the bug is just an ordinary page. The
renderer
> is
> > not compromised; just executing normal JS shouldn't result in a renderer
kill
> > situation.
> 
> The bug was in VrDisplay.cpp, fixed in this change. We were mistakenly sending
> mojo messages where we shouldn't have been.
> 
> The page in the bug not works as expected, unchanged.

s/not/now

[dcheng, 2017-02-23 21:20:02]

LGTM

Perhaps clarify the description to cover the logic fixes (outside of the crash
fix) in the Blink plumbing too though.

[mthiesse, 2017-02-23 21:26:04]

Description was changed from

==========
Prevent browser crash resulting from misbehaving WebVR renderer requesting
multiple VSyncs.

BUG=695470
==========

to

==========
Prevent browser crash resulting from misbehaving WebVR renderer requesting
multiple VSyncs.

This also fixes a logic error, where if we have multiple rAF loops running for
the same VR display, we mistakenly fire off a VSync request for each rAF loop,
when we should be sending only one at a time.

BUG=695470
==========

[mthiesse, 2017-02-23 21:26:35]

Description was changed from

==========
Prevent browser crash resulting from misbehaving WebVR renderer requesting
multiple VSyncs.

This also fixes a logic error, where if we have multiple rAF loops running for
the same VR display, we mistakenly fire off a VSync request for each rAF loop,
when we should be sending only one at a time.

BUG=695470
==========

to

==========
Prevent browser crash resulting from misbehaving WebVR renderer requesting
multiple VSyncs.

This also fixes a logic error, where if we have multiple rAF loops running for
the same VrDisplay, we mistakenly fire off a VSync request for each rAF loop,
when we should be sending only one at a time.

BUG=695470
==========

[mthiesse, 2017-02-23 21:26:43]

The CQ bit was checked by mthiesse@chromium.org

[mthiesse, 2017-02-23 21:26:44]

The patchset sent to the CQ was uploaded after l-g-t-m from bajones@chromium.org
Link to the patchset: https://codereview.chromium.org/2711173002/#ps60001
(title: "nit")

[commit-bot: I haz the power, 2017-02-23 21:27:19]

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-23 22:59:34]

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

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

Failed to apply patch for
chrome/browser/android/vr_shell/non_presenting_gvr_delegate.cc:
While running git apply --index -p1;
  error: patch failed:
chrome/browser/android/vr_shell/non_presenting_gvr_delegate.cc:110
  error: chrome/browser/android/vr_shell/non_presenting_gvr_delegate.cc: patch
does not apply

Patch:       chrome/browser/android/vr_shell/non_presenting_gvr_delegate.cc
Index: chrome/browser/android/vr_shell/non_presenting_gvr_delegate.cc
diff --git a/chrome/browser/android/vr_shell/non_presenting_gvr_delegate.cc
b/chrome/browser/android/vr_shell/non_presenting_gvr_delegate.cc
index
2e69283b77756ce329c1810f4350599111a2e4c3..337501cfc659e8a96e48750b0e58343908c8ab82
100644
--- a/chrome/browser/android/vr_shell/non_presenting_gvr_delegate.cc
+++ b/chrome/browser/android/vr_shell/non_presenting_gvr_delegate.cc
@@ -61,7 +61,9 @@ NonPresentingGvrDelegate::OnSwitchToPresentingDelegate() {
 void NonPresentingGvrDelegate::StopVSyncLoop() {
   vsync_task_.Cancel();
   if (!callback_.is_null()) {
-    base::ResetAndReturn(&callback_).Run(nullptr, base::TimeDelta(), -1);
+    base::ResetAndReturn(&callback_)
+        .Run(nullptr, base::TimeDelta(), -1,
+             device::mojom::VRVSyncProvider::Status::RETRY);
   }
   gvr_api_->PauseTracking();
   // If the loop is stopped, it's not considered to be paused.
@@ -110,6 +112,7 @@ void NonPresentingGvrDelegate::GetVSync(const
GetVSyncCallback& callback) {
     if (!callback_.is_null()) {
       mojo::ReportBadMessage("Requested VSync before waiting for response to "
           "previous request.");
+      binding_.Close();
       return;
     }
     callback_ = callback;
@@ -134,7 +137,8 @@ void NonPresentingGvrDelegate::SendVSync(base::TimeDelta
time,
 
   gvr::Mat4f head_mat = gvr_api_->ApplyNeckModel(
       gvr_api_->GetHeadSpaceFromStartSpaceRotation(target_time), 1.0f);
-  callback.Run(VrShell::VRPosePtrFromGvrPose(head_mat), time, -1);
+  callback.Run(VrShell::VRPosePtrFromGvrPose(head_mat), time, -1,
+               device::mojom::VRVSyncProvider::Status::SUCCESS);
 }
 
 bool NonPresentingGvrDelegate::SupportsPresentation() {

[mthiesse, 2017-02-23 23:46:50]

The CQ bit was checked by mthiesse@chromium.org

[mthiesse, 2017-02-23 23:46:50]

The patchset sent to the CQ was uploaded after l-g-t-m from
bajones@chromium.org, dcheng@chromium.org
Link to the patchset: https://codereview.chromium.org/2711173002/#ps80001
(title: "rebase")

[commit-bot: I haz the power, 2017-02-23 23:47:37]

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-24 01:45:29]

CQ is committing da patch.

Bot data: {"patchset_id": 80001, "attempt_start_ts": 1487893610135550,
"parent_rev": "208c75a57f289ba9e0fc10582179320900ad77e5", "commit_rev":
"45c6392a896fefd5bdaef09d80920a55e1ea2ac4"}

[commit-bot: I haz the power, 2017-02-24 01:46:54]

Description was changed from

==========
Prevent browser crash resulting from misbehaving WebVR renderer requesting
multiple VSyncs.

This also fixes a logic error, where if we have multiple rAF loops running for
the same VrDisplay, we mistakenly fire off a VSync request for each rAF loop,
when we should be sending only one at a time.

BUG=695470
==========

to

==========
Prevent browser crash resulting from misbehaving WebVR renderer requesting
multiple VSyncs.

This also fixes a logic error, where if we have multiple rAF loops running for
the same VrDisplay, we mistakenly fire off a VSync request for each rAF loop,
when we should be sending only one at a time.

BUG=695470

Review-Url: https://codereview.chromium.org/2711173002
Cr-Commit-Position: refs/heads/master@{#452711}
Committed:
https://chromium.googlesource.com/chromium/src/+/45c6392a896fefd5bdaef09d8092...
==========

[commit-bot: I haz the power, 2017-02-24 01:46:56]

Committed patchset #5 (id:80001) as
https://chromium.googlesource.com/chromium/src/+/45c6392a896fefd5bdaef09d8092...