963278 - Turn the "dom.browser_frames.useAsyncPanZoom" pref on by default in b2g

Status: RESOLVED FIXED

(Core :: Panning and Zooming, defect)

Description

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

Bug 961047 added a pref in gecko called "dom.browser_frames.useAsyncPanZoom" to enable APZ in mozbrowser frames by default. The pref is set to false by default because if we set it to true then some tests fail. See bug 961047 comment 29 for logs. So we left it be false; in the gaia environment where it is used in practice it will get set to true because there is a gaia pref that controls it.

We should figure out the test failures and make the flag true by default so that the tests reflect more accurately what happens on actual devices.

Comment 1

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

This is not needed for 1.3 or Qualcomm.

Comment 2

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

The three tests that fail when the pref is enabled (according to my recent try run at [1]) are:

docshell/test/navigation/test_bug430723.html
dom/events/test/test_bug946632.html
layout/forms/test/test_bug564115.html

I started by looking at the second one, and that one appears to be a race in the code. The scroll position is set by script, and the APZC code bounces that scroll position back to content because of [2] and in the meantime script has changed the scroll position again so we end up clobbering it. The clobber-prevention code at [3] doesn't work perfectly because the last-scroll-origin flag is reset at the end of the layer building, and that is the case when the repaint request comes in. I'll think about the proper way to fix this; I verified that commenting out the line at [2] makes the test pass but it will regress other behaviour so we don't want to do that.

The third test in the list above, as far as I can tell, is not specifically related to the APZC code. The page sets the scroll position to 5000 on the new window it opens, and APZC doesn't tamper with that. I see APZC receive a layers updated with the scroll offset at 5000. Then, something else changes the scroll position to 11, and the APZC receives the notification for that. Given that there is an input field on the page I suspect this is related to the issue daleharvey was debugging recently where the autofocus code makes the scroll position jump after interacting with the page (bug 958666). I can retest with that patch to verify.

Haven't looked into the first test on the list yet, but will do that shortly.

[1] https://tbpl.mozilla.org/?tree=Try&rev=3928961892cd
[2] http://mxr.mozilla.org/mozilla-central/source/gfx/layers/ipc/AsyncPanZoomController.cpp?rev=39acb0de3d5a#1607
[3] http://mxr.mozilla.org/mozilla-central/source/widget/xpwidgets/APZCCallbackHelper.cpp?rev=0fbc3cd1bf54#106

Comment 3

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

Attachment: Backtrace of code that sets scroll position to 11

Unfortunately my hypothesis for the autofocus code was incorrect for the third test in the previous comment. However I did grab a stacktrace of the code that sets the scroll position back to 11; see attached. The culprit appears to be some JS code which I assume is the stuff at [1] which is invoked from [2] at a delay of 20ms from a resize event. This would explain why when I set a 10 second timeout in the test after the input.focus(), the test passed. I'm concluding that this is just a bad test that needs to be more robust. Turning on APZ might trigger an extra resize event that wasn't there before and the test doesn't expect that might happen.

[1] http://mxr.mozilla.org/mozilla-central/source/dom/inputmethod/forms.js?rev=c0335f9a2498#1034
[2] http://mxr.mozilla.org/mozilla-central/source/dom/inputmethod/forms.js?rev=c0335f9a2498#418

Comment 4

[[:fabrice] Fabrice Desré]

Interesting. Note that there are issues with keyboard window resizing when APZ is enabled that you could have an idea on. See bug 963584

Comment 5

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

The first test on the list appears to suffer from the same problem as the second one. So I just need to figure out a better solution for that.

Comment 6

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

Attachment: Part 1 - Deal with race when concurrently updating scroll offset from APZ and other places

This is my attempt at fixing the harder problem. It seems to work locally but I still need to make sure it didn't regress anything.

Comment 7

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

Attachment: Part 2 - Ugly robustification of finicky test

Comment 8

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

Attachment: Part 3 - Enable the pref

Pushed to try at https://tbpl.mozilla.org/?tree=Try&rev=c5b55cfe8b05

Comment 10

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

Carrying over blocker from dupe.

Comment 11

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

I noticed that on previous try runs of this flag being turned on the marionette (Mnw) tests went red. I couldn't reproduce that locally, for what it's worth. Looking around in try runs it looks like the redness happens when it takes >= 60 seconds for the homescreen to become available (as determined by the mozbrowserloadend event) and even on green runs that currently takes ~45 seconds. It's possible that even innocuous changes could push it over the edge. The Nuwa change might also suffer from this problem, based on the latest b2g-inbound runs.

Comment 12

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

Indeed, the above timeout was bumped up in bug 948895. Very well then.

Comment 13

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

Attachment: Part 1 - Deal with race when concurrently updating scroll offset from APZ and other places (v2)

Small update to fix the windows build. Try at https://tbpl.mozilla.org/?tree=Try&rev=1994ec3f6357

Comment 14

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

Attachment: Part 1 - Deal with race when concurrently updating scroll offset from APZ and other places (v2)

Whoops, wrong patch last time. I've verified that this doesn't regress bug 951113.

Comment 15

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

Comment on attachment 8368236 [details] [diff] [review]
Part 2 - Ugly robustification of finicky test

Any suggestions on better ways to do this are welcome. I don't particularly like magic number timeouts.

Comment 16

[Jim Mathies [:jimm]]

Comment on attachment 8368557 [details] [diff] [review]
Part 1 - Deal with race when concurrently updating scroll offset from APZ and other places (v2)

Didn't see any noticeable side effects when scrolling with the mouse wheel, keyboard, or scroll bars.

Comment 17

[(no longer active)]

Comment on attachment 8368236 [details] [diff] [review]
Part 2 - Ugly robustification of finicky test

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

This is pretty hackish.  Can you please convert that value into a pref and use it both in forms.js and here?

Comment 18

[(no longer active)]

Or even better, let's avoid using a pref for this, and just add a comment to forms.js saying that if somebody changes that value of that value they remember to change the value in the test as well.  r=me with that.

Comment 19

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

(In reply to :Ehsan Akhgari (needinfo? me!) (@ work week, reduced responsiveness) from comment #18)
> Or even better, let's avoid using a pref for this, and just add a comment to
> forms.js saying that if somebody changes that value of that value they
> remember to change the value in the test as well.  r=me with that.

I changed it to use a pref but it turns out that there might be something else going on here, because even though the value in forms.js is 20, the delay in the test needs to be at least 80 for it to pass. I think the resize event itself gets fired fairly late and that's causing the problem. I'm trying to find another solution that is more robust.

Comment 20

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

So yeah, the resize event fires after the load event. Even after the pageshow, in fact. And so we need to wait >20ms from the resize event rather than >20ms from the input.focus() call before doing the scroll test. Either that, or don't put focus in the input until after the resize event is done. I was trying to do the latter but it's hard because the resize event may or may not fire, so I need a timeout to trip the test to run even if it doesn't fire. Ugh.

Comment 21

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

Attachment: Part 2 - Less ugly robustification of finicky test (v2)

Ok, here's a better fix, more in line with the rest of the code. The resize events fire before the window receives focus, so by the time we're doing the meat of the test the window is stable. Passes locally; re-pushed the whole set to try at https://tbpl.mozilla.org/?tree=Try&rev=61b0c29265c7

Comment 22

[(no longer active)]

Comment on attachment 8368854 [details] [diff] [review]
Part 2 - Less ugly robustification of finicky test (v2)

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

This is much much better, thanks!

Comment 23

[Botond Ballo [:botond]]

Comment on attachment 8368557 [details] [diff] [review]
Part 1 - Deal with race when concurrently updating scroll offset from APZ and other places (v2)

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

I like this approach! r=me

::: dom/ipc/PBrowser.ipdl
@@ +342,5 @@
>  
>      UpdateDimensions(nsRect rect, nsIntSize size, ScreenOrientation orientation) compress;
>  
>      UpdateFrame(FrameMetrics frame) compress;
> +    AcknowledgeScrollUpdate(ViewID aScrollId);

Would be awesome to have a comment here.

Comment 24

[Timothy Nikkel (:tnikkel)]

Comment on attachment 8368557 [details] [diff] [review]
Part 1 - Deal with race when concurrently updating scroll offset from APZ and other places (v2)

Talked to kats about this. The AcknowledgeScrollUpdate reset of scroll origin could clobber a content initiated scroll that happens after we send off a layers update to the compositor. So we're going to go with a scroll generation approach.

Comment 25

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

Attachment: Part 1 - Deal with race when concurrently updating scroll offset from APZ and other places (v3)

Updated part 1 to have a scroll generation. I can't test this locally at the moment (emulator doesn't work over SSH) so pushed to try: https://tbpl.mozilla.org/?tree=Try&rev=70a8a98d695f

Will flag for review if it's good.

Comment 26

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

Comment on attachment 8370504 [details] [diff] [review]
Part 1 - Deal with race when concurrently updating scroll offset from APZ and other places (v3)

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

Try looks good. This is a low-priority review since it's not blocking 1.3 stuff right now (although it *might* help the keyboard-not-appearing issue). Straightforward change from the last version.

Comment 27

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

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/319abcb8c59e
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/6a459bfe7689
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/8690649f9dd3

Comment 28

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/319abcb8c59e
https://hg.mozilla.org/mozilla-central/rev/6a459bfe7689
https://hg.mozilla.org/mozilla-central/rev/8690649f9dd3

Comment 29

[Ryan VanderMeulen [:RyanVM]]

This blocks Nuwa, right?

Comment 30

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

No, not really. The Nuwa landing disabled the tests that were failing here. We can uplift if you want but I don't think it's necessary. The race condition fix is a nice-to-have but I don't think it's worthy of 1.3+.

Comment 31

[Jim Mathies [:jimm]]

In some experimental perf tests we have for metro, this may have regressed a scrollBy perf test by about 13%. Would something like that be expected?

Comment 32

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

No; if anything I would have expected this patch to result in fewer paints and therefore an improvement. Is the scroll initiated on the content side (e.g window.scrollTo) or the APZ side (by simulating touch events)? Also, is there a bug on file for the regression?

Comment 33

[Jim Mathies [:jimm]]

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #32)
> No; if anything I would have expected this patch to result in fewer paints
> and therefore an improvement. Is the scroll initiated on the content side
> (e.g window.scrollTo) or the APZ side (by simulating touch events)? Also, is
> there a bug on file for the regression?

Fewer presented frames is what we're seeing - 

http://www.mathies.com/mozilla/mochiperf.html?testid=79235F74-037C-4F6B-AE47-FDCCC13458C3

here's the actual test - 

http://mxr.mozilla.org/mozilla-central/source/browser/metro/base/tests/mochiperf/browser_miscgfx_01.js#16

No bug filed yet since the tests are pretty new and we just switch over from counting mozAfterPaints to the domUtil frame time recording api.

It looks like painting slowed down, intervals.length decreased across the same time interval.

Comment 34

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

I see no obvious problems with the test, but the range over which the regression happened is rather large (includes two merges to m-c, if I'm reading it right). Is it possible to narrow that down? It's definitely worth filing a bug about.

Comment 35

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

The "part 1" patch on this bug was uplifted to 1.3 in order to fix bug 980041. It is rolled up inside this changeset:

https://hg.mozilla.org/releases/mozilla-b2g28_v1_3/rev/46bb232303b8