Added early out for pinch to prevent crash when no inner scroll layer.

Added early out for pinch to prevent crash when no inner scroll layer.

The layer tree seems to receive gesture events before it's initialized
(or while being destroyed). This means the pinch events get executed
while InnerViewportScrollLayer() == NULL, which was assumed to be non
NULL. The event handler should probably not be sending events to an
uninitialized layer tree but in the mean time I've disabled the
DCHECK and added an early out to prevent crashes.

BUG=340303
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=252205

[bokan, 2014-02-18 21:09:46]

I've filed crbug.com/344617 to track actually gating the events so that they
don't get to the layer tree before/after InnerViewportScrollLayer (and possibly
other pieces of the tree) are valid. In the mean time, I've turned the DCHECK
into an early out (needed in all of Begin/Update/End since they keep state
between themselves).

[wjmaclean, 2014-02-18 21:16:59]

https://codereview.chromium.org/170743004/diff/1/cc/trees/layer_tree_host_imp...
File cc/trees/layer_tree_host_impl.cc (right):

https://codereview.chromium.org/170743004/diff/1/cc/trees/layer_tree_host_imp...
cc/trees/layer_tree_host_impl.cc:2494: if (!InnerViewportScrollLayer())
Do we really need early outs in PinchGestureBegin()/PinchGestureEnd()? Having
three checks seems excessive, since the original code only checked in the
PinchGestureUpdate() function, and that's the only place (I think) a problem can
actually occur.

[bokan, 2014-02-18 22:12:10]

https://codereview.chromium.org/170743004/diff/1/cc/trees/layer_tree_host_imp...
File cc/trees/layer_tree_host_impl.cc (right):

https://codereview.chromium.org/170743004/diff/1/cc/trees/layer_tree_host_imp...
cc/trees/layer_tree_host_impl.cc:2494: if (!InnerViewportScrollLayer())
On 2014/02/18 21:16:59, wjmaclean wrote:
> Do we really need early outs in PinchGestureBegin()/PinchGestureEnd()? Having
> three checks seems excessive, since the original code only checked in the
> PinchGestureUpdate() function, and that's the only place (I think) a problem
can
> actually occur.

I think it's safer/cleaner not to have any side effects if we're not actually
performing the pinch - but I've tried it without and it doesn't crash (at least
in the limited testing I've done) so I've removed them on Begin/End.

[wjmaclean, 2014-02-18 22:34:37]

On 2014/02/18 22:12:10, bokan wrote:
>
https://codereview.chromium.org/170743004/diff/1/cc/trees/layer_tree_host_imp...
> File cc/trees/layer_tree_host_impl.cc (right):
> 
>
https://codereview.chromium.org/170743004/diff/1/cc/trees/layer_tree_host_imp...
> cc/trees/layer_tree_host_impl.cc:2494: if (!InnerViewportScrollLayer())
> On 2014/02/18 21:16:59, wjmaclean wrote:
> > Do we really need early outs in PinchGestureBegin()/PinchGestureEnd()?
Having
> > three checks seems excessive, since the original code only checked in the
> > PinchGestureUpdate() function, and that's the only place (I think) a problem
> can
> > actually occur.
> 
> I think it's safer/cleaner not to have any side effects if we're not actually
> performing the pinch - but I've tried it without and it doesn't crash (at
least
> in the limited testing I've done) so I've removed them on Begin/End.

I understand where you're coming from, although if the side effects were
worrysome then the check would have existed in all three functions from the
get-go I suspect.

LGTM

[bokan, 2014-02-19 16:52:20]

+aelias@ for OWNER

[aelias_OOO_until_Jul13, 2014-02-19 19:27:00]

lgtm

[bokan, 2014-02-19 21:56:45]

The CQ bit was checked by bokan@chromium.org

[commit-bot: I haz the power, 2014-02-19 22:15:18]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/bokan@chromium.org/170743004/70001

[commit-bot: I haz the power, 2014-02-20 00:47:46]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/bokan@chromium.org/170743004/70001

[commit-bot: I haz the power, 2014-02-20 03:38:31]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/bokan@chromium.org/170743004/70001

[commit-bot: I haz the power, 2014-02-20 08:46:04]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/bokan@chromium.org/170743004/70001

[commit-bot: I haz the power, 2014-02-20 12:19:42]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/bokan@chromium.org/170743004/70001

[commit-bot: I haz the power, 2014-02-20 13:07:04]

Change committed as 252205