1270955 - Can't scroll audilibrary.audiusa.com with e10s enabled

Status: RESOLVED FIXED

(Core :: Panning and Zooming, defect, P1)

Description

[Mike Taylor [:miketaylr]]

STR: 

1) Navigate to http://audilibrary.audiusa.com/viewer/brochures/17/en_US.audi.Brochures.2017.a4_menu/
2) Let the car/shatter animation complete, click on one of 4 circle categories (Brilliance, etc.)
3) Try to scroll down with trackpad

Expected: Can scroll
Actual: Get stuck

I get Expected Results when I turn off e10s.

Comment 1

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

Attachment: Logging output

Looks like the APZ hit test finds node 0x11c382110 which goes to APZC 0x12c610000 which is guid={ l=3, p=5, v=3 }. However the main-thread hit test finds guid={ l=3, p=7, v=4 }. Unfortunately there's no dispatch-to-content region so the APZ code is "sure" that it found the right APZC, and doesn't use the main-thread hit test result.

Looks like a bug in the event regions, we should find out why the main thread hit-test is giving a different result and why that's not reflected in the event regions.

Comment 2

[Tracy Walker [:tracy]]

I can reproduce with latest Nightly and latest Aurora on Mac.  Using a New non-e10s window does not work.  As reported, track pad scroll of the content experience only works if e10s is turned off.  This page is cleverly using scroll to step through an animation in each of the four demos.

Mac 10.11, Nightly 49.0a1, 20160510030240 - Fail
Win 7, Nightly 49.0a1, 20160510030240 - Partial Pass
Linux, Nightly 49.0a1, 20160510030240 - Fail

Note:  I call Win 7 a Partial Pass because it takes some time, while it appears to be failing to scroll, before the track pad scroll works in e10s. Where as with e10s disabled the track pad scroll works almost immediately.  Also, in all cases the scroll bar works as expected.

Comment 3

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

The layer in question is the ClientTiledPaintedLayer shown below, which has a hitregion exceeding the valid/visible regions:

      ClientContainerLayer (0x1623c9000) [clip=(x=200, y=-16, w=904, h=678)] [visible=< (x=579, y=542, w=133, h=22); >] [isFixedPosition scrollId=4 sides=0x0 anchor=(200,-15.6)]
        ClientTiledPaintedLayer (0x110b63400) [transform=[ 1 0; 0 1; 200 -16; ]] [effective-transform=[ 1 0; 0 1; 200 -16; ]] [bounds=(x=379, y=558, w=133, h=22)] [visible=< (x=379, y=558, w=133, h=22); >] { hitregion=< (x=0, y=0, w=904, h=601); >} [valid=< (x=379, y=558, w=133, h=22); >]

and that comes from these event regions:

    LayerEventRegions p=0x117f9cd38 f=0x11c463820(Block(div)(5) id:preloads class:skrollable skrollable-between) z=100 bounds(0,0,0,0) layerBounds(-12000,936,0,0) visible(12000,-936,54240,36015) componentAlpha(0,0,0,0) clip() scrollClip() ref=0x11aaf8a68 agr=0x11c463820 (hitRegion < (x=12000, y=-936, w=54240, h=36015); >)
    LayerEventRegions p=0x117f9ce20 f=0x11da03620(Block(div)(2) id:loader class:Brilliance skrollable skrollable-after) z=100 bounds(0,0,0,0) layerBounds(-12000,936,0,0) visible(12000,-936,54240,36015) componentAlpha(0,0,0,0) clip() scrollClip() ref=0x11aaf8a68 agr=0x11c463820 (hitRegion < (x=12000, y=-936, w=54240, h=36015); >)

However, when I check what the main-thread hit-test is returning, it is also hitting the id="loader" element (seen in the second LayerEventRegions above), so it seems like the APZ hit test and main-thread hit test are finding the same display item/layer. Presumably, then, the problem is that the layer gets mapped to the wrong APZC.

Looking at the hit-testing tree structure in attachment 8750860 [details], we see 0x12b6f3c60 maps to the ref layer for the content process. Inside that we have some layers for the main page, and 0x1192e6eb0 marks the container layer for the <iframe> that is scrolling. A few of the layers inside that container layer have 0x11cf0d800 as the APZC, but not all the layers do. In particular the layers 0x11c382240 and 0x11c381fe0 do not. 0x11c381fe0 corresponds to the ClientContainerLayer 0x1623c9000 in my snippet above. The layer is marked as fixed-position, which I suspect is related to the problem.

Comment 4

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

Ok, so this basically seems to boil down to hit-testing while over fixed-pos layers inside an iframe. Simple test case at http://people.mozilla.org/~kgupta/bug/1270955.html - if you mouse over the green boxes and try scrolling it doesn't work. If you open the contents of the iframe as a top-level document (and resize your window to make it scrollable) it does scroll. I suspect the code at [1] needs to actually find the root APZC for that particular document, rather than for the layers id.

[1] http://mxr.mozilla.org/mozilla-central/source/gfx/layers/apz/src/APZCTreeManager.cpp?rev=114ca1fc9c51#1778

Comment 5

[Mike Taylor [:miketaylr]]

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #4)
> Ok, so this basically seems to boil down to hit-testing while over fixed-pos
> layers inside an iframe. 

Yeah, I get EXPECTED RESULTS if I visit the iframe'd doc directly: http://audilibrary.audiusa.com/viewer/brochures/17/a4/index.html

Comment 6

[Jim Mathies [:jimm]]

kats, can you take a look?

Comment 7

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

I'm already taking a look.

Comment 8

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

Attachment: Patch

Here's a patch that fixes the problem. I still need to write a test, make sure it doesn't break any existing tests, and check to see if position:sticky suffers from the same problem.

Comment 9

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

Attachment: MozReview Request: Bug 1270955 - When hit-testing on fixed-pos layers, find the root APZC for the scroll container rather than the layers id. r?botond

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

Comment 10

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

Attachment: MozReview Request: Bug 1270955 - Add mochitests for scrolling while over position:fixed and sticky elements. r?botond

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

Comment 11

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

Added a test, verified that position:sticky doesn't have the same problem, and there's a try push on the mozreview going.

Comment 12

[Botond Ballo [:botond]]

Comment on attachment 8751314 [details]
MozReview Request: Bug 1270955 - When hit-testing on fixed-pos layers, find the root APZC for the scroll container rather than the layers id. r?botond

https://reviewboard.mozilla.org/r/51949/#review48791

::: gfx/layers/apz/src/APZCTreeManager.h:471
(Diff revision 1)
>                                           const ParentLayerPoint& aHitTestPoint,
>                                           HitTestResult* aOutHitResult,
>                                           bool* aOutHitScrollbar);
> -  AsyncPanZoomController* FindRootApzcForLayersId(uint64_t aLayersId) const;
> +  // Find the root APZC for the given layers id; if the provided scrollid is
> +  // not NULL_SCROLL_ID, this further filters APZCs to ones that match that
> +  // scrollid.

Based on the implementation, it doesn't look like this is a "further filter", it looks like if a scroll id is specified, this finds the first APZC matching that layers id and scroll id.

Given that, I think it would make sense to have a separate function for this.

Comment 13

[Botond Ballo [:botond]]

Comment on attachment 8751315 [details]
MozReview Request: Bug 1270955 - Add mochitests for scrolling while over position:fixed and sticky elements. r?botond

https://reviewboard.mozilla.org/r/51951/#review48797

Could this have been tested with a gtest? I realize that wouldn't test the entire stack the way a mochitest does, but it would exercise the code being modified by the fix, and gtests have a better track record for intermittent failures than mochitests :)

Comment 14

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

(In reply to Botond Ballo [:botond] from comment #12)
> Based on the implementation, it doesn't look like this is a "further
> filter", it looks like if a scroll id is specified, this finds the first
> APZC matching that layers id and scroll id.
> 
> Given that, I think it would make sense to have a separate function for this.

Sure, I can do that.

(In reply to Botond Ballo [:botond] from comment #13)
> Could this have been tested with a gtest? I realize that wouldn't test the
> entire stack the way a mochitest does, but it would exercise the code being
> modified by the fix, and gtests have a better track record for intermittent
> failures than mochitests :)

It's doable in a gtest, but we'd have to hard-code the layer tree. Which means if we change the layer tree structure then the gtest is liable to become worthless.

Comment 15

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

(That being said, the test is failing on Linux on try, because it's scrolling 12px instead of 10px - I'm debugging)

Comment 16

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

Comment on attachment 8751314 [details]
MozReview Request: Bug 1270955 - When hit-testing on fixed-pos layers, find the root APZC for the scroll container rather than the layers id. r?botond

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

Comment 17

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

Comment on attachment 8751315 [details]
MozReview Request: Bug 1270955 - Add mochitests for scrolling while over position:fixed and sticky elements. r?botond

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

Comment 18

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

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #14)
> (In reply to Botond Ballo [:botond] from comment #12)
> > Based on the implementation, it doesn't look like this is a "further
> > filter", it looks like if a scroll id is specified, this finds the first
> > APZC matching that layers id and scroll id.
> > 
> > Given that, I think it would make sense to have a separate function for this.
> 
> Sure, I can do that.

I discovered we had an existing GetTargetNode function which I could reuse, so I did that.

Updated the test to not require specific scroll offsets, as long as it scrolls that's good enough. The other wheel scroll tests do the same thing, because the actual amount scrolled varies by platform.

Comment 19

[Botond Ballo [:botond]]

Comment on attachment 8751315 [details]
MozReview Request: Bug 1270955 - Add mochitests for scrolling while over position:fixed and sticky elements. r?botond

https://reviewboard.mozilla.org/r/51951/#review48821

::: gfx/layers/apz/test/mochitest/test_scroll_window.html:18
(Diff revision 1)
> +// a new window and waits for it to call testDone()
> +var tests = [
> +  {'file': 'helper_scroll_on_position_fixed.html', 'prefs': [
> +        ['mousewheel.transaction.ignoremovedelay', 0],
> +        ['general.smoothScroll', false],
> +        ['mousewheel.transaction.timeout', 0]]}

Could you say a few words about why these prefs are necessary?

::: gfx/layers/apz/test/mochitest/test_scroll_window.html:24
(Diff revision 1)
> +];
> +
> +var testIndex = -1;
> +var w = null;
> +
> +function testDone() {

I don't have the HTML/JS-fu to be able to suggest how, but is it possible to avoid repeating some of this boilerplate in each test?

::: gfx/layers/apz/test/mochitest/test_scroll_window.html:27
(Diff revision 1)
> +var w = null;
> +
> +function testDone() {
> +  var test = tests[testIndex];
> +  if (w) {
> +    if (!!test.prefs) {

I know we've talked about this before, but based on what I've heard from people (jrmuizel, and I think djvj) since then, using !! in the condition of an if statement is redundant and not idiomatic (because the condition of an if statement is automatically coerced to become a boolean).

Comment 20

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

(In reply to Botond Ballo [:botond] from comment #19)
> > +        ['mousewheel.transaction.ignoremovedelay', 0],
> > +        ['general.smoothScroll', false],
> > +        ['mousewheel.transaction.timeout', 0]]}
> 
> Could you say a few words about why these prefs are necessary?

Yup, will do.

> > +
> > +function testDone() {
> 
> I don't have the HTML/JS-fu to be able to suggest how, but is it possible to
> avoid repeating some of this boilerplate in each test?

Yeah, it should be. I'll do it as part of bug 1264017 because I'd like to keep these patches easy to uplift. Bug 1264017 introduces yet another test that does this sort of "cycle through windows with sub-tests" thing, so it seems just as appropriate to do the refactoring in that bug.

> ::: gfx/layers/apz/test/mochitest/test_scroll_window.html:27
> > +    if (!!test.prefs) {
> 
> I know we've talked about this before, but based on what I've heard from
> people (jrmuizel, and I think djvj) since then, using !! in the condition of
> an if statement is redundant and not idiomatic (because the condition of an
> if statement is automatically coerced to become a boolean).

Fair enough, I'll take it out from this patch. In bug 1264017 when I do the refactoring I'll take it out of the other places as well.

Comment 21

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/fecfcebc50b7
https://hg.mozilla.org/integration/mozilla-inbound/rev/36986a82075f

Comment 22

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/fecfcebc50b7
https://hg.mozilla.org/mozilla-central/rev/36986a82075f

Comment 23

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

I'll give this a few days to bake before requesting uplift to 48.

Comment 24

[Liz Henry (:lizzard) (relman/hg->git project)]

Since this caused a regression in nightly (now fixed in bug 1272429) do you want to uplift this patch and the one in 1272429? Or combine their approaches?

Comment 25

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

Yeah, I think they should be good to uplift now. I'll request approval on the patches.

Comment 26

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

Comment on attachment 8751314 [details]
MozReview Request: Bug 1270955 - When hit-testing on fixed-pos layers, find the root APZC for the scroll container rather than the layers id. r?botond

Approval Request Comment
[Feature/regressing bug #]: APZ
[User impact if declined]: when the mouse is over a fixed-pos layer, wheel-scrolling should scroll the root content, but it doesn't.
[Describe test coverage new/current, TreeHerder]: on m-c for a week. the second patch is a test
[Risks and why]: relatively low risk. there was a regression (bug 1272429) which I will also request uplift approval for.
[String/UUID change made/needed]: none

Comment 27

[Sylvestre Ledru [:Sylvestre]]

Comment on attachment 8751314 [details]
MozReview Request: Bug 1270955 - When hit-testing on fixed-pos layers, find the root APZC for the scroll container rather than the layers id. r?botond

apz & e10s, taking it.

Comment 28

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/b345678b3ef0
https://hg.mozilla.org/releases/mozilla-aurora/rev/cef07b141805