1372233 - Remove "default" branches from switch statements in APZ code

Status: RESOLVED FIXED

(Core :: Panning and Zooming, enhancement, P3)

Description

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

default branches should be used sparingly when switching over enums, because if new entries are added to the enum the handling of that entry could get accidentally skipped. [1] is a specific example.

[1] http://searchfox.org/mozilla-central/rev/d840ebd5858a61dbc1622487c1fab74ecf235e03/gfx/layers/apz/src/AsyncPanZoomController.cpp#1091

Comment 1

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

Attachment: Bug 1372233 - Clean up handling of default branches in APZ switch statements.

Wherever possible, we exhaustively list all the possibilities instead of
having "default" branches. This way if new items are added to the enum
they won't silently end up in the "default" branch, but the compiler can
tell us that we skipped handling it.
Also wherever possible, the existing NS_WARNINGs were upgrade to
MOZ_ASSERTs or equivalent.

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

Comment 2

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

https://treeherder.mozilla.org/#/jobs?repo=try&revision=fbedbd204684167376209eaf202899717b602e8e

Comment 3

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

Comment on attachment 8878067 [details]
Bug 1372233 - Clean up handling of default branches in APZ switch statements.

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

Comment 4

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

That first patch didn't pick up all the hunks I meant to include. Here's a better one:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=e0e0a7d84d0f46ffe151693c7f2e44aec422dbeb

Comment 5

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

Comment on attachment 8878067 [details]
Bug 1372233 - Clean up handling of default branches in APZ switch statements.

I swear this compiled locally!

Comment 6

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

Comment on attachment 8878067 [details]
Bug 1372233 - Clean up handling of default branches in APZ switch statements.

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

Comment 7

[Botond Ballo [:botond]]

Comment on attachment 8878067 [details]
Bug 1372233 - Clean up handling of default branches in APZ switch statements.

https://reviewboard.mozilla.org/r/149486/#review154610

::: gfx/layers/apz/src/AsyncPanZoomController.cpp:1047
(Diff revision 3)
>        case TapGestureInput::TAPGESTURE_CANCEL: rv = OnCancelTap(tapGestureInput); break;
> -      default: NS_WARNING("Unhandled tap gesture"); break;
> +      case TapGestureInput::TAPGESTURE_SENTINEL: MOZ_ASSERT_UNREACHABLE("Invalid value"); break;
>      }
>      break;
>    }
> -  default: NS_WARNING("Unhandled input event"); break;
> +  default: MOZ_ASSERT(false, "Unhandled input event"); break;

Why not MOZ_ASSERT_UNREACHABLE?

::: gfx/layers/apz/src/AsyncPanZoomController.cpp
(Diff revision 3)
>      case PANNING_LOCKED_Y:
>      case PINCHING:
>        NS_WARNING("Received impossible touch in OnTouchStart");
>        break;
> -    default:
> -      NS_WARNING("Unhandled case in OnTouchStart");

One thing to keep in mind is that it *is* possible for a variable of enumeration type with a value outside the range of enumerators to arise. For example, someone could write |PanZoomState(45)|, or an improperly implementated serialization routine could result in garbage being read into an enum value (though that won't happen if we're using ContiguousEnumSerializer).

Previously, such cases would have been caught by this |default| branch.

Admittedly that's a very minor thing compared to the advantage of getting the compiler to complain if we add a new enumerator but forget to update this switch, I just wanted to mention it for completeness. (If we cared, we could replace the breaks with returns, and emit a warning or assertion if we get past the switch.)

::: gfx/layers/apz/src/AsyncPanZoomController.cpp:1730
(Diff revision 3)
>  
>  static ScrollInputMethod
>  ScrollInputMethodForWheelDeltaType(ScrollWheelInput::ScrollDeltaType aDeltaType)
>  {
>    switch (aDeltaType) {
> +    case ScrollWheelInput::SCROLLDELTA_SENTINEL:

Is there a reason to prefer this over having the case at the bottom and returning ApzWheelLine? I find it more readable to leave the error cases at the bottom.

::: gfx/layers/apz/src/TouchCounter.cpp:39
(Diff revision 3)
>        break;
>      case MultiTouchInput::MULTITOUCH_CANCEL:
>        mActiveTouchCount = 0;
>        break;
> -    default:
> +    case MultiTouchInput::MULTITOUCH_SENTINEL:
> +      MOZ_FALLTHROUGH_ASSERT("Invalid input");

Same here.

::: gfx/layers/apz/src/WheelScrollAnimation.cpp:105
(Diff revision 3)
>    case ScrollWheelInput::SCROLLDELTA_PIXEL:
>      mOriginMaxMS = clamped(gfxPrefs::PixelSmoothScrollMaxDurationMs(), 0, 10000);
>      mOriginMinMS = clamped(gfxPrefs::PixelSmoothScrollMinDurationMs(), 0, mOriginMaxMS);
>      break;
> +  case ScrollWheelInput::SCROLLDELTA_SENTINEL:
> +    MOZ_FALLTHROUGH_ASSERT("Invalid value");

(Here I can see the motivation: we're reusing a nontrivial amount of code between a success case and the error case.)

Comment 8

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

(In reply to Botond Ballo [:botond] from comment #7)
> > -  default: NS_WARNING("Unhandled input event"); break;
> > +  default: MOZ_ASSERT(false, "Unhandled input event"); break;
> 
> Why not MOZ_ASSERT_UNREACHABLE?

I had used MOZ_ASSERT everywhere in an earlier version and I guess I missed this when retrofitting MOZ_ASSERT_UNREACHABLE. Fixed.

> > -    default:
> > -      NS_WARNING("Unhandled case in OnTouchStart");
> 
> One thing to keep in mind is that it *is* possible for a variable of
> enumeration type with a value outside the range of enumerators to arise. For
> example, someone could write |PanZoomState(45)|, or an improperly
> implementated serialization routine could result in garbage being read into
> an enum value (though that won't happen if we're using
> ContiguousEnumSerializer).
> 
> Previously, such cases would have been caught by this |default| branch.
> 
> Admittedly that's a very minor thing compared to the advantage of getting
> the compiler to complain if we add a new enumerator but forget to update
> this switch, I just wanted to mention it for completeness. (If we cared, we
> could replace the breaks with returns, and emit a warning or assertion if we
> get past the switch.)

You're right, but I don't think I care enough about this one :)

> ::: gfx/layers/apz/src/AsyncPanZoomController.cpp:1730
> > +    case ScrollWheelInput::SCROLLDELTA_SENTINEL:
> 
> Is there a reason to prefer this over having the case at the bottom and
> returning ApzWheelLine? I find it more readable to leave the error cases at
> the bottom.

Yeah, I do too. Again, an earlier version of the patch didn't have anything past the end of the switch so this felt better but now I've changed it to move that case to the bottom and just fall out into the error at the bottom.

> ::: gfx/layers/apz/src/TouchCounter.cpp:39
> > +    case MultiTouchInput::MULTITOUCH_SENTINEL:
> > +      MOZ_FALLTHROUGH_ASSERT("Invalid input");
> 
> Same here.

Also fixed.

> ::: gfx/layers/apz/src/WheelScrollAnimation.cpp:105
> > +  case ScrollWheelInput::SCROLLDELTA_SENTINEL:
> > +    MOZ_FALLTHROUGH_ASSERT("Invalid value");
> 
> (Here I can see the motivation: we're reusing a nontrivial amount of code
> between a success case and the error case.)

I left this one as-is.

Comment 9

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

Comment on attachment 8878067 [details]
Bug 1372233 - Clean up handling of default branches in APZ switch statements.

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

Comment 10

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

Another try push with the updates to make sure I didn't anger the compiler gods: https://treeherder.mozilla.org/#/jobs?repo=try&revision=72c2a17f680991a42685d72a1714b7ca2fcd79ef

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/f22f7f42d2d4
Clean up handling of default branches in APZ switch statements. r=botond

Comment 12

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

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

Comment 13

[Botond Ballo [:botond]]

FWIW this introduced a (non-fatal) warning on local clang 3.8 builds:

 0:13.97 In file included from /home/botond/dev/mozilla/out-of-tree-objdirs/refactoring-desktop-clang-opt/gfx/layers/Unified_cpp_gfx_layers2.cpp:101:0:
 0:13.97 /home/botond/dev/mozilla/refactoring/gfx/layers/apz/util/APZCCallbackHelper.cpp: In static member function 'static void mozilla::layers::APZCCallbackHelper::NotifyPinchGesture(mozilla::PinchGestureInput::PinchGestureType, mozilla::LayoutDeviceCoord, mozilla::Modifiers, nsIWidget*)':
 0:13.97 /home/botond/dev/mozilla/refactoring/gfx/layers/apz/util/APZCCallbackHelper.cpp:973:16: warning: 'msg' may be used uninitialized in this function [-Wmaybe-uninitialized]