Bug 930939 (input-thread)

Separate the threads that run gecko and that receive input events from the OS

RESOLVED FIXED in Firefox 38, Firefox OS v2.2

Status

()

Core
Widget: Gonk
P1
normal
RESOLVED FIXED
4 years ago
2 years ago

People

(Reporter: kats, Assigned: kats)

Tracking

({perf})

unspecified
mozilla38
All
Gonk (Firefox OS)
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(feature-b2g:2.2+, tracking-b2g:+, firefox38 fixed, b2g-v2.2 fixed, b2g-master fixed)

Details

(Whiteboard: [c=effect p=5 s= u=][input-thread-uplift-part7])

Attachments

(8 attachments, 43 obsolete attachments)

420.05 KB, image/png
Details
14.21 KB, image/svg+xml
Details
13.79 KB, image/svg+xml
Details
1.20 MB, video/quicktime
Details
36.72 KB, patch
kats
: review+
Details | Diff | Splinter Review
3.05 KB, text/plain
Details
5.17 KB, patch
kats
: review+
Details | Diff | Splinter Review
16.22 KB, patch
mwu
: review+
botond
: review+
Details | Diff | Splinter Review
One of the goals for future FxOS versions is to improve the responsiveness so that the latency between a user touching the screen and a visual effect happening on the screen is 120ms or less [1].

Currently, the fastest codepath we can do this on has input events getting received from the OS on the gecko thread (which may be busy doing things like running JS or GC'ing), sending that input event through gecko for hit testing, then sending it to the AsyncPanZoomController and having it update some state. At the next frame composite, the compositor thread would pick up the updated transform from the APZC and show it on the screen. This path involves (1) waiting for gecko, (2) hit testing in gecko, (3) more hit testing in APZC, and (4) transfer of state from the gecko thread to the compositor thread.

I had previously filed bug 920036 to move (2) off this codepath, but I think we can get rid of (1) as well by moving gecko off the process main thread. The process main thread should be used strictly for dispatching the events received from the OS to the right thread (either the gecko thread or the compositor thread) as fast as possible. Fennec already uses this model, because the gecko thread is different from the Android UI main thread where events are received, and motion events can result in compositor updates without going through gecko.

Note that one option that would get rid of (4) is to run the compositor on the process main thread. However on B2G the compositor blocks on vsync, so if the compositor is blocked there it will hold up processing of other events being received by the OS. On platforms where the compositor never blocks it might be possible to do this but that's outside the scope of this bug.

Another constraint is that for scenarios like full-screen games, we want touch events delivered to content JS with the same (or even lower) latency. For this case it would be counterproductive to always routing the events through the compositor for this; it would be better if the input-handling thread had sufficient information to send the input directly to the content process so that it could be dispatched to JS there quicker.

[1] https://etherpad.mozilla.org/graphics-work-week (in case this is gone, this is coming from the FxOS UX team)
You're talking specifically about touch events that trigger panning and zooming in APZC, right? Not, say, touching a button? That's implied by your comment but not stated.

On B2G, couldn't we modify Gonk to receive touch events on any thread we want?
According to vlad getting touch events to content has an even tighter responsiveness requirement because of games. This means we want both touch events for panning/zooming in APZC and touch events for buttons to be fast.

And yes, I assume we can modify Gonk to receive touch events on any thread, but I don't think that any of existing threads we have on B2G's current architecture are good candidates because they all result in trading off latency to APZC vs latency to content. That's why I'm suggesting a new thread/architecture for this purpose.
Created attachment 827404 [details]
Current event flow on B2G

For the record, this is what the current input event flow looks like on B2G. I drew this up on the flight back from the rendering work week and meant to blog about it but in case I end up not doing that I still want people to see it.
(The red lines are cross-process messaging and the yellow lines are cross-thread)
Very nice. Which thread is the are all the first steps done in? The b2g process' main thread? That would mean events waits for the status bar to paint before we can dispatch them to content processes.
Yeah, the first steps are all on the b2g process main thread, which runs gecko.
Nice diagram.

It's not clear to me how you plan to avoid going through the compositor thread. I suppose once we have touch regions hooked up (I have a patch for computing the regions now) the compositor could, on every frame, push to some event-handling thread a map of regions indicating for each point, which thread a touchstart event for that point should go to (the compositor thread, or the content thread of some process). But it might be difficult to dispatch events to the content thread of some subprocess without going through the content thread of the b2g process due to synchronization issues.

Updated

4 years ago
Keywords: perf
Whiteboard: [c=effect p= s= u=]

Updated

4 years ago
Blocks: 944564

Comment 8

4 years ago
Just would like to know if this bug would be helpful for mine.
https://bugzilla.mozilla.org/show_bug.cgi?id=944564#c0 (2nd problem)

The current input event flow of swiping Homescreen on B2G looks like ...
(process/thread, -> cross-thread, --> cross-process)
b2g/InputReader -> b2g/b2g --> content/Homescreen --> b2g/Compositor

I noticed that there're 3 problems make transmission of input event blocked.
b2g/b2g costs ViewportFrame::BuildDisplayList() for touch_start.
b2g/b2g is doing GC.
content/Homescreen is re-flowing or painting layer. (BenWa has implemented pre-render to prevent from painting for homescreen, but other apps may block receiving input event for some reasons.)

So to overcome these blocks, are you going to move all gecko stuff off the main thread ?
I'm also concerned about synchronization problem.
This bug should in theory get rid of the b2g/b2g step in your flow, which will help alleviate some of the problems. However since we don't have a concrete plan yet for how to organize the input event flow I can't say that for certain.

For the record, I would like come up with a modified version of attachment 827404 [details] and have everybody sign off on it before any code changes are made. If anybody has the time and is willing to try doing that please feel free. (It's a hand-written SVG so you you can even just grab it and edit it manually).

Comment 10

4 years ago
Does GeckoInputDispatcher::dispatchOnce() have to be called in main thread for some reasons now ? Or why not just simply take advantage of InputManager class in libui ? Then there will be a dedicated "InputDispatcher" thread for running it.
The current implementation of GeckoInputDispatcher::dispatchOnce() invokes nsWindow::DispatchInputEvent which delivers the event to gecko so it must run on the gecko thread. This bug is about changing that. using the InputManager class in libui is one possible option, yes.
Depends on: 920036
See Also: → bug 911133
Depends on: 988858

Updated

4 years ago
Priority: -- → P3
Depends on: 1014189
Whiteboard: [c=effect p= s= u=] → [c=effect p= s= u=] [TCP]
Blocks: 987532
Created attachment 8435014 [details]
Update event flow on B2G with edge-gestures implemented

Yay more complicated event flow!
Attachment #827404 - Attachment is obsolete: true
Assignee: nobody → mchang
Priority: P3 → P1
Whiteboard: [c=effect p= s= u=] [TCP] → [c=effect p=5 s= u=] [TCP]
Status: NEW → ASSIGNED
Created attachment 8435911 [details]
Touch start flow

Hey Kats, here's a first stab of the overall flow for what happens on a touch start only in the new architecture where we process the input events off main thread. Any comments or is this accurate from what we discussed? Thanks!
Flags: needinfo?(bugmail.mozilla)
Hey, sorry for the delay. I got swamped with some tricky bugs that I wanted to get out of the way first. So the main problem in your flow is that the "Dispatch touch even to main thread" happens before the APZ untransform, but it needs to happen after. Also after the event goes through the main process main thread it needs to go through the child process steps as well.

I like the way you used the colors in your diagram, it helps with understanding what happens where. I might update my SVG to do that as well so maybe things will be clearer. Also I think the "Gecko untransform" box is obsolete now and can probably be removed. At least I don't remember what that is anymore.
Flags: needinfo?(bugmail.mozilla)
(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #14)
> Also I think the "Gecko untransform" box is
> obsolete now and can probably be removed. At least I don't remember what
> that is anymore.

It sounds like the untransformation done in TabParent::MapEventCoordinatesForChildProcess to get the events into the child process' coordinate system, although that's done in the APZ case as well.
Created attachment 8437066 [details]
Existing event flow (v3)
Attachment #8435014 - Attachment is obsolete: true
Created attachment 8437068 [details]
Desired event flow (v1)
(In reply to Botond Ballo [:botond] from comment #15)
> It sounds like the untransformation done in
> TabParent::MapEventCoordinatesForChildProcess to get the events into the
> child process' coordinate system, although that's done in the APZ case as
> well.

Yeah that sounds right. I put it on both paths in the updated flow.
Depends on: 1027263
Created attachment 8443167 [details] [diff] [review]
WIP: Create GeckoTouchDispatcher and convert gonk input to APZ Input

Updated

4 years ago
See Also: → bug 913942
Created attachment 8443824 [details] [diff] [review]
WIP: Send TouchEvents to APZ on input thread on TabParent::Capture path

Just to test that I was sending the right events, on TabParent::Capture paths, we directly send the touch events to the APZC controller. It actually all works and we can scroll! It completely crashes on non-scroll events such as a tap, but one step at a time.

I also flashed two flames, one with and one without this patch. There is a very subtle but nice noticeable improvement in scrolling responsiveness! The other fallout is that we see low res tiles a lot more often, probably because I'm not correctly notifying Gecko to paint.

Either way, I think this patch at least translates touch input scroll events correctly.
Attachment #8443167 - Attachment is obsolete: true
Attachment #8443824 - Attachment description: WIP: Send TouchEvents to APZ on TabCapture parent → WIP: Send TouchEvents to APZ on input thread on TabParent::Capture path
Comment on attachment 8443824 [details] [diff] [review]
WIP: Send TouchEvents to APZ on input thread on TabParent::Capture path

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

::: gfx/layers/apz/src/APZCTreeManager.h
@@ +129,5 @@
>    nsEventStatus ReceiveInputEvent(const InputData& aEvent,
>                                    ScrollableLayerGuid* aOutTargetGuid);
>  
> +  nsEventStatus ReceiveInputEventOnInputThread(const MultiTouchInput& aMultiTouchInput,
> +                                  ScrollableLayerGuid* aOutTargetGuid);

You don't need this function. The ReceiveInputEvent(const InputData& aEvent, ScrollableLayerGuid* aOutTargetGuid) is meant for exactly this purpose.
Created attachment 8444504 [details] [diff] [review]
WIP: Send Touch Start/End on Main thread, Moves on input thread

Changing the touch start/ends to be on the main thread and the touch moves on the input thread makes tapping work again. 

(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #21)
> Comment on attachment 8443824 [details] [diff] [review]
> You don't need this function. The ReceiveInputEvent(const InputData& aEvent,
> ScrollableLayerGuid* aOutTargetGuid) is meant for exactly this purpose.

Yeah, I thought I needed to change more things so I was playing with the code. Looks like I probably won't need this function for now.
Attachment #8443824 - Attachment is obsolete: true
(In reply to Mason Chang [:mchang] from comment #22)
> Created attachment 8444504 [details] [diff] [review]
> WIP: Send Touch Start/End on Main thread, Moves on input thread
> 
> Changing the touch start/ends to be on the main thread and the touch moves
> on the input thread makes tapping work again. 
> 

Except we still want to deal with touch start/ends on the input thread, not the main thread. The likely culprit for tapping breaking is that when the APZ code tries to schedule a delayed task (by invoking [1]) then it doesn't work because MessageLoop::current is the input thread's message loop, or something.

[1] http://mxr.mozilla.org/mozilla-central/source/layout/ipc/RenderFrameParent.cpp?rev=5da74b54a472#642
(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #23)
> (In reply to Mason Chang [:mchang] from comment #22)
> > Created attachment 8444504 [details] [diff] [review]
> > WIP: Send Touch Start/End on Main thread, Moves on input thread
> > 
> > Changing the touch start/ends to be on the main thread and the touch moves
> > on the input thread makes tapping work again. 
> > 
> 
> Except we still want to deal with touch start/ends on the input thread, not
> the main thread. The likely culprit for tapping breaking is that when the
> APZ code tries to schedule a delayed task (by invoking [1]) then it doesn't
> work because MessageLoop::current is the input thread's message loop, or
> something.
> 
> [1]
> http://mxr.mozilla.org/mozilla-central/source/layout/ipc/RenderFrameParent.
> cpp?rev=5da74b54a472#642

Thanks for the hint on where it is breaking. Yeah I still want to move all touch events to the input thread, was just seeing why it broke so I have a grounding in the code. Just tracing through the code some more :)
Created attachment 8444933 [details] [diff] [review]
WIP: Touch events on input thread

Seems to work sometimes, at least for the vertical home screen. When I load an app, the app crashes when I scroll. The Input thread's message loop always seems to crash when we post an event to it, so I have to investigate and see if the input thread even has a message loop.

Another interesting thing, once I move my finger, there is quite a bit of delay until we start scrolling, more so than without this patch. I think it's because we're processing touch events but waiting for the GeckoContent to callback saying or to create scrollable layers, which I think occurs on the child main thread. Still have to investigate.
Attachment #8444504 - Attachment is obsolete: true
Depends on: 1024595
(In reply to Mason Chang [:mchang] from comment #25)
> Another interesting thing, once I move my finger, there is quite a bit of
> delay until we start scrolling, more so than without this patch. I think
> it's because we're processing touch events but waiting for the GeckoContent
> to callback saying or to create scrollable layers, which I think occurs on
> the child main thread. Still have to investigate.

You can verify this by looking at the layer dumps before and after. It might also be that the APZ code is having to wait longer before it hears back from content about touch listeners via the ContentReceivedTouch callback.
See Also: → bug 966172
Created attachment 8446256 [details] [diff] [review]
WIP: Touch Events on Input Thread

Ugly ugly hack. The android input thread doesn't have it's own message loop, so I added one since APZ needs to post tasks. Unfortunately, the input thread is always asleep unless an input event comes in, so the delayed tasks don't run. This is an ugly hack to wake the thread up every 16 ms (to align with vsync) so that our delayed tasks do run.

In the future, we'll run APZ on the vsync thread which should be a full nsThread with a MessageLoop.

Things that probably break: any gestures (long taps, etc), multi touch, anything other than scrolling. But pure scroll events are now on the input thread.
Attachment #8444933 - Attachment is obsolete: true
Best validation that this works. I went to nytimes.com, which loads to the desktop site. I tried with two flames, one with and one without this patch. nytimes.com runs a video and lots of JS. I could barely scroll on the flame off tip without this patch. I could scroll mostly pretty well (and I'm sure it's because I broke touch start in some cases) with this patch. hoorah!
Depends on: 1030941
Created attachment 8447494 [details]
Patch on a flame on nytimes.com
Created attachment 8447495 [details]
m-c on a nexus 4 on nytimes.com
Depends on: 1031601
Depends on: 987527
Depends on: 987529
Created attachment 8450596 [details] [diff] [review]
WIP: Touch Events on Input Thread

Updated WIP. This depends on the vsync thread from bug 987527, and a few tweaks to that patch that will be removed later. We also have an InputThreadLoop, which should be removed once 987529 lands and we have a vsync thread with a MessageLoop.

Currently, we get touch events on the input thread and put them in a queue. Every vsync, we notify the main thread and process the touch events through APZ on the vsync thread. Most of the changes involved in APZC are making sure we update the transformToGecko in APZCTreeManager::ReceiveInputEvent and in the RenderFrameParent/GeckoContentController to make sure we post the callbacks on the input thread rather than the main thread. The guiding assumption is callbacks which directly call TabParent/TabChild go on the main thread, all other callbacks should go on the input thread. This is because TabParent/TabChild adjust things based on the DOM which can only be accessed on the main thread. Sending IPDL messages on different threads but on the same channel results in sporadic crashes. 

It all seems to work, including edge gestures. It's still quite dirty and I will clean it up after some more testing.
Attachment #8446256 - Attachment is obsolete: true
Depends on: 1035356
Created attachment 8452021 [details] [diff] [review]
WIP: Touch Events on Input Thread

Cleaned up a bit more and we also send key events correctly now.
Attachment #8450596 - Attachment is obsolete: true
Created attachment 8454115 [details] [diff] [review]
WIP: Touch Events on vsync Thread

WIP to get touch events on the vsync thread. This fixes issues with overscroll animation getting stuck and the overflow APZC hit test (http://people.mozilla.org/~kgupta/tmp/overflow.html#). Currently, after APZC processes the touch event, we send a clone of the original touch data through the main thread as a WidgeTouchEvent. When TabParent::MaybeForwardToRenderFrame comes around, we update the touch event with the Gecko Untransform.

A few things until this patch should hopefully be ready for review:
1) We need the full vsync thread / dispatcher in bug 987529. Jerry is working on that.
2) Cleaned up in a few places with a lot fewer printfs and removal of the InputThreadLoop. The Vsync thread is a chromium::Thread and will have a MessageLoop.
3) One thing I'm concerned about is that we process touch events on the Vsync thread, then dispatch the same touch event through the main thread. When that same touch event comes around to the APZCTreeManager::ReceiveInputEvent, we currently check if we have an mApzcForInputBlock and return a status based on mInOverscrolledApzc, which might be sending different results to TabParent for the same touch. For example, if we get a TOUCH_END on the vsync thread before the parent process sees the TOUCH_END on the main thread, we already set mApzcForInputBlock to null. This hasn't seemed to be a problem in practice, but maybe we should feed the result of ReceiveInputThread on from the input thread all the way through Gecko?

Kats, any thoughts on #3? Thanks!
Attachment #8452021 - Attachment is obsolete: true
Flags: needinfo?(bugmail.mozilla)
(In reply to Mason Chang [:mchang] from comment #33)
> maybe we should
> feed the result of ReceiveInputThread on from the input thread all the way
> through Gecko?

Yes, exactly. In bug 1035356 which I landed recently the ReceiveInputEvent(InputData) function now applies the gecko untransform as well. So if you create the WidgetInputEvent clone from the InputData *after* calling ReceiveInputEvent, get the gecko-untransformed version, and you can just send that through gecko. That way you won't need to send the input event again to the APZC in MaybeForwardToRenderFrame at all. Does that make sense?
Flags: needinfo?(bugmail.mozilla)
(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #34)
> calling ReceiveInputEvent, get the gecko-untransformed version, and you can

s/get/you will get/
Created attachment 8455852 [details] [diff] [review]
WIP: Touch Events on Vsync Thread

Rebased with the vsync work from Jerry. I was able to delete the InputThreadLoop. All 3 things from Comment 33 are done. At the moment, after APZ Processes the event, we store both the original touch data and the APZ untransformed data and put it into a WidgetAPZTouchEvent, which carries both touch sets. We pass that through Gecko with the original touch data, then at TabParent, send the APZ untransformed data through TabChild. It's a lot of data to carry around through the pipeline, not sure how I like it as a solution.

Another side bug I found was while having some overlays, APZ sees the data as well as the overlay, so we get something like two events. For example, in the Settings app, the ADB and devtools setting + the developer hud button overlay each other. When choosing one option, the touch event gets sent to both the button overlay + the developer hud.
Attachment #8454115 - Attachment is obsolete: true
Created attachment 8456590 [details] [diff] [review]
WIP: Touch Events on Vsync Thread

Updated to return true/false if we processed an input event on a vsync. This helps Jerry do some potential optimizations. Otherwise it's the same.
Attachment #8455852 - Attachment is obsolete: true
Depends on: 1021413
No longer depends on: 1021413
Depends on: 973096
Created attachment 8458990 [details] [diff] [review]
WIP: Touch Events on Vsync Thread

Updated to compile on desktop linux / os x platforms. Cleaned up to use the new WIP from Jerry's vsync bug. After bug 973096 lands, I don't see any other major bugs. I will test and clean up, then start asking for feedback. Reviews will come after Jerry's patches land as we need the vsync thread there.
Attachment #8456590 - Attachment is obsolete: true
Whiteboard: [c=effect p=5 s= u=] [TCP] → [c=effect p=5 s= u=]
Created attachment 8460281 [details] [diff] [review]
Part 1: Modify APZ to process events on vsync thread
Attachment #8447494 - Attachment is obsolete: true
Attachment #8447495 - Attachment is obsolete: true
Attachment #8458990 - Attachment is obsolete: true
Attachment #8460281 - Flags: feedback?(bugmail.mozilla)
Created attachment 8460282 [details] [diff] [review]
Part 2: Add WidgetAPZTouchEvent to hold APZ untransformed touches
Attachment #8460282 - Flags: feedback?(bugmail.mozilla)
Created attachment 8460283 [details] [diff] [review]
Part 3: Enable conversions to / from MultiTouchEvent - WidgetTouchEvent
Attachment #8460283 - Flags: feedback?(bugmail.mozilla)
Created attachment 8460286 [details] [diff] [review]
Part 4: Add Gecko Touch Dispatcher

After talking with :mwu, we want to go from nsAppShell -> Gecko Touch Dispatcher -> APZ. The Gecko Touch Dispatcher mostly converts android touch data -> MultiTouchInput and is the glue between gonk / APZ.
Created attachment 8460287 [details] [diff] [review]
Part 5: Dispatch Touch Event from gonk to APZ
Attachment #8460287 - Flags: feedback?(bugmail.mozilla)
Created attachment 8460291 [details] [diff] [review]
Part 6: Hook Input Dispatcher into Vsync Dispatcher

Just some changes required to hook into vsync from bug 987529. No need to review this, this part is just some glue work.
Attachment #8460286 - Flags: feedback?(bugmail.mozilla)
Comment on attachment 8460281 [details] [diff] [review]
Part 1: Modify APZ to process events on vsync thread

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

For future patches, please make sure you include more lines of context (usually at least 8) so that it's easier to see where your changes.

Also, in general you should try to order the patches so that each patch only depends on previous ones in the sequence. This patch won't even compile by itself.

::: dom/ipc/TabParent.cpp
@@ +956,5 @@
> +bool
> +TabParent::SendWidgetTouchEvent(WidgetTouchEvent& aEvent)
> +{
> +  // Might not be true, probably still need for other platforms?
> +  NS_RUNTIMEABORT("Shouldn't get here anymore");

This code might be needed for e10s-enabled platforms without APZ. I would remove the runtimeabort.

@@ +981,5 @@
> +    return false;
> +  }
> +
> +  WidgetTouchEvent apzProcessedTouch(true, aApzTouchEvent->message, nullptr);
> +  aApzTouchEvent->ReplaceWithAPZTransforms(apzProcessedTouch);

ReplaceWithAPZTransforms is a very confusing name. Without looking at a future patch on this bug it's very hard for me to tell what this is doing.

@@ +1070,5 @@
>  
> +  const WidgetAPZTouchEvent* apzEvent = aEvent.AsAPZTouchEvent();
> +  if (apzEvent) {
> +    WidgetAPZTouchEvent apzTouch(*apzEvent);
> +    return SendRealTouchEvent(apzTouch);

Does this work? You're passing a WidgetAPZTouchEvent to a function that takes a WidgetTouchEvent&. Even if it does work, what's the point of doing this? Why can't you just leave it as always passing |event|?

::: gfx/layers/apz/src/APZCTreeManager.cpp
@@ +659,4 @@
>  
>    switch (aEvent.eventStructType) {
>      case NS_TOUCH_EVENT: {
> +      NS_RUNTIMEABORT("WE SHOULDNT GET HERE ANYMORE");

Leave this function as-is. It's still used on Metro.

::: layout/ipc/RenderFrameParent.cpp
@@ +668,5 @@
>    virtual void PostDelayedTask(Task* aTask, int aDelayMs) MOZ_OVERRIDE
>    {
> +    if (NS_IsMainThread()) {
> +      MessageLoop::current()->PostDelayedTask(FROM_HERE, aTask, aDelayMs);
> +      NS_RUNTIMEABORT("SHOULD NOT BE ON MAIN THREAD\n");

Remove.

@@ +670,5 @@
> +    if (NS_IsMainThread()) {
> +      MessageLoop::current()->PostDelayedTask(FROM_HERE, aTask, aDelayMs);
> +      NS_RUNTIMEABORT("SHOULD NOT BE ON MAIN THREAD\n");
> +    } else if (MessageLoop::current()) {
> +      // Probably the Compositor thread

Remove this comment, since it's not actually the compositor thread that will be running here.

@@ +1172,5 @@
>      return;
>    }
> +
> +  #ifdef MOZ_WIDGET_GONK
> +  mContentController->RequestContentReceivedTouch(aGuid, aPreventDefault);

This seems unnecessarily convoluted. All you should need to do is, at the top of RenderFrameParent::ContentReceivedTouch, add this:

#ifdef MOZ_WIDGET_GONK
  // For gonk this needs to run on the vsync thread, so re-dispatch to there.
  if (NS_IsMainThread()) {
    GonkVsyncDispatcher::GetInstance()->GetMessageLoop()->PostDelayedTask(FROM_HERE, NewRunnableMethod(this, &RenderFrameParent::ContentReceivedTouch, aGuid, aPreventDefault), 0);
  }
#endif
Attachment #8460281 - Flags: feedback?(bugmail.mozilla)
Comment on attachment 8460282 [details] [diff] [review]
Part 2: Add WidgetAPZTouchEvent to hold APZ untransformed touches

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

This patch is incomplete and missing changes to InputData.cpp. This set of changes (adding WidgetAPZTouchEvent and conversion stuff) should be patch #1

::: widget/TouchEvents.h
@@ +214,5 @@
>  
> +/******************************************************************************
> + * mozilla::WidgetAPZTouchEvent
> + * For touch events already processed by APZ. Contains two sets of transforms
> + * Those from the InputReader in Gonk and those already processed by APZ

This comment doesn't seem to match the code. I don't see anything that looks like "transforms" stored in the class, and there is only one TouchArray.
Attachment #8460282 - Flags: feedback?(bugmail.mozilla)
Comment on attachment 8460283 [details] [diff] [review]
Part 3: Enable conversions to / from MultiTouchEvent - WidgetTouchEvent

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

::: widget/xpwidgets/InputData.cpp
@@ +123,5 @@
> +
> +void
> +MultiTouchInput::ConvertToWidgetTouchEvent(WidgetTouchEvent& aWidgetTouchEvent) const
> +{
> +  uint32_t touchType;

This should probably assert it's on the main thread. Ditto for the other functions
Attachment #8460283 - Flags: feedback?(bugmail.mozilla)
Comment on attachment 8460286 [details] [diff] [review]
Part 4: Add Gecko Touch Dispatcher

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

::: widget/gonk/GeckoTouchDispatcher.cpp
@@ +226,5 @@
> +                                    aApzStatus, aGuid);
> +    aGeckoTouch.ConvertToWidgetAPZTouchEvent(touchEvent, aApzTouch);
> +    nsEventStatus status = nsWindow::DispatchInputEvent(touchEvent, &wasCaptured);
> +
> +    if (!wasCaptured) {

I think we'll have to get rid of the capture code path because it will break the contract about APZ receiving content responses. That is, for every touch block the APZ code receives, it MUST receive exactly one call to ContentReceivedTouch(). The capture code path means that the APZ is still getting input events but then they are captured by the root process and so it never receives the corresponding ContentReceivedTouch() calls.

Actually I'm not sure even if that is enough, because the root process might do a preventDefault on the event regardless of the capture code path, and we would have to notify the APZ of that.
Attachment #8460286 - Flags: feedback?(bugmail.mozilla)
Comment on attachment 8460287 [details] [diff] [review]
Part 5: Dispatch Touch Event from gonk to APZ

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

I mostly don't understand this patch. There's a lot of unrelated whitespace/formatting changes mixed together it looks like. Those types of non-functional changes should be in their own patch. There's also a bunch of vsync stuff in here that I'm not sure about, I think the only part I care about is the nsWindow::DispatchInputEventToAPZ function.
Attachment #8460287 - Flags: feedback?(bugmail.mozilla)
Overall I think we're heading in the right direction. The only "big" concern I had was ensuring that any time the APZ code processes events (i.e. APZCTreeManager::ReceiveInputEvent returns nsEventStatus_eConsumeDoDefault) that we make sure that touch block gets a ContentReceivedResponse(). To be fair that's a requirement from bug 1009733 which I landed pretty recently so it didn't cross my mind before that or I would have brought it up sooner. If we can sort that out (which shouldn't be too hard, I think) and clean up the patches I think we're good.
I just had a discussion with kats regarding his comments. Overall:

> Does this work? You're passing a WidgetAPZTouchEvent to a function that takes a WidgetTouchEvent&. Even 
> if it does work, what's the point of doing this? Why can't you just leave it as always passing |event|?

We are given a WidgetGUIEvent and convert it to a WidgetTouchEvent. However, the WidgetAPZTouchEvent contains two sets of touch events, so converting it to a WidgetTouchEvent will lose one set of touch events. Thus, we'll change SendRealTouchEvent to take a WidgetTouchEvent*.

> This seems unnecessarily convoluted. All you should need to do is, at the top of 
> RenderFrameParent::ContentReceivedTouch, add this:

RenderFrameParent is not ref counted so we cannot create tasks and post to MessageLoops. We can get around this by moving the functionality out of RenderFrameParent and into the GeckoContentController.

> This comment doesn't seem to match the code. I don't see anything that looks like "transforms" stored in 
> the class, and there is only one TouchArray.

Because WidgetAPZTouchEvent inherits from WidgetTouchEvent, we have two touch arrays. One defined in WidgetTouchEvent and one defined in WidgetAPZTouchEvent.

> I think we'll have to get rid of the capture code path because it will break
> the contract about APZ receiving content responses. That is, for every touch
> block the APZ code receives, it MUST receive exactly one call to
> ContentReceivedTouch(). The capture code path means that the APZ is still
> getting input events but then they are captured by the root process and so
> it never receives the corresponding ContentReceivedTouch() calls.

For now we will keep the capture path for two reasons. One is that even on the capture path, we send the touch event to the content so we should still receive the ContentReceivedTouch. The second reason is that currently, if we do not capture a touch event, we also send a mouse event in addition to the touch event. This is probably some optimization. For now, we will keep the capture path but we would like to rip it out after the parent process has APZ enabled.


> I mostly don't understand this patch. There's a lot of unrelated
> whitespace/formatting changes mixed together it looks like. Those types of
> non-functional changes should be in their own patch. There's also a bunch of
> vsync stuff in here that I'm not sure about, I think the only part I care
> about is the nsWindow::DispatchInputEventToAPZ function.

I will clean this patch up. Most of the work to extract out a GeckoTouchDispatcher will be done in bug 970751.
Blocks: 1045755
Created attachment 8464371 [details] [diff] [review]
Part 1: Add WidgetAPZTouchEvent and methods to convert to/from MultiTouchInput

This part enables us to have the WidgetAPZTouchEvent, which contains two sets of touch events. One from the original gonk touch data and one after APZ has already transformed the touch events. It also enables support to transition between MultiTouchInput and WidgetAPZTouchEvent.
Attachment #8460281 - Attachment is obsolete: true
Attachment #8460282 - Attachment is obsolete: true
Attachment #8460283 - Attachment is obsolete: true
Attachment #8460286 - Attachment is obsolete: true
Attachment #8460287 - Attachment is obsolete: true
Attachment #8460291 - Attachment is obsolete: true
Attachment #8464371 - Flags: feedback?(bugmail.mozilla)
Created attachment 8464372 [details] [diff] [review]
Part 2: Enable nsWindow to dispatch input event to APZ

This portion sets up the code so that we have a reference in nsWindow to the APZCTreeManager where we can send touch events from gonk to APZ.
Attachment #8464372 - Flags: feedback?(bugmail.mozilla)
Created attachment 8464373 [details] [diff] [review]
Part 3: Modify TabParent to send APZ transformed touch events to content

This portion changes TabParent to not send touch events to APZ and instead, use the WidgetAPZTouchEvent data to directly send both the return value from APZ and the transformed touch data to the content.
Attachment #8464373 - Flags: feedback?(bugmail.mozilla)
Created attachment 8464374 [details] [diff] [review]
Part 4: Modify GeckoTouchDispatcher to listen to Vsync

Nothing to review here for now.
Created attachment 8464375 [details] [diff] [review]
Part 5: Dispatch Touch events from gonk to APZ

Modified the GeckoTouchDispatcher to convert gonk touch data into MultiTouchInput data. Then we send the touch event through to APZ and redispatch the touch event to the main thread on the parent process. There are some printfs that I'm using to debug because I think there is bug in APZ here:

http://dxr.mozilla.org/mozilla-central/source/gfx/layers/apz/src/AsyncPanZoomController.cpp?from=AsyncPanZoomController.cpp&case=true#2565

If we scroll and we have the content timeout, we process the touch events but never decrement mTouchBlockBalance. Then if we scroll again, when the content receives the touch, mTouchBlockBalance is > 0 so we never process the touch event:

http://dxr.mozilla.org/mozilla-central/source/gfx/layers/apz/src/AsyncPanZoomController.cpp?from=AsyncPanZoomController.cpp&case=true#2578

Thus we are in a state of perpetual timeouts. Is this a bug or am I misunderstanding the code? Thanks!
Attachment #8464375 - Flags: feedback?(bugmail.mozilla)
Flags: needinfo?(bugmail.mozilla)
Created attachment 8464380 [details] [diff] [review]
Part 6: Make sure all tasks from APZ are run on the vsync thread

When content tells APZ that it received a touch, it occurs on the main thread. This patch ensures that we process the touch event in APZ on the vsync thread and not on both the main thread and vsync thread. We've had IPC crashes when before if APZ processes touch events on two different threads.
Attachment #8464380 - Flags: feedback?(bugmail.mozilla)
For part 6, I will delete a lot of the runtime aborts after some more testing.
(In reply to Mason Chang [:mchang] from comment #56)
> If we scroll and we have the content timeout, we process the touch events
> but never decrement mTouchBlockBalance. Then if we scroll again, when the
> content receives the touch, mTouchBlockBalance is > 0 so we never process
> the touch event:

I haven't looked at the patches, but mTouchBlockBalance should be decremented as part of the ContentReceivedTouch callback. Every touch block MUST have exactly one ContentReceivedTouch callback, see [1]. The most likely problem here is that the ContentReceivedTouch callback is getting skipped somehow. Once I look through the patches I might have a better idea of what's happening.

[1] http://mxr.mozilla.org/mozilla-central/source/gfx/layers/apz/src/AsyncPanZoomController.h?rev=02c5fd1fe559#734
Flags: needinfo?(bugmail.mozilla)
Attachment #8464371 - Flags: feedback?(bugmail.mozilla) → feedback+
Attachment #8464372 - Flags: feedback?(bugmail.mozilla) → feedback+
Comment on attachment 8464373 [details] [diff] [review]
Part 3: Modify TabParent to send APZ transformed touch events to content

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

These patches are looking a million times better than the last set, thanks!
Attachment #8464373 - Flags: feedback?(bugmail.mozilla) → feedback+
Comment on attachment 8464375 [details] [diff] [review]
Part 5: Dispatch Touch events from gonk to APZ

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

::: widget/gonk/GeckoTouchDispatcher.cpp
@@ +318,5 @@
> +
> +  NS_IMETHOD Run()
> +  {
> +    MOZ_ASSERT(NS_IsMainThread());
> +    mTouchDispatcher->DispatchInputEvent(mGeckoTouch,

Is the GeckoTouchDispatcher ever destroyed? Just want to make sure we don't end up with a bad ref here.
Attachment #8464375 - Flags: feedback?(bugmail.mozilla) → feedback+
Comment on attachment 8464380 [details] [diff] [review]
Part 6: Make sure all tasks from APZ are run on the vsync thread

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

::: layout/ipc/RenderFrameParent.cpp
@@ +523,5 @@
> +  {
> +    // This happens on the main thread, so repost the content received touch onto
> +    // the vsync thread.
> +    Task* contentRecvTouch = NewRunnableMethod(this,
> +                           &RemoteContentController::DoContentReceivedTouch,

You can merge these two functions into a single one like so:

if (NS_IsMainThread()) {
  // stuff from RequestContentReceivedTouch, but s/DoContentReceivedTouch/RequestContentReceivedTouch/
} else {
  // stuff from DoContentReceivedTouch
}

I find that a little more self-documenting and robust, because then you don't need to assert which thread you're on, the code just handles it automatically.

@@ +664,5 @@
> +    if (NS_IsMainThread()) {
> +      MessageLoop::current()->PostDelayedTask(FROM_HERE, aTask, aDelayMs);
> +      NS_RUNTIMEABORT("SHOULD NOT BE ON MAIN THREAD\n");
> +    } else if (MessageLoop::current()) {
> +      // Probably the Compositor thread

s/Compositor/input/ but really you can combine the if/else branches because they do the same thing.

@@ +1168,5 @@
>    }
> +
> +  if (NS_IsMainThread()) {
> +    mContentController->RequestContentReceivedTouch(aGuid, aPreventDefault);
> +  } else if (GetApzcTreeManager()) {

These conditions will need some work, I think. You want to make sure that GetApzcTreeManager()->ContentReceivedTouch is called on the same thread that ReceiveInputEvent is called on. With your changes, on B2G, that will be the input thread. This code however might be used in e10s on non-B2G platforms in which case it should remain on the main thread (or whatever thread input events are delivered on).
Attachment #8464380 - Flags: feedback?(bugmail.mozilla) → feedback+
Created attachment 8465810 [details] [diff] [review]
Part 6: Make sure all tasks from APZ are run on the vsync thread

(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #62)
> Comment on attachment 8464380 [details] [diff] [review]
> Part 6: Make sure all tasks from APZ are run on the vsync thread
> 
> Review of attachment 8464380 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: layout/ipc/RenderFrameParent.cpp
> @@ +523,5 @@
> > +  {
> > +    // This happens on the main thread, so repost the content received touch onto
> > +    // the vsync thread.
> > +    Task* contentRecvTouch = NewRunnableMethod(this,
> > +                           &RemoteContentController::DoContentReceivedTouch,
> 
> You can merge these two functions into a single one like so:
> 
> if (NS_IsMainThread()) {
>   // stuff from RequestContentReceivedTouch, but
> s/DoContentReceivedTouch/RequestContentReceivedTouch/
> } else {
>   // stuff from DoContentReceivedTouch
> }
> 
> I find that a little more self-documenting and robust, because then you
> don't need to assert which thread you're on, the code just handles it
> automatically.
> 

Updated to do that in this version. 

> @@ +664,5 @@
> > +    if (NS_IsMainThread()) {
> > +      MessageLoop::current()->PostDelayedTask(FROM_HERE, aTask, aDelayMs);
> > +      NS_RUNTIMEABORT("SHOULD NOT BE ON MAIN THREAD\n");
> > +    } else if (MessageLoop::current()) {
> > +      // Probably the Compositor thread
> 
> s/Compositor/input/ but really you can combine the if/else branches because
> they do the same thing.
> 

I will do that, I just want to leave the runtime abort there to test more corner cases to ensure that we really are processing all events on the vsync thread.

> @@ +1168,5 @@
> >    }
> > +
> > +  if (NS_IsMainThread()) {
> > +    mContentController->RequestContentReceivedTouch(aGuid, aPreventDefault);
> > +  } else if (GetApzcTreeManager()) {
> 
> These conditions will need some work, I think. You want to make sure that
> GetApzcTreeManager()->ContentReceivedTouch is called on the same thread that
> ReceiveInputEvent is called on. With your changes, on B2G, that will be the
> input thread. This code however might be used in e10s on non-B2G platforms
> in which case it should remain on the main thread (or whatever thread input
> events are delivered on).

For the moment, I put the call to mContentController in an #ifdef MOZ_WIDGET_GONK until I think of a better way to ensure that the thread that calls ReceiveInputEvent == the thread that handles content touch received.
Attachment #8464374 - Attachment is obsolete: true
Attachment #8464380 - Attachment is obsolete: true
Created attachment 8467289 [details] [diff] [review]
Part 1: Add WidgetAPZTouchEvent and methods to convert to/from MultiTouchInput

Updated to merge cleanly with bug 970751, which contains GeckoTouchDispatcher work.
Attachment #8464371 - Attachment is obsolete: true
Attachment #8464372 - Attachment is obsolete: true
Attachment #8464373 - Attachment is obsolete: true
Attachment #8464375 - Attachment is obsolete: true
Attachment #8465810 - Attachment is obsolete: true
Attachment #8467289 - Flags: feedback+
Created attachment 8467290 [details] [diff] [review]
Part 2: Enable nsWindow to dispatch input event to APZ

Carrying f+ over.
Attachment #8467290 - Flags: feedback+
Created attachment 8467291 [details] [diff] [review]
Part 3: Modify TabParent to send APZ transformed touch events to content
Attachment #8467291 - Flags: feedback+
Created attachment 8467295 [details] [diff] [review]
Part 4: Dispatch Touch Events from Gonk to APZ

Modifications to take touch events from GeckoTouchDispatcher in bug 970751 and dispatch the resampled event to APZ. 

> Is the GeckoTouchDispatcher ever destroyed? Just want to make sure we don't 
> end up with a bad ref here.

From :mwu's feedback, we now use an nsRefPtr<GeckoTouchDispatcher>. However, this is only destroyed when GeckoInputDispatcher is destroyed, which I'm not sure it ever is.
Attachment #8467295 - Flags: feedback+
Created attachment 8467296 [details] [diff] [review]
Part 5: Ensure all APZ tasks are run on the vsync thread

Same from comment 6. Will again remove all printfs / runtime aborts after more testing.
Attachment #8467296 - Flags: feedback+
Duplicate of this bug: 920036
Created attachment 8471639 [details] [diff] [review]
APZ Process Touch Events on Vsync Thread

Rebased off master and with bug 970751. Will split back into multiple parts once bug 970751 lands. Carrying f+=kats.
Attachment #8467289 - Attachment is obsolete: true
Attachment #8467290 - Attachment is obsolete: true
Attachment #8467291 - Attachment is obsolete: true
Attachment #8467295 - Attachment is obsolete: true
Attachment #8467296 - Attachment is obsolete: true
Attachment #8471639 - Flags: feedback+
Created attachment 8473085 [details] [diff] [review]
APZ Process Touch Events on Vsync Thread

Rebased off master. Renamed MultiTouchInput::ConvertToWidgetAPZTouchEvent to use the same naming style as ToWidgetTouchEvent. Now use MultiTouchInput::ToWidgetAPZTouchEvent() const; Carrying f+=kats.
Attachment #8471639 - Attachment is obsolete: true
Attachment #8473085 - Flags: feedback+
Depends on: 1053892
Created attachment 8474903 [details] [diff] [review]
APZ Process Touch Events on Vsync Thread

Rebased off master and off 970751. Lots of printfs which I want to keep in to debug bug 1053892 and other regressions that probably come up.
Attachment #8473085 - Attachment is obsolete: true
Depends on: 950934
Looks like until we get APZ hit testing working and in the parent process, this will have to bitrot :(.
feature-b2g: --- → 2.2+

Comment 74

3 years ago
Hi, Milan, do we know who could be the best one to own this bug? Thanks.
Flags: needinfo?(milan)

Updated

3 years ago
tracking-b2g: --- → +
(In reply to Kevin Hu [:khu] from comment #74)
> Hi, Milan, do we know who could be the best one to own this bug? Thanks.

I don't understand the question - the bug is assigned, let me know what more details you need.
Flags: needinfo?(milan)
Created attachment 8552019 [details] [diff] [review]
Feed input to APZ on the compositor thread (WIP)

This moves the touch input to the compositor thread, although we could bump it to a different thread entirely if so desired. The only requirement is that we be able to post tasks to that thread, and unfortunately doing that with either the vsync thread or the libui thread is not easy.

This patch seems to work fine on top of my local patch queue but needs more testing.
Assignee: mchang → bugmail.mozilla
Attachment #8474903 - Attachment is obsolete: true
Created attachment 8555484 [details] [diff] [review]
Part 1 - Introduce APZThreadUtils
Created attachment 8555485 [details] [diff] [review]
Part 2 - Encapsulate running things on the controller thread
Created attachment 8555488 [details] [diff] [review]
Part 3 - Switch the B2G controller thread to the compositor thread
Attachment #8552019 - Attachment is obsolete: true
Comment on attachment 8555484 [details] [diff] [review]
Part 1 - Introduce APZThreadUtils

Might as well get this one reviewed since it would be good to land regardless.
Attachment #8555484 - Flags: review?(botond)
from irc: setting a need info on myself to test this.
Flags: needinfo?(bugmail.mozilla)
Flags: needinfo?(bugmail.mozilla) → needinfo?(mchang)
I got to test this out, great stuff. With just master, so only the vsync compositor enabled, it works pretty well. There is one bug though:

1) Load contacts app
2) Load a contact
3) push back to go back to the contacts list
4) Cannot scroll, bug

I also see that the notification bar is lot jankier. But super heavy websites, like nytimes and reddit work really well. I can scroll up/down (into checkerboarding), but it's responsive. Edge swipes are a little worse, but the "bad" cases are already bad and even though they feel jankier, it's not that bad. I think overall it's a nice win.

I also tested with touch resampling enabled. Most of the jankiness on the main thread go away with touch resampling. The notification bar is smoother while tracking my finger than just master without silk or this patch. Edge swipes are still janky, but I've never seen them smooth generally. There was also the occasional super jank with touch resampling while scrolling the homescreen due to the main thread being busy. With this patch, I can't get it to jank on the homescreen and the two combined really improved the responsiveness by far! Scrolling text heavy sites used to be really bad, now it's pretty good.
Flags: needinfo?(mchang)
Created attachment 8555585 [details]
Video of patch + resampling vs master

Just showing a very nice perf win!
Hmm the bug from comment 82 seems to be in master without this patch. Might be related to 1126579 then.
Attachment #8555485 - Flags: review?(mwu)
Attachment #8555485 - Flags: review?(botond)
Attachment #8555488 - Flags: review?(mwu)
Attachment #8555488 - Flags: review?(botond)
Comment on attachment 8555484 [details] [diff] [review]
Part 1 - Introduce APZThreadUtils

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

::: gfx/layers/apz/src/APZCTreeManager.cpp
@@ +137,5 @@
>  void
>  APZCTreeManager::SetAllowedTouchBehavior(uint64_t aInputBlockId,
>                                           const nsTArray<TouchBehaviorFlags> &aValues)
>  {
> +  APZThreadUtils::AssertOnControllerThread();

InputQueue::SetAllowedTouchBehavior already asserts this. Why double-assert?

@@ +691,5 @@
>  APZCTreeManager::ProcessTouchInput(MultiTouchInput& aInput,
>                                     ScrollableLayerGuid* aOutTargetGuid,
>                                     uint64_t* aOutInputBlockId)
>  {
> +  APZThreadUtils::AssertOnControllerThread();

I think this assertion would make more sense in APZCTreeManager::ReceiveInputEvent(WidgetInputEvent), as the other overload of ReceiveInputEvent has it, and this is a helper function for both.
Attachment #8555484 - Flags: review?(botond) → review+
Comment on attachment 8555485 [details] [diff] [review]
Part 2 - Encapsulate running things on the controller thread

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

Does RenderFrameParent::NotifyInputEvent need to use RunOnControllerThread?

::: layout/ipc/RenderFrameParent.cpp
@@ +564,5 @@
> +    void (APZCTreeManager::*setTargetApzcFunc)(uint64_t, const nsTArray<ScrollableLayerGuid>&)
> +        = &APZCTreeManager::SetTargetAPZC;
> +    APZThreadUtils::RunOnControllerThread(NewRunnableMethod(
> +        GetApzcTreeManager(), setTargetApzcFunc,
> +        aInputBlockId, aTargets));

Yuck!

Now that we can use C++11 lambdas in our code, you may want to consider using one:

nsRefPtr<layers::APZCTreeManager> treeManager = GetApzcTreeManager();
APZThreadUtils::RunOnControllerThread(NewRunnableFunction([=]() {
  treeManager->SetTargetAPZC(aInputBlockId, aTargets);
}));

(The purpose of the local variable is to avoid having the lambda capture the 'this' pointer (to the RenderFrameParent object), which it would if you called a method like GetApzcTreeManager() inside the lambda.)

::: widget/gonk/nsWindow.cpp
@@ +291,5 @@
> +    // we need more. Also we can't pass in |this| to the task because nsWindow
> +    // refcounting is not threadsafe. Instead we just use the gFocusedWindow
> +    // static ptr inside the task.
> +    NS_DispatchToMainThread(new DispatchTouchInputOnMainThread(
> +        aInput, guid, inputBlockId));

template <typename Lambda>
class LambdaRunnable : public nsIRunnable {
  Lambda mLambda;
  LambdaRunnable(const Lambda& aLambda) : mLambda(aLambda) {}
  NS_IMETHOD Run() {
    mLambda();
  }
};

...

NS_DispatchToMainThread(new LambdaRunnable([=]() {
  if (gFocusedWindow) {
    gFocusedWindow->DispatchTouchEventForApz(aInput, guid, inputBlockId);
  }
}));

@@ +427,5 @@
> +    // non-const ref. At this callsite we don't care about the mutations that
> +    // the function performs so this is fine. Also we can't pass |this| to the
> +    // task because nsWindow refcounting is not threadsafe. Instead we just use
> +    // the gFocusedWindow static ptr instead the task.
> +    APZThreadUtils::RunOnControllerThread(new DispatchTouchInputOnControllerThread(inputToDispatch));

NewRunnableFunction([=]() mutable {
  if (gFocusedWindow) {
    gFocusedWindow->DispatchTouchInputViaAPZ(inputToDispatch);
  }
});

Note the use of 'mutable' to allow the lambda to mutate the captured variable 'inputToDispatch'.

::: widget/nsBaseWidget.cpp
@@ +962,5 @@
> +    // need a local var to disambiguate between the SetTargetAPZC overloads.
> +    void (APZCTreeManager::*setTargetApzcFunc)(uint64_t, const ScrollableLayerGuid&)
> +        = &APZCTreeManager::SetTargetAPZC;
> +    APZThreadUtils::RunOnControllerThread(NewRunnableMethod(
> +        mAPZC.get(), setTargetApzcFunc, aInputBlockId, aGuid));

nsRefPtr<APZCTreeManager> treeManager = mAPZC;
NewRunnableFunction([=]() {
  treeManager->SetTargetAPZC(aInputBlockId, aGuid);
})
Comment on attachment 8555488 [details] [diff] [review]
Part 3 - Switch the B2G controller thread to the compositor thread

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

I didn't realize the controller thread would be the compositor thread! I thought it would be a new thread.

Does this mean that some of what we do in APZ for sharing data between the compositor thread and the controller thread (for example, making a copy of some things stored in the layer tree for access on the controller thread) is now technically unnecessary?
(In reply to Botond Ballo [:botond] from comment #86)
> Now that we can use C++11 lambdas in our code

Actually, we can only use lambdas in 38 and later, but we want to uplift this patch to 37. Never mind then :)
Would the changes to nsWindow in the Part 2 patch be more appropriate in Part 3?
(In reply to Botond Ballo [:botond] from comment #85)
> ::: gfx/layers/apz/src/APZCTreeManager.cpp
> @@ +137,5 @@
> >  void
> >  APZCTreeManager::SetAllowedTouchBehavior(uint64_t aInputBlockId,
> >                                           const nsTArray<TouchBehaviorFlags> &aValues)
> >  {
> > +  APZThreadUtils::AssertOnControllerThread();
> 
> InputQueue::SetAllowedTouchBehavior already asserts this. Why double-assert?

I was mostly just adding them locally to be able to trace all the APZCTM functions that have this requirement. I can remove this instance.

> @@ +691,5 @@
> >  APZCTreeManager::ProcessTouchInput(MultiTouchInput& aInput,
> >                                     ScrollableLayerGuid* aOutTargetGuid,
> >                                     uint64_t* aOutInputBlockId)
> >  {
> > +  APZThreadUtils::AssertOnControllerThread();
> 
> I think this assertion would make more sense in
> APZCTreeManager::ReceiveInputEvent(WidgetInputEvent), as the other overload
> of ReceiveInputEvent has it, and this is a helper function for both.

Sounds good, I'll move it over.

(In reply to Botond Ballo [:botond] from comment #86)
> 
> Does RenderFrameParent::NotifyInputEvent need to use RunOnControllerThread?

In theory, yes, but that function will go away entirely. All input should be entering the APZ from widget rather than in RenderFrameParent. I verified that on B2G this is the case now. Other platforms might still have some stray events routing through here, but once we turn on parent-process APZ those will be wrong and will need to be removed. (This goes hand in hand with increasing the number of events that early-return out of TabParent::MaybeForwardEventToRenderFrame).

> Now that we can use C++11 lambdas in our code, you may want to consider
> using one:
> 
> nsRefPtr<layers::APZCTreeManager> treeManager = GetApzcTreeManager();
> APZThreadUtils::RunOnControllerThread(NewRunnableFunction([=]() {
>   treeManager->SetTargetAPZC(aInputBlockId, aTargets);
> }));
> 
> (The purpose of the local variable is to avoid having the lambda capture the
> 'this' pointer (to the RenderFrameParent object), which it would if you
> called a method like GetApzcTreeManager() inside the lambda.)

That's pretty neat! I'm disappointed now that I can't use it just yet. Any other changes I can make that are upliftable to 37 to clean this up?

(In reply to Botond Ballo [:botond] from comment #87)
> I didn't realize the controller thread would be the compositor thread! I
> thought it would be a new thread.

Actually I wanted it to be the libui thread that we receive the input events on, but we can't easily post tasks to that thread which is a requirement of the controller thread. Mason had a hacky approach to do that in attachment 8446256 [details] [diff] [review] but I'd rather use the compositor thread for now and then revisit this in the future. Mason would also prefer it on the compositor thread for now since then we don't have to worry as much about racing with composition.

> Does this mean that some of what we do in APZ for sharing data between the
> compositor thread and the controller thread (for example, making a copy of
> some things stored in the layer tree for access on the controller thread) is
> now technically unnecessary?

For B2G right now, yes. However I would still like to in the long run switch the controller thread to be another thread. Even if we don't, platforms like Fennec are definitely not going to have the controller thread be the same as the compositor thread because that would involve running the compositor on the Java UI thread (which would jank the UI) or redispatching events from the Java UI thread to the compositor thread needlessly. So the machinery we put in place for the hit-testing tree is still needed, and makes it easy for us to decide on a controller thread on a per-platform basis.

(In reply to Botond Ballo [:botond] from comment #89)
> Would the changes to nsWindow in the Part 2 patch be more appropriate in
> Part 3?

Yeah, that makes sense. When I split those patches initially I was hoping to make part 3 trivial but it turned out to not be as I imagined. I'll split and merge those bits.
Created attachment 8556155 [details] [diff] [review]
Part 1 - Introduce APZThreadUtils

Updated, carrying r=botond
Attachment #8555484 - Attachment is obsolete: true
Attachment #8556155 - Flags: review+
Created attachment 8556156 [details] [diff] [review]
Part 2 - Introduce APZThreadUtils::RunOnControllerThread (v2)
Attachment #8555485 - Attachment is obsolete: true
Attachment #8555485 - Flags: review?(mwu)
Attachment #8555485 - Flags: review?(botond)
Attachment #8556156 - Flags: review?(botond)
Created attachment 8556157 [details] [diff] [review]
Part 3 - Switch the B2G controller thread to the compositor thread (v2)
Attachment #8555488 - Attachment is obsolete: true
Attachment #8555488 - Flags: review?(mwu)
Attachment #8555488 - Flags: review?(botond)
Attachment #8556157 - Flags: review?(mwu)
Attachment #8556157 - Flags: review?(botond)
(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #90)
> (In reply to Botond Ballo [:botond] from comment #86)
> > 
> > Does RenderFrameParent::NotifyInputEvent need to use RunOnControllerThread?
> 
> In theory, yes, but that function will go away entirely. All input should be
> entering the APZ from widget rather than in RenderFrameParent. I verified
> that on B2G this is the case now.

Can we assert to this effect?

> > Now that we can use C++11 lambdas in our code, you may want to consider
> > using one:
> > 
> > nsRefPtr<layers::APZCTreeManager> treeManager = GetApzcTreeManager();
> > APZThreadUtils::RunOnControllerThread(NewRunnableFunction([=]() {
> >   treeManager->SetTargetAPZC(aInputBlockId, aTargets);
> > }));
> > 
> > (The purpose of the local variable is to avoid having the lambda capture the
> > 'this' pointer (to the RenderFrameParent object), which it would if you
> > called a method like GetApzcTreeManager() inside the lambda.)
> 
> That's pretty neat! I'm disappointed now that I can't use it just yet. Any
> other changes I can make that are upliftable to 37 to clean this up?

The best I can think of is to add an overload to NewRunnableMethod that looks like this:

  template <class Return, class T, class Arg1, class Arg2>
  CancelableTask* NewRunnableMethod(T* object, const Arg1& arg1, const Arg2& arg2, Return (T::*method)(Arg1, Arg2));

and then call it like this:

  NewRunnableMethod<void>(GetApzcTreeManager(), aInputBlockId, aTargets, &APZCTreeManager::SetTargetAPZC);

This signature makes the following changes compared to the one we would call if we didn't have an overloaded function:

  - Instead of making the method pointer its own template parameter, we write out the
    method pointer type in terms of the class, parameter, and return types.

  - |arg1| and |arg2| now come before |method|.

  - We introduce a template parameter for the return type, which needs to be explicitly
    specified (hence the '<void>' in the call).

The idea is that *if* each of the component types in 'Return (T::*)(Arg1, Arg2)' have been deduced by the time the compiler processes that type, the compiler will deduce the overload of SetTargetAPZC that matches this signature. The reason we had to move |arg1| and |arg2| before |method| was so that |Arg1| and |Arg2| got deduced in time. The reason we have to explicitly specify |Return| is because there isn't any place we can deduce it from.

Up to you if you like this. We could also rename one of the overloads, or leave things as in the current patch. I'm OK with either.
Comment on attachment 8556156 [details] [diff] [review]
Part 2 - Introduce APZThreadUtils::RunOnControllerThread (v2)

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

r+ with the assertion added in NotifyInputEvent.
Attachment #8556156 - Flags: review?(botond) → review+
Comment on attachment 8556157 [details] [diff] [review]
Part 3 - Switch the B2G controller thread to the compositor thread (v2)

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

Aesthetics aside, this looks good to me :)
Attachment #8556157 - Flags: review?(botond) → review+
(In reply to Botond Ballo [:botond] from comment #94)
> > In theory, yes, but that function will go away entirely. All input should be
> > entering the APZ from widget rather than in RenderFrameParent. I verified
> > that on B2G this is the case now.
> 
> Can we assert to this effect?

We can, what kind of assert would you like to see? I was thinking that the assert inside APZCTreeManager::ReceiveInputEvent already took care of pretty much everything.

> The best I can think of is to add an overload to NewRunnableMethod that
> looks like this:
>
> [snip]
>
> Up to you if you like this. We could also rename one of the overloads, or
> leave things as in the current patch. I'm OK with either.

Hm, interesting. I'm not really sure I like this approach particularly since we should be able to switch to lambdas (which I find cleaner) going forward. I also don't want to uplift new versions of NewRunnableMethod to 37. Given that I'd rather leave things as-is and then switch to lambdas in the future.
(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #97)
> (In reply to Botond Ballo [:botond] from comment #94)
> > > In theory, yes, but that function will go away entirely. All input should be
> > > entering the APZ from widget rather than in RenderFrameParent. I verified
> > > that on B2G this is the case now.
> > 
> > Can we assert to this effect?
> 
> We can, what kind of assert would you like to see? I was thinking that the
> assert inside APZCTreeManager::ReceiveInputEvent already took care of pretty
> much everything.

Oh, good point! Ignore that then :)

Comment 99

3 years ago
(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #90)
> (In reply to Botond Ballo [:botond] from comment #87)
> > I didn't realize the controller thread would be the compositor thread! I
> > thought it would be a new thread.
> 
> Actually I wanted it to be the libui thread that we receive the input events
> on, but we can't easily post tasks to that thread which is a requirement of
> the controller thread. Mason had a hacky approach to do that in attachment
> 8446256 [details] [diff] [review] but I'd rather use the compositor thread
> for now and then revisit this in the future. Mason would also prefer it on
> the compositor thread for now since then we don't have to worry as much
> about racing with composition.


Can you please file a followup bug for this and post it here? Just to be sure this gets noted in Bugzilla.
(In reply to Florian Bender from comment #99)
> Can you please file a followup bug for this and post it here? Just to be
> sure this gets noted in Bugzilla.

Yup, I'll do that if mwu gives an r+ for the patch pending review. He might prefer just hacking up libui as part of this bug...

Updated

3 years ago
Blocks: 1120949
Comment on attachment 8556156 [details] [diff] [review]
Part 2 - Introduce APZThreadUtils::RunOnControllerThread (v2)

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

The assumption:

  // On B2G the controller thread is the compositor thread, and this function
  // is always called from the libui thread or the main thread.

does not seem to hold. Stack trace:

Program received signal SIGSEGV, Segmentation fault.
[Switching to Thread 2963.3009]
0xb470cf2e in mozilla::layers::APZThreadUtils::RunOnControllerThread (aTask=0xab584580) at ../../../gfx/layers/apz/util/APZThreadUtils.cpp:59
warning: Source file is more recent than executable.
59        MOZ_ASSERT(MessageLoop::current() != loop);
(gdb) i s
#0  0xb470cf2e in mozilla::layers::APZThreadUtils::RunOnControllerThread (aTask=0xab584580) at ../../../gfx/layers/apz/util/APZThreadUtils.cpp:59
#1  0xb509073e in mozilla::GeckoTouchDispatcher::NotifyVsync (aVsyncTimestamp=...) at ../../../widget/gonk/GeckoTouchDispatcher.cpp:106
#2  0xb4748ba6 in DispatchTouchEvents (aVsyncTimestamp=..., this=<optimized out>) at ../../../gfx/layers/ipc/CompositorParent.cpp:371
#3  mozilla::layers::CompositorVsyncObserver::Composite (this=0xae191000, aVsyncTimestamp=...) at ../../../gfx/layers/ipc/CompositorParent.cpp:341
#4  0xb4736a22 in DispatchToMethod<mozilla::layers::CompositorParent, void (mozilla::layers::CompositorParent::*)(mozilla::TimeStamp), mozilla::TimeStamp> (arg=..., method=
    (void (mozilla::layers::CompositorParent::*)(mozilla::layers::CompositorParent * const, mozilla::TimeStamp)) 0xb4748b29 <mozilla::layers::CompositorVsyncObserver::Composite(mozilla::TimeStamp)>, obj=<optimized out>) at ../../../ipc/chromium/src/base/tuple.h:393
#5  RunnableMethod<mozilla::layers::CompositorParent, void (mozilla::layers::CompositorParent::*)(mozilla::TimeStamp), Tuple1<mozilla::TimeStamp> >::Run (this=<optimized out>)
    at ../../../ipc/chromium/src/base/task.h:310
#6  0xb43ff0ec in MessageLoop::RunTask (this=0xb06c3cc0, task=0xab2430a0) at ../../../ipc/chromium/src/base/message_loop.cc:361
#7  0xb4402222 in MessageLoop::DeferOrRunPendingTask (this=<optimized out>, pending_task=...) at ../../../ipc/chromium/src/base/message_loop.cc:369
#8  0xb4403fd8 in DoWork (this=<optimized out>) at ../../../ipc/chromium/src/base/message_loop.cc:447
#9  MessageLoop::DoWork (this=0xb06c3cc0) at ../../../ipc/chromium/src/base/message_loop.cc:426
#10 0xb440015c in base::MessagePumpDefault::Run (this=0xb0999d80, delegate=0xb06c3cc0) at ../../../ipc/chromium/src/base/message_pump_default.cc:34
#11 0xb44000d8 in MessageLoop::RunInternal (this=this@entry=0xb06c3cc0) at ../../../ipc/chromium/src/base/message_loop.cc:233
#12 0xb44000f2 in RunHandler (this=0xb06c3cc0) at ../../../ipc/chromium/src/base/message_loop.cc:226
#13 MessageLoop::Run (this=this@entry=0xb06c3cc0) at ../../../ipc/chromium/src/base/message_loop.cc:200
#14 0xb4405700 in base::Thread::ThreadMain (this=0xb09c8040) at ../../../ipc/chromium/src/base/thread.cc:170
#15 0xb43f9280 in ThreadFunc (closure=<optimized out>) at ../../../ipc/chromium/src/base/platform_thread_posix.cc:39
#16 0xb6e5f22c in __thread_entry (func=0xb43f9279 <ThreadFunc(void*)>, arg=0xb09c8040, tls=0xb06c3dd0) at bionic/libc/bionic/pthread_create.cpp:105
#17 0xb6e5f3c4 in pthread_create (thread_out=0xb09c8048, attr=<optimized out>, start_routine=0xb43f9279 <ThreadFunc(void*)>, arg=0x78) at bionic/libc/bionic/pthread_create.cpp:224
#18 0x00000000 in ?? ()
Attachment #8556156 - Flags: review+ → review-
Comment on attachment 8556157 [details] [diff] [review]
Part 3 - Switch the B2G controller thread to the compositor thread (v2)

This has rotted a bit, will post new version.
Attachment #8556157 - Flags: review?(mwu)
Created attachment 8559267 [details]
Assertion failure

Need to fix this too.
Comment on attachment 8556156 [details] [diff] [review]
Part 2 - Introduce APZThreadUtils::RunOnControllerThread (v2)

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

::: gfx/layers/apz/util/APZThreadUtils.cpp
@@ +61,5 @@
> +#else
> +  // On non-B2G platforms this is only ever called from the controller thread
> +  // itself.
> +  AssertOnControllerThread();
> +  aTask->Run();

On a different note, if we Run() a task without posting it to a message loop, aren't we responsible for deleting it?
Created attachment 8559979 [details] [diff] [review]
Workaround for assertion in comment 101

This patch fixes the assertion in comment 101 for me locally, and allows me to continue developing bug 1127066 on top of this bug for now.
Created attachment 8560013 [details] [diff] [review]
Part 2 - Introduce APZThreadUtils::RunOnControllerThread (v3)

(In reply to Botond Ballo [:botond] from comment #104)
> On a different note, if we Run() a task without posting it to a message
> loop, aren't we responsible for deleting it?

Indeed, good catch! Updated patch to do that.
Attachment #8556156 - Attachment is obsolete: true
Attachment #8560013 - Flags: review+
Created attachment 8560015 [details] [diff] [review]
Part 3 - Switch the B2G controller thread to the compositor thread (v3)

Updated part 3 to deal with the assertion.
Attachment #8556157 - Attachment is obsolete: true
Attachment #8559979 - Attachment is obsolete: true
I tried those 3 patches, and found some issues that I don't see in latest nightly:
- system messages, like the geolocation permission or crash notification messages do not accept UI input (i.e., I can't press the buttons
- Going to www.youtube.com from the browser, it crashes.
- In gallery app, when I tap the thumbnail of a picture, it shows black screen.

Not sure whether this is due to the patch, or the tip of master branch actually contains some other bugs at the time, but they all seem to be UI related (except the last one) so might worth taking a look.  let me know if you want me to perform specific steps.
(In reply to No-Jun Park [:njpark] from comment #108)
> I tried those 3 patches, and found some issues that I don't see in latest
> nightly:
> - system messages, like the geolocation permission or crash notification

Related to bug 1130067?

> messages do not accept UI input (i.e., I can't press the buttons
> - Going to www.youtube.com from the browser, it crashes.

This looks like bug 1130086.
I guess the latest tinderbox builds contain these fixes?
(In reply to No-Jun Park [:njpark] from comment #108)
> - In gallery app, when I tap the thumbnail of a picture, it shows black
> screen.

And this one is Bug 1130403 it seems.  I'll be running another check on the latest nightly today just to make sure.
Thanks no-jun! It does look like all of those issues are unrelated to my patches.

I did some more local testing on a debug build and found an error though.

(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #90)
> (In reply to Botond Ballo [:botond] from comment #86)
> > 
> > Does RenderFrameParent::NotifyInputEvent need to use RunOnControllerThread?
> 
> In theory, yes, but that function will go away entirely. All input should be
> entering the APZ from widget rather than in RenderFrameParent. I verified
> that on B2G this is the case now. Other platforms might still have some
> stray events routing through here, but once we turn on parent-process APZ
> those will be wrong and will need to be removed. (This goes hand in hand
> with increasing the number of events that early-return out of
> TabParent::MaybeForwardEventToRenderFrame).

Turns out this wasn't entirely accurate. I ran into a case where the mouse events dispatched from ChromeProcessController::HandleSingleTap were getting dispatched to the child process and therefore entering RenderFrameParent::NotifyInputEvent. This caused a crash on debug builds because of the AssertOnControllerThread violation. I'll attach another patch to remove this codepath entirely since I think it's safe to do at this point.
Depends on: 1130535
Attachment #8560015 - Flags: review?(mwu)
Attachment #8560015 - Flags: review?(botond)

Updated

3 years ago
Attachment #8560015 - Flags: review?(botond) → review+
(Assignee)

Comment 112

3 years ago
greentry
Try push at https://treeherder.mozilla.org/#/jobs?repo=try&revision=bad983ec228b is green.
Blocks: 1126090

Comment 113

3 years ago
Comment on attachment 8560015 [details] [diff] [review]
Part 3 - Switch the B2G controller thread to the compositor thread (v3)

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

I don't have a good feel for the entire patch, but things seem mostly right locally. Don't think I'm gonna get any good insights by reading this again (for probably the 4th time) tomorrow, so r=me.

::: widget/gonk/GeckoTouchDispatcher.h
@@ +47,5 @@
>  
>  public:
>    GeckoTouchDispatcher();
>    void NotifyTouch(MultiTouchInput& aTouch, TimeStamp aEventTime);
> +  void DispatchTouchEvent(MultiTouchInput aMultiTouch);

Is this change intentional?
Attachment #8560015 - Flags: review?(mwu) → review+
Comment on attachment 8560015 [details] [diff] [review]
Part 3 - Switch the B2G controller thread to the compositor thread (v3)

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

::: widget/gonk/GeckoTouchDispatcher.h
@@ +47,5 @@
>  
>  public:
>    GeckoTouchDispatcher();
>    void NotifyTouch(MultiTouchInput& aTouch, TimeStamp aEventTime);
> +  void DispatchTouchEvent(MultiTouchInput aMultiTouch);

I believe so. The patch changes the way of invoking this method from direct invocation to being scheduled on the controller thread via NewRunnableMethod. Digging a bit in the implementation of NewRunnableMethod reveals that the when it runs the method, it passes all arguments by const reference [1], so the call wouldn't compile if the method took the argument by nonconst reference.

[1] https://dxr.mozilla.org/mozilla-central/source/ipc/chromium/src/base/tuple.h#392
Yup, botond is right, it was intentional for exactly that reason.

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/d805db38cd5f
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/e2b0f9037728
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/5a35ca7c0adc
https://hg.mozilla.org/mozilla-central/rev/d805db38cd5f
https://hg.mozilla.org/mozilla-central/rev/e2b0f9037728
https://hg.mozilla.org/mozilla-central/rev/5a35ca7c0adc
Status: ASSIGNED → RESOLVED
Last Resolved: 3 years ago
status-firefox38: --- → fixed
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla38
No longer blocks: 1126090
Whiteboard: [c=effect p=5 s= u=] → [c=effect p=5 s= u=][NO_UPLIFT][input-thread-uplift-part7]
Comment on attachment 8556155 [details] [diff] [review]
Part 1 - Introduce APZThreadUtils

NOTE: Please see https://wiki.mozilla.org/Release_Management/B2G_Landing to better understand the B2G approval process and landings.

[Approval Request Comment]
Bug caused by (feature/regressing bug #): required for parent-process APZ (bug 950934) and input-thread (bug 930939) which are 2.2+ features. I'm tracking the full set of bugs that will need uplifting together at http://mzl.la/1zvKgmk; requesting approval on bugs individually but will wait until everything has approval before I uplift any of it.
User impact if declined: can't have parent-process APZ or input-thread
Testing completed: on master. some of these bugs have been baking for a while; others are more recent.
Risk to taking this patch (and alternatives if risky): there's likely some edge cases that we haven't run into yet and will be uncovered with further testing. Bug 1128887 is the only known issue that we don't yet have a fix for but I consider that relatively minor and something we can fix after uplifting everything else.
String or UUID changes made by this patch: none
Attachment #8556155 - Flags: approval-mozilla-b2g37?
Attachment #8560013 - Flags: approval-mozilla-b2g37?
Attachment #8560015 - Flags: approval-mozilla-b2g37?

Comment 118

3 years ago
Hi Kartikaya Gupta,

   whether bug 930939 will land on 2.1&2.1s?

   Thanks a lot
Flags: needinfo?(bugmail.mozilla)
No, 2.2 and later.  There is a large number of large changes involved in enabling this functionality, I would not recommend attempting somebody trying to uplift to 2.1.
Flags: needinfo?(bugmail.mozilla)

Updated

3 years ago
Attachment #8556155 - Flags: approval-mozilla-b2g37? → approval-mozilla-b2g37+

Updated

3 years ago
Attachment #8560013 - Flags: approval-mozilla-b2g37? → approval-mozilla-b2g37+

Updated

3 years ago
Attachment #8560015 - Flags: approval-mozilla-b2g37? → approval-mozilla-b2g37+
(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #117)
> Comment on attachment 8556155 [details] [diff] [review]
> Part 1 - Introduce APZThreadUtils
> 
> NOTE: Please see https://wiki.mozilla.org/Release_Management/B2G_Landing to
> better understand the B2G approval process and landings.
> 
> [Approval Request Comment]
> Bug caused by (feature/regressing bug #): required for parent-process APZ
> (bug 950934) and input-thread (bug 930939) which are 2.2+ features. I'm
> tracking the full set of bugs that will need uplifting together at
> http://mzl.la/1zvKgmk; requesting approval on bugs individually but will
> wait until everything has approval before I uplift any of it.
> User impact if declined: can't have parent-process APZ or input-thread
> Testing completed: on master. some of these bugs have been baking for a
> while; others are more recent.
> Risk to taking this patch (and alternatives if risky): there's likely some
> edge cases that we haven't run into yet and will be uncovered with further
> testing. Bug 1128887 is the only known issue that we don't yet have a fix
> for but I consider that relatively minor and something we can fix after
> uplifting everything else.
> String or UUID changes made by this patch: none

Eric, once kats land this on 2.2 please make sure to verify some of the graphic tests cases ASAP and schedule a more extensive testing as well. These have baked on m-c and kats did address the fallouts we had, so hopefully once uplifted to the branch we do not run into major failure.

:kats, depending on when you land these, can you flag njpark for a graphics sanity run on 2.2, thanks!
https://hg.mozilla.org/releases/mozilla-b2g37_v2_2/rev/1046c97bfd7d
https://hg.mozilla.org/releases/mozilla-b2g37_v2_2/rev/7768b0596a5c
https://hg.mozilla.org/releases/mozilla-b2g37_v2_2/rev/ee7bef814675
status-b2g-v2.2: --- → fixed
status-b2g-master: --- → fixed
Whiteboard: [c=effect p=5 s= u=][NO_UPLIFT][input-thread-uplift-part7] → [c=effect p=5 s= u=][input-thread-uplift-part7]
I landed this (long with the other bugs it depends on) on 2.2, so flagging njpark for a sanity run as per comment 120.
Flags: needinfo?(npark)
(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #122)
> I landed this (long with the other bugs it depends on) on 2.2, so flagging
> njpark for a sanity run as per comment 120.

Hi Kats, I ran some sanity checks on 2.2, and found the following.  Maybe you can quickly check whether they are related to this patch?  I ni?ed milan since they could be graphics bugs.
https://bugzilla.mozilla.org/show_bug.cgi?id=1133966
https://bugzilla.mozilla.org/show_bug.cgi?id=1133961
https://bugzilla.mozilla.org/show_bug.cgi?id=1133955
Flags: needinfo?(npark)
Thanks, No-Jun! I'll take a look at them today or (more likely) tomorrow.
No longer depends on: 988858
Blocks: 1203132
You need to log in before you can comment on or make changes to this bug.