1278480 - [Static Analysis][Dereference null return value] In function TouchActionHelper::GetAllowedTouchBehavior

Status: RESOLVED FIXED

(Core :: Graphics, defect)

Description

[Andi [:andi]]

The Static Analysis tool Coverity detected that pointer |view| be assigned a null return value:

>>  nsView *view = nsView::GetViewFor(aWidget);
>>  nsIFrame *viewFrame = view->GetFrame();

|view| should be checked with an assert before being used since in function GetFrame member variable |mFrame| will be return thus producing a null pointer dereference. 

>>  nsView *view = nsView::GetViewFor(aWidget);
>>  MOZ_ASSERT(view, "view should not be null");
>>  nsIFrame *viewFrame = view->GetFrame();

Comment 1

[Andi [:andi]]

Attachment: Bug 1278480 - prevent null pointer dereference.

Review commit: https://reviewboard.mozilla.org/r/58182/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/58182/

Comment 2

[Jeff Muizelaar [:jrmuizel]]

Comment on attachment 8760637 [details]
Bug 1278480 - prevent null pointer dereference.

I think this should either handle nullptr properly or have a comment explaining why it won't get a nullptr.

Comment 3

[Kartikaya Gupta (email:[e-mail])]

Comment on attachment 8760637 [details]
Bug 1278480 - prevent null pointer dereference.

https://reviewboard.mozilla.org/r/58182/#review55068

I think in this case adding a null check would be more correct. I don't think there's any guarantee here that the view will be non-null. You can move the |TouchBehaviorFlags behavior = ...| line near the bottom of the function up to the top and return that as the value if the view is null.

Comment 4

[Andi [:andi]]

Comment on attachment 8760637 [details]
Bug 1278480 - prevent null pointer dereference.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/58182/diff/1-2/

Comment 5

[Kartikaya Gupta (email:[e-mail])]

Comment on attachment 8760637 [details]
Bug 1278480 - prevent null pointer dereference.

https://reviewboard.mozilla.org/r/58182/#review55370

::: gfx/layers/apz/util/TouchActionHelper.cpp:54
(Diff revision 2)
>    nsView *view = nsView::GetViewFor(aWidget);
> +  TouchBehaviorFlags behavior = AllowedTouchBehavior::VERTICAL_PAN | AllowedTouchBehavior::HORIZONTAL_PAN |
> +                                AllowedTouchBehavior::PINCH_ZOOM | AllowedTouchBehavior::DOUBLE_TAP_ZOOM;
> +
> +  if (!view)
> +    return behavior;

Braces around the body of the if statement, please. r+ with that.

Comment 6

[Andi [:andi]]

Comment on attachment 8760637 [details]
Bug 1278480 - prevent null pointer dereference.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/58182/diff/2-3/

Comment 7

[Andi [:andi]]

Comment on attachment 8760637 [details]
Bug 1278480 - prevent null pointer dereference.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/58182/diff/3-4/

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/ec866ebf94bc
prevent null pointer dereference. r=kats

Comment 9

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/ec866ebf94bc