Closed Bug 1223946 Opened 4 years ago Closed 4 years ago

In Fennec with C++APZ enabled, wheel events dispatched from nsDOWWindowUtil cause assert

Categories

(Core :: Panning and Zooming, defect)

Unspecified
Android
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla45
Tracking Status
firefox45 --- fixed

People

(Reporter: rbarker, Assigned: rbarker)

References

Details

Attachments

(2 files, 4 obsolete files)

The base widget will assert that controller thread != main when wheel events are dispatched fro from nsDOMWindowUtil when C++APZ is enabled in Fennec. Wheel events need to be dispatched from the proper thread in Fennec to prevent the assert.
Assignee: nobody → rbarker
Blocks: 1207748
Ideally I'd like to see this patch split in two - the first does the justification change and other cleanup, and the second introduces the new code to dispatch on the new thread.
Comment on attachment 8686749 [details] [diff] [review]
0002-Bug-1223946-Part-2-Ensure-wheel-event-from-nsDOMWindowUtil-is-dispatched-on-correct-thread-15111212-2fc58b5.patch

Review of attachment 8686749 [details] [diff] [review]:
-----------------------------------------------------------------

::: widget/nsBaseWidget.cpp
@@ +1052,5 @@
> +                                 mWheelInput(aWheelInput),
> +                                 mWidget(aWidget),
> +                                 mAPZResult(aAPZResult),
> +                                 mInputBlockId(aInputBlockId),
> +                                 mGuid(aGuid)

Move the initializers over, so this is formatted like:

  ConstructorName(Arg1 arg1,
                  Arg2 arg2)
    : m1(arg1)
    , m2(arg2)
  {
  }

Ditto for the other helper class below

@@ +1106,5 @@
>  nsEventStatus
>  nsBaseWidget::DispatchAPZAwareEvent(WidgetInputEvent* aEvent)
>  {
>    if (mAPZC) {
> +    if (APZThreadUtils::IsControllerThreadOnMainThread()) {

I think this should just be APZThreadUtils::IsControllerThread(). (I'll put a comment on the other patch that explains more)

@@ +1118,5 @@
> +      return ProcessUntransformedAPZEvent(aEvent, guid, inputBlockId, result);
> +    } else {
> +      WidgetWheelEvent* wheelEvent = aEvent->AsWheelEvent();
> +      if (wheelEvent) {
> +        MOZ_ASSERT(NS_IsMainThread());

Move this assert to the start of the function. This function must run on the main thread because it's dealing with a WidgetInputEvent which can only be used on the main thread.
Attachment #8686749 - Flags: review+
Comment on attachment 8686753 [details] [diff] [review]
0001-Bug-1223946-Part-1-Clean-up-and-support-needed-for-converting-WidgetWheelEvent-to-ScrollWheelInput-15111212-d1350e3.patch

Review of attachment 8686753 [details] [diff] [review]:
-----------------------------------------------------------------

::: gfx/layers/apz/util/APZThreadUtils.cpp
@@ +100,5 @@
>  
> +/*static*/ bool
> +APZThreadUtils::IsControllerThreadOnMainThread()
> +{
> +  return (sControllerThread == MessageLoop::current()) && NS_IsMainThread();

This function name implies that you can call it from any thread and it will return true iff the main thread is the same as the controller thread on the current platform. However, that's not what this is doing, since it's also checking that you're calling it from the main thread.

I think this function can be renamed to just be IsControllerThread(), and only do the first check (sControllerThread == MessageLoop::current()). I believe it will need to be ifdef'd for MOZ_ANDROID_APZ as well though, since the controller thread there doesn't have a MessageLoop:current(), we'll need to call AndroidBridge::IsJavaUiThread() instead.
Attachment #8686753 - Flags: review+
Also for posterity I want to say that ideally we should update the tests that are relying on this to use synthesizeNativeWheel instead (and support that function on all the relevant platforms).

Back in https://bugzilla.mozilla.org/show_bug.cgi?id=1126090#c42 dvander wanted to do basically this exact same thing for B2G and I asked him to disable the tests on B2G instead because B2G didn't support wheel events. However on Android the JPZC does support wheel events, so we'll need to add support for that here, and so we should probably keep these tests. There's definitely a better way to do all this though in the APZ world. I'll file a follow-up for that.
https://hg.mozilla.org/mozilla-central/rev/ffa10317f183
https://hg.mozilla.org/mozilla-central/rev/ca5fae7924d2
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla45
You need to log in before you can comment on or make changes to this bug.