Added DCHECK() to prevent webwidget_->isInputThrottled() from being called when webwidget_ is NULL.

Added DCHECK() to prevent webwidget_->isInputThrottled() from being called when webwidget_ is NULL.

BUG=
TEST=
CID=103685
TBR=jamesr

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=149918

[Tyler Breisacher (Chromium), 2012-07-26 18:32:14]

http://codereview.chromium.org/10830024/diff/1/content/renderer/render_widget.cc
File content/renderer/render_widget.cc (right):

http://codereview.chromium.org/10830024/diff/1/content/renderer/render_widget...
content/renderer/render_widget.cc:569: if (!processed && webwidget_)
I think you misread what Coverity is saying. This line implies that webwidget_
could be null in some cases. Your DCHECK implies that it should never be null.
Which one is right? I don't know. This is another case where you probably want
to talk to the owner or original author of the code and find out which states
are possible and which ones should be impossible. Maybe webwidget_ should
actually never be null in which case you can just change this line to

    if (!processed)

but I don't know without looking at the code a lot more carefully.

[Kyle Horimoto, 2012-07-26 18:40:57]

+nduca, apavlov

nduca & apavlov, you both contributed code to
RenderWidget::OnHandleInputEvent(). Coverity (static code analysis tool)
discovered that we may be dereferencing a NULL pointer on line 587.

apavlov, your code checks if webwidget_ is NULL, implying that it could possibly
be NULL at this point. However, nduca, your code dereferences webwidget_ without
checks, implying that it cannot possibly be NULL at this point (and this is what
Coverity complains about). Which is the correct assumption? And is the DCHECK()
that I inserted correct?

http://codereview.chromium.org/10830024/diff/1/content/renderer/render_widget.cc
File content/renderer/render_widget.cc (right):

http://codereview.chromium.org/10830024/diff/1/content/renderer/render_widget...
content/renderer/render_widget.cc:569: if (!processed && webwidget_)
On 2012/07/26 18:32:15, Tyler Breisacher wrote:
> I think you misread what Coverity is saying. This line implies that webwidget_
> could be null in some cases. Your DCHECK implies that it should never be null.
> Which one is right? I don't know. This is another case where you probably want
> to talk to the owner or original author of the code and find out which states
> are possible and which ones should be impossible. Maybe webwidget_ should
> actually never be null in which case you can just change this line to
> 
>     if (!processed)
> 
> but I don't know without looking at the code a lot more carefully.

Line 587 dereferences webwidget_. If it is indeed NULL, we will segfault (there
are no return statements between my DCHECK() and line 587). So that line implies
that webwidget_ should never be NULL. I'll add in the author of this file to
check.

[nduca, 2012-07-26 19:27:11]

http://codereview.chromium.org/10830024/diff/1/content/renderer/render_widget.cc
File content/renderer/render_widget.cc (right):

http://codereview.chromium.org/10830024/diff/1/content/renderer/render_widget...
content/renderer/render_widget.cc:569: if (!processed && webwidget_)
Just say webwidget_ ? webwidget_xxx : yyy where needed, where throttled can be
false.

[Kyle Horimoto, 2012-07-26 21:58:51]

http://codereview.chromium.org/10830024/diff/1/content/renderer/render_widget.cc
File content/renderer/render_widget.cc (right):

http://codereview.chromium.org/10830024/diff/1/content/renderer/render_widget...
content/renderer/render_widget.cc:569: if (!processed && webwidget_)
On 2012/07/26 19:27:11, nduca wrote:
> Just say webwidget_ ? webwidget_xxx : yyy where needed, where throttled can be
> false.

Sorry, I'm don't understand exactly you're saying here. Could you explain a
little more? (I didn't work at all on this code; I'm only fixing this because
the code analysis tool found this error.)

[nduca, 2012-07-30 17:52:40]

Instead of doing an early return from the function, can you find a way to just
skip the dereference? I suggested using a ternary operator to do so.

[Kyle Horimoto, 2012-07-30 23:25:55]

On 2012/07/30 17:52:40, nduca wrote:
> Instead of doing an early return from the function, can you find a way to just
> skip the dereference? I suggested using a ternary operator to do so.

Sure, this sounds reasonable. In this case, what should be the value of
webwidget_->isInputThrottled() if webwidget_ is NULL?

[nduca, 2012-07-31 00:26:03]

false :)

[Kyle Horimoto, 2012-08-02 20:46:16]

Implemented your suggested change. Can I get a review? Thanks :)

[nduca, 2012-08-02 20:51:53]

https://chromiumcodereview.appspot.com/10830024/diff/2004/content/renderer/re...
File content/renderer/render_widget.cc (right):

https://chromiumcodereview.appspot.com/10830024/diff/2004/content/renderer/re...
content/renderer/render_widget.cc:586: webwidget_ ?
webwidget_->isInputThrottled() : false ||
(  )'s around the ternary clause to make clearer?  LGTM with nit

[Kyle Horimoto, 2012-08-03 18:37:56]

https://chromiumcodereview.appspot.com/10830024/diff/2004/content/renderer/re...
File content/renderer/render_widget.cc (right):

https://chromiumcodereview.appspot.com/10830024/diff/2004/content/renderer/re...
content/renderer/render_widget.cc:586: webwidget_ ?
webwidget_->isInputThrottled() : false ||
On 2012/08/02 20:51:53, nduca wrote:
> (  )'s around the ternary clause to make clearer?  LGTM with nit

Done.

[commit-bot: I haz the power, 2012-08-03 18:38:10]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/khorimoto@chromium.org/10830024/10003

[commit-bot: I haz the power, 2012-08-03 20:50:23]

Change committed as 149918