Protect WebViewImpl::handleGestureEvent from null client

Protect WebViewImpl::handleGestureEvent from null client

It is possible to crash in WebViewImpl::handleGestureEvent
due to a null client, as shown in the unit test
ClientTapHandlingClosedWindow. So if the client
is null return early.

Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=191797

[rwlbuis, 2015-03-06 21:50:49]

rob.buis@samsung.com changed reviewers:
+ rbyers@chromium.org

[rwlbuis, 2015-03-06 21:50:50]

PTAL

[Rick Byers, 2015-03-07 15:26:01]

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

[Rick Byers, 2015-03-07 15:26:28]

LGTM.  I can imagine there are shutdown races here, thanks!

+aelias for OWNERS

[aelias_OOO_until_Jul13, 2015-03-08 20:37:13]

https://codereview.chromium.org/981223003/diff/1/Source/web/tests/WebViewTest...
File Source/web/tests/WebViewTest.cpp (right):

https://codereview.chromium.org/981223003/diff/1/Source/web/tests/WebViewTest...
Source/web/tests/WebViewTest.cpp:1286: webView->close();
Given that the close() is after the handleInputEvent() call, it doesn't seem
like this test gives us any coverage?  Does it really crash without the
WebViewImpl change?

[rwlbuis, 2015-03-09 13:58:29]

On 2015/03/08 20:37:13, aelias wrote:
>
https://codereview.chromium.org/981223003/diff/1/Source/web/tests/WebViewTest...
> File Source/web/tests/WebViewTest.cpp (right):
> 
>
https://codereview.chromium.org/981223003/diff/1/Source/web/tests/WebViewTest...
> Source/web/tests/WebViewTest.cpp:1286: webView->close();
> Given that the close() is after the handleInputEvent() call, it doesn't seem
> like this test gives us any coverage?  Does it really crash without the
> WebViewImpl change?

Sorry, that was a bit confusing. I first set out to try to cause a null client
through close(), but that derefs the WebViewImpl. Later I discovered it is
possible
to construct WebViews with null client(s) so I did that instead, but forgot to
change the (now worng) test name. Patch set 2 now has a better test name, and
the close at the end is just for freeing up the WebViewImpl that was created.

[aelias_OOO_until_Jul13, 2015-03-10 17:24:39]

On 2015/03/09 at 13:58:29, rob.buis wrote:
> On 2015/03/08 20:37:13, aelias wrote:
> >
https://codereview.chromium.org/981223003/diff/1/Source/web/tests/WebViewTest...
> > File Source/web/tests/WebViewTest.cpp (right):
> > 
> >
https://codereview.chromium.org/981223003/diff/1/Source/web/tests/WebViewTest...
> > Source/web/tests/WebViewTest.cpp:1286: webView->close();
> > Given that the close() is after the handleInputEvent() call, it doesn't seem
> > like this test gives us any coverage?  Does it really crash without the
> > WebViewImpl change?
> 
> Sorry, that was a bit confusing. I first set out to try to cause a null client
> through close(), but that derefs the WebViewImpl. Later I discovered it is
possible
> to construct WebViews with null client(s) so I did that instead, but forgot to
> change the (now worng) test name. Patch set 2 now has a better test name, and
> the close at the end is just for freeing up the WebViewImpl that was created.

It's possible we may want to make it impossible to set a null client in the
future (for example by passing a reference), and then this test would interfere
with that.  In practice, what's the scenario this occurred in and is it possible
to make the test recreate it?

[rwlbuis, 2015-03-11 21:15:57]

On 2015/03/10 17:24:39, aelias wrote:
> On 2015/03/09 at 13:58:29, rob.buis wrote:
> > On 2015/03/08 20:37:13, aelias wrote:
> > >
>
https://codereview.chromium.org/981223003/diff/1/Source/web/tests/WebViewTest...
> > > File Source/web/tests/WebViewTest.cpp (right):
> > > 
> > >
>
https://codereview.chromium.org/981223003/diff/1/Source/web/tests/WebViewTest...
> > > Source/web/tests/WebViewTest.cpp:1286: webView->close();
> > > Given that the close() is after the handleInputEvent() call, it doesn't
seem
> > > like this test gives us any coverage?  Does it really crash without the
> > > WebViewImpl change?
> > 
> > Sorry, that was a bit confusing. I first set out to try to cause a null
client
> > through close(), but that derefs the WebViewImpl. Later I discovered it is
> possible
> > to construct WebViews with null client(s) so I did that instead, but forgot
to
> > change the (now worng) test name. Patch set 2 now has a better test name,
and
> > the close at the end is just for freeing up the WebViewImpl that was
created.
> 
> It's possible we may want to make it impossible to set a null client in the
> future (for example by passing a reference), and then this test would
interfere
> with that.  In practice, what's the scenario this occurred in and is it
possible
> to make the test recreate it?

This was actually found by a static analyzer tool.
We could leave a FIXME or note to remove the test once WebViewImpl is
changed to always have a non-null client.

[aelias_OOO_until_Jul13, 2015-03-12 19:32:51]

OK, it looks like there's a comment saying m_client is expected to be sometimes
NULL for cases like shared workers, so lgtm.

[rwlbuis, 2015-03-12 20:03:58]

The CQ bit was checked by rob.buis@samsung.com

[rwlbuis, 2015-03-12 20:03:59]

The patchset sent to the CQ was uploaded after l-g-t-m from rbyers@chromium.org
Link to the patchset: https://codereview.chromium.org/981223003/#ps20001 (title:
"Update test name")

[commit-bot: I haz the power, 2015-03-12 20:04:27]

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

[commit-bot: I haz the power, 2015-03-12 21:30:49]

Committed patchset #2 (id:20001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=191797