Closed Bug 963278 Opened 6 years ago Closed 6 years ago

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

Categories

(Core :: Panning and Zooming, defect)

All
Gonk (Firefox OS)
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla30

People

(Reporter: kats, Assigned: kats)

References

Details

Attachments

(4 files, 4 obsolete files)

7.02 KB, text/plain
Details
940 bytes, patch
vingtetun
: review+
Details | Diff | Splinter Review
1.75 KB, patch
ehsan
: review+
Details | Diff | Splinter Review
33.17 KB, patch
tnikkel
: review+
Details | Diff | Splinter Review
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.
Blocks: 942267
This is not needed for 1.3 or Qualcomm.
No longer blocks: 942267
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
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
Interesting. Note that there are issues with keyboard window resizing when APZ is enabled that you could have an idea on. See bug 963584
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.
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.
See Also: 965351
Duplicate of this bug: 965351
Carrying over blocker from dupe.
Blocks: 950266
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.
Indeed, the above timeout was bumped up in bug 948895. Very well then.
Whoops, wrong patch last time. I've verified that this doesn't regress bug 951113.
Assignee: nobody → bugmail.mozilla
Attachment #8368540 - Attachment is obsolete: true
Attachment #8368557 - Flags: review?(tnikkel)
Attachment #8368557 - Flags: review?(jmathies)
Attachment #8368557 - Flags: review?(botond)
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.
Attachment #8368236 - Flags: review?(ehsan)
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.
Attachment #8368557 - Flags: review?(jmathies) → review+
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?
Attachment #8368236 - Flags: review?(ehsan) → review-
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.
(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.
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.
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
Attachment #8368236 - Attachment is obsolete: true
Attachment #8368854 - Flags: review?(ehsan)
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!
Attachment #8368854 - Flags: review?(ehsan) → review+
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.
Attachment #8368557 - Flags: review?(botond) → review+
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.
Attachment #8368557 - Flags: review?(tnikkel)
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.
Attachment #8368557 - Attachment is obsolete: true
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.
Attachment #8370504 - Flags: review?(tnikkel)
Attachment #8370504 - Flags: review?(tnikkel) → review+
This blocks Nuwa, right?
blocking-b2g: --- → 1.3?
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+.
blocking-b2g: 1.3? → ---
Depends on: 970070
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?
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?
(In reply to Kartikaya Gupta (email:kats@mozilla.com) 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.
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.
Blocks: 961047
No longer depends on: 961047
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
You need to log in before you can comment on or make changes to this bug.