See https://treeherder.mozilla.org/#/jobs?repo=try&revision=8739d63c549e - test_layerization (in M4) is failing all over the place when I flip the APZ pref to true. (You can ignore the non-desktop platforms here, and perhaps even the non-e10s ones. But I was still expecting it to pass in M-e10s on Linux).

Looks like there are at least two issues here: - The div 'outer1' is being unexpectedly layerized upon page load. I verified that it happens due to the "layerize the primary async scrollable frame" optimization. I'm not sure why this optimization is firing - I'm explicitly guarding against it by giving making the page content tall enough to make it scrollable (and thereby make the *page* be the PASF). I can't reproduce this failure locally. - The test is timing out due to a wheel event being wrongly targeted. I suspect this is a coordinate systems issue, and that whether the numbers are wrong enough to cause a failure depends on the screen size. I can reproduce it locally on my laptop (which has a smaller screen than my desktop which is where I was testing before landing). I'm continuing to investigate on both fronts.

(In reply to Botond Ballo [:botond] from comment #1) > - The test is timing out due to a wheel event being wrongly targeted. > I suspect this is a coordinate systems issue, and that whether the > numbers are wrong enough to cause a failure depends on the screen > size. I can reproduce it locally on my laptop (which has a smaller > screen than my desktop which is where I was testing before landing). It's not a coordinate systems issue. The wrong targeting is caused by the presence of an event-regions layer in an unexpected position in the layer tree. Timothy thinks it might be caused by a bug in FrameLayerBuilder.

(In reply to Botond Ballo [:botond] from comment #1) > - The div 'outer1' is being unexpectedly layerized upon page load. > I verified that it happens due to the "layerize the primary > async scrollable frame" optimization. I'm not sure why this > optimization is firing - I'm explicitly guarding against it by > giving making the page content tall enough to make it scrollable > (and thereby make the *page* be the PASF). I can't reproduce this > failure locally. The problem is that the page content is contained inside an iframe marked with 'scrolling="no"' [1], which prevents it from being scrollable and thus disqualifies it from being the target of the "layerize the primary async scrollable frame" optimization. The reason I couldn't reproduce the failure locally was that when running locally, the *enclosing* top-level document was scrollable, and so *that* was the target of the optimization, but in infra the top-level document is not scrollable. This can be worked around fairly easily by introducing an additional scrollable div grouping the ones being tested to catch the optimization. Alternatively, Timothy suggested removing the 'scrolling="no"' marking from the mochitest harness. [1] http://mxr.mozilla.org/mozilla-central/source/testing/mochitest/harness.xul?rev=e63758ec19f8#85 (In reply to Botond Ballo [:botond] from comment #2) > (In reply to Botond Ballo [:botond] from comment #1) > > - The test is timing out due to a wheel event being wrongly targeted. > > I suspect this is a coordinate systems issue, and that whether the > > numbers are wrong enough to cause a failure depends on the screen > > size. I can reproduce it locally on my laptop (which has a smaller > > screen than my desktop which is where I was testing before landing). > > It's not a coordinate systems issue. The wrong targeting is caused by the > presence of an event-regions layer in an unexpected position in the layer > tree. Timothy thinks it might be caused by a bug in FrameLayerBuilder. I wrote a patch with Timothy's help that fixes this locally, but it's not proper enough to land. Will post anyways for the record.

Bug 1177018 - Work around the iframe in which mochitests are run not being scrollable. r=kats

Bug 1177018 - Include event regions in a layer's visible region. r=tn

Here's a Try push with these two patches: https://treeherder.mozilla.org/#/jobs?repo=try&revision=c84e1bfc5cea It's still failing, but later in the test than the previous failures, so I guess there are additional issues.

(In reply to Botond Ballo [:botond] from comment #6) > It's still failing, but later in the test than the previous failures, so I > guess there are additional issues. I'm continuing to investigate this. Annoyingly, the test failures disappear when I add logging, so either my logging is inadvertently changing functionality, or changing timings...

OK, it looks like the remaining problem is the same one that made me have to disable smooth scrolling in test_wheel_transactions: smooth scrolling kicks off an "async" scroll animation that causes scroll events for a wheel event to arrive well after subsequent wheel events. Disabling smooth scrolling, in addition to the two fixes posted so far, seems to make the test pass consistently: https://treeherder.mozilla.org/#/jobs?repo=try&revision=5d0d1b4d1e6f

(In reply to Botond Ballo [:botond] from comment #8) > Disabling smooth scrolling, in addition to the two fixes posted so far, > seems to make the test pass consistently: > https://treeherder.mozilla.org/#/jobs?repo=try&revision=5d0d1b4d1e6f *Sigh*. The same fixes, with logging removed, and the failures are back: https://treeherder.mozilla.org/#/jobs?repo=try&revision=71072a318bc4

I think I'm making some headway here: I think the reason for the difference between the above two try runs is not the logging, but that one only runs APZ tests (with '--tag apz') while the other runs all mochitests. I'm finding that in the failing case, calls to isLayerized() take on the order of 20-30 seconds, and so in aggregate they result in the test exceeding its 5-minute time limit. I believe the reason these calls take so long is that over a long series of mochitest runs, a lot of APZ test data accumulates (on the order of 8000 paints' worth). The reason this much data accumulates is that APZ test logging is enabled for all tests, not just APZ tests [1]. Restricting the logging to just APZ tests should solve this problem. Kats suggested doing this by keeping the apz.test.logging_enabled pref off in general, and only turning it on inside tests that need it. The problem is that SpecialPowers.pushPrefEnv() only affects the content process in which the mochitest runs, but in e10s mode we need to change the pref in the chrome process as well (because that's where the compositor-side APZ tests data is collected). My best idea for how to do this is to introduce an nsIDOMWindowUtils API for turning APZ test data collection on and off on both the content and compositor sides (with the content side communicating the request to the compositor side via PLayerTransaction, similar to how the data is queried). If anyone has a better suggestion, I'm open to one. [1] http://mxr.mozilla.org/mozilla-central/source/testing/profiles/prefs_general.js?rev=75bc56b719ce#252

It might be better to add something in http://mxr.mozilla.org/mozilla-central/source/testing/specialpowers/content/specialpowersAPI.js to be able to send pref change requests to the parent process. I'm sure it would be handy in other tests as well.

(In reply to Botond Ballo [:botond] from comment #10) > The problem is that SpecialPowers.pushPrefEnv() only affects the content > process in which the mochitest runs, but in e10s mode we need to change the > pref in the chrome process as well (because that's where the compositor-side > APZ tests data is collected). It turns out I was mistaken - pushPrefEnv *does* set the prefs in the parent process! My actual problem was that the pref I was setting was 'Once' in gfxPrefs, and so the consumers weren't picking up the change. Doh!

Here's a Try push with a fix that restricts the APZ test data collection to tests that actually need it: https://treeherder.mozilla.org/#/jobs?repo=try&revision=c7384f4feab2 The 30-second long isLayerized() calls and timeouts caused by them are gone, but there are still a few remaining intermittent timeouts, where it seems that an event we are waiting for (a mouse move after synthesizing one, or a scroll event after synthesizing a wheel scroll) never arrives.

Progress update: I'm continuing to debug the remaining low-volume intermittent failure by pushing patches with various logging added to Try. The most recent logs indicate that APZ hit testing is intermittently hitting the wrong APZC - the page's APZC, rather than the subframe we intend to scroll; since the page's APZC can't scroll, no 'scroll' event is fired and the test waits around until it times out.

Comment on attachment 8626810 [details] MozReview Request: Bug 1177018 - When finding a painted layer for a display item, include event regions in a layer's visible region. r=tn I don't think this patch can land as is, as it would lead to larger visible regions for layers.

(In reply to Timothy Nikkel (:tn) from comment #15) > Comment on attachment 8626810 [details] > MozReview Request: Bug 1177018 - Include event regions in a layer's visible > region. r=tn > > I don't think this patch can land as is, as it would lead to larger visible > regions for layers. Right. (I said as much in comment 3.) Do you have an idea of what a proper fix would look like?

Sorry for missing that. Two possible ways: 1) make the visible region of PaintedLayerData's include the event regions, and then separately keep track of the actual visible region we want to set on the layer (that doesn't include event regions). 2) keep track of the area covered by all event regions in a PaintedLayerData, make PaintedLayerDataNode::FindPaintedLayerFor consider the union of the visible region and the area covered by event regions when choosing a layer.

After doing some more Try pushes with more logging, there seem to be three different intermittent issues remaining: (1) Sometimes APZ hit testing for a wheel event incorrectly hits the page's APZC with HitLayer as the result, in spite of the content-side hit test giving a different result. The results of the content-side hit test are sent to APZ via the SetConfirmedTargetAPZC notification, but by that time the event has already been processed. (2) Sometimes APZ hit testing for a wheel event correctly hits the container div's APZC with HitDispatchToContent as the result, but it takes more than 300 ms to dispatch the event to content, wait for the next paint, and send the SetConfirmedTargetAPZC notification back to APZ; by the time that notification arrives, APZ already timed out and dispatched the wheel event to the tentative target. Both (1) and (2) cause the 'scroll' event the test expects the wheel event to trigger, to never be fired (because the wrong APZC is being scrolled). (3) A synthesized mouse event never makes it back to content. I haven't had a chance to investigate this further, as I was focusing on (1) and (2) which are higher- volume. In my latest Try push [1]: (1) caused 4/4 failures in the opt build (out of a total 20 runs) (2) caused 3/4 failures in the debug build (total 20 runs) (3) caused 1/4 failures in the debug build [1] https://treeherder.mozilla.org/#/jobs?repo=try&revision=14bf1701b95e

(In reply to Botond Ballo [:botond] from comment #18) > (1) Sometimes APZ hit testing for a wheel event incorrectly hits the page's APZC > with HitLayer as the result It looks like the clip rect of the layers belonging to the APZC of the 'container' div (which is a child of the page's APZC) is incorrect in the failing cases. In the passing cases, the clip rect is (21,304,470,486); in the failing cases it's (10,152,236,243), a factor of 2 smaller. As a result of the clip rect being too small, hit testing doesn't hit the container, falling back on the page instead. I'm not sure yer where the wrong value of the clip rect comes from. > (2) Sometimes APZ hit testing for a wheel event correctly hits the container div's > APZC with HitDispatchToContent as the result, but it takes more than 300 ms > to dispatch the event to content, wait for the next paint, and send the > SetConfirmedTargetAPZC notification back to APZ; by the time that notification > arrives, APZ already timed out and dispatched the wheel event to the tentative > target. This problem went away after increasing the content response timeout to 3 seconds.

(In reply to Botond Ballo [:botond] from comment #19) > In the passing cases, the clip rect is (21,304,470,486); in the failing > cases it's (10,152,236,243), a factor of 2 smaller. > > [...] > > I'm not sure yer where the wrong value of the clip rect comes from. It looks like the page is zoomed to 0.5x for some reason.

(In reply to Botond Ballo [:botond] from comment #20) > (In reply to Botond Ballo [:botond] from comment #19) > > In the passing cases, the clip rect is (21,304,470,486); in the failing > > cases it's (10,152,236,243), a factor of 2 smaller. > > > > [...] > > > > I'm not sure yer where the wrong value of the clip rect comes from. > > It looks like the page is zoomed to 0.5x for some reason. This intermittent has gone away after updating to a tree that includes the fix for bug 1178847! (Thanks for suggesting to check this, Kats.) That just leaves the lost mouse move issue.

(In reply to Botond Ballo [:botond] from comment #21) > That just leaves the lost mouse move issue. By running the test in chaos mode, I was finally able to repro this with sufficient logging to track down the cause: - In addition to the mouse move event generated by the test, the parent process sometimes also generates mouse move events in PresShell::ProcessSynthMouseMoveEvent() [1] (which are then dispatched on a refresh driver tick [2]). Based on this comment [3], the purpose of these events appears to be to notify content of the mouse being over different content as a result of the content moving or scrolling under it. - Mouse move events generated by the above mechanism have their |reason| field set to |WidgetMouseEvent::eSynthesized| [4]. (By contrast, the event the test generates has the field set to |WidgetMouseEvent::eReal|). - Mouse events with |reason == eSynthesized| do not generate DOM events [5]. - Both kinds of mouse events are sent to the child process via the PBrowser::RealMouseMoveEvent message. Thanks to IPDL compression [6], if an |eSynthesized| mouse move event is generated sufficiently quickly after my |eReal| mouse move event that IPDL hasn't yet sent the |eReal| one when the |eSynthesized| one is queued up, the |eReal| one can be dropped in favour of the |eSynthesized| one, resulting in no DOM mouse move event ever being received by the content. I discussed this with :smaug on IRC, and he suggested using a different IPDL message to send |eSynthesized| events, to avoid compression across the two event types. (Moreover, he said |eSynthesized| events don't need to be compressed at all because they are dispatched rarely.) Here's an updated Try push that includes this fix: [7]. Fingers crossed! [1] http://mxr.mozilla.org/mozilla-central/source/layout/base/nsPresShell.cpp?rev=9bca608ab65a#5439 [2] http://mxr.mozilla.org/mozilla-central/source/layout/base/nsPresShell.cpp?rev=9bca608ab65a#5597 [3] http://mxr.mozilla.org/mozilla-central/source/layout/base/nsIPresShell.h?rev=9bca608ab65a#1449 [4] http://mxr.mozilla.org/mozilla-central/source/layout/base/nsPresShell.cpp?rev=9bca608ab65a#5589 [5] http://mxr.mozilla.org/mozilla-central/source/widget/WidgetEventImpl.cpp?rev=94eb248b77b0#231 [6] http://mxr.mozilla.org/mozilla-central/source/dom/ipc/PBrowser.ipdl?rev=7e72681dff59#623 [7] https://treeherder.mozilla.org/#/jobs?repo=try&revision=514d6fe6c50e

Woah that's quite the subtle bug. Thanks for digging into it!

(In reply to Botond Ballo [:botond] from comment #22) > Here's an updated Try push that includes this fix: [7]. Fingers crossed! Looking green! Also Tried with all the logging removed [1], as that brought back failures before, but that's clean too! To summarize, there were six different issues with the test, with six different fixes: (1) Tweaking the test page's structure so that regardless of the size of the window the mochitest runs in, the same scrollable frame is layerized as the "primary async scrollable frame". (2) Fixing a bug in FrameLayerBuilder related to computing event-regions. (3) Disabling smooth scrolling to avoid scroll events from long-running smooth scroll animations from interfering with future test actions. (4) Restricting APZ test logging to just the tests that need it to avoid operations on excessively large test data structures exceeding the test's time limit. (5) Increasing the APZ content response timeout to make sure we're always scrolling the confirmed target APZ. (6) The IPDL compression issue described in comment 22. One more thing remains to be done: the current fix for (2) is not landable as-is, and needs to be reworked as described in comment 17. [1] https://treeherder.mozilla.org/#/jobs?repo=try&revision=41a9ed6f94a7

(In reply to Botond Ballo [:botond] from comment #24) > One more thing remains to be done: the current fix for (2) is not landable > as-is, and needs to be reworked as described in comment 17. I implemented approach (2) from that comment, and cleaned up my patch series containing the various fixes. Try pushes: https://treeherder.mozilla.org/#/jobs?repo=try&revision=401841da8866 https://treeherder.mozilla.org/#/jobs?repo=try&revision=a5792d7c9d0b

(In reply to Botond Ballo [:botond] from comment #25) > Try pushes: > > https://treeherder.mozilla.org/#/jobs?repo=try&revision=401841da8866 > https://treeherder.mozilla.org/#/jobs?repo=try&revision=a5792d7c9d0b Looking good! Posting patch series for review.

Comment on attachment 8626809 [details] MozReview Request: Bug 1177018 - Work around the iframe in which mochitests are run not being scrollable. r=kats Bug 1177018 - Work around the iframe in which mochitests are run not being scrollable. r=kats

Comment on attachment 8626810 [details] MozReview Request: Bug 1177018 - When finding a painted layer for a display item, include event regions in a layer's visible region. r=tn Bug 1177018 - When finding a painted layer for a display item, include event regions in a layer's visible region. r=tn

Bug 1177018 - Disable smooth scrolling in the APZ layerization test. r=kats

Bug 1177018 - Only enable APZ test logging for tests that actually use it. r=kats This avoids excessive amounts of test data accumulating which can slow tests down. As part of this change, the pref for enabling the logging was made 'Live'.

Bug 1177018 - Increase the APZ content response timeout to 15 seconds for all tests. r=kats It has previously been increased for a specific test, but we have found another test that needs it increased. Rather than increasing it for individual tests, increase it for all tests.

Bug 1177018 - Send mouse move events generated via nsIPresShell::SynthesizeMouseMove() to the child process through a different IPDL message than real mouse move events. r=smaug This avoids a real event being dropped in favour of a synthesized event via IPDL compression, which is important because synthesized events don't generate 'mousemove' DOM events.

Bug 1177018 - Enable chaos mode for the APZ layerization test. r=kats

The first six patches fix the six issues described in comment 24. The fourth enables chaos mode for the test, for good measure.

Comment on attachment 8626809 [details] MozReview Request: Bug 1177018 - Work around the iframe in which mochitests are run not being scrollable. r=kats https://reviewboard.mozilla.org/r/12141/#review13025

Comment on attachment 8640875 [details] MozReview Request: Bug 1177018 - Disable smooth scrolling in the APZ layerization test. r=kats https://reviewboard.mozilla.org/r/14381/#review13027 Ship It!

Comment on attachment 8640876 [details] MozReview Request: Bug 1177018 - Only enable APZ test logging for tests that actually use it. r=kats https://reviewboard.mozilla.org/r/14383/#review13029 Ship It!

Comment on attachment 8640877 [details] MozReview Request: Bug 1177018 - Increase the APZ content response timeout to 15 seconds for all tests. r=kats https://reviewboard.mozilla.org/r/14385/#review13031 Ship It!

https://reviewboard.mozilla.org/r/14387/#review13033 ::: dom/ipc/PBrowser.ipdl:631 (Diff revision 1) > + RealSynthMouseMoveEvent(WidgetMouseEvent event); I'd rename this to just "SynthMouseMoveEvent". To me "Real" != "Synth" so putting both is paradoxical.

Comment on attachment 8640879 [details] MozReview Request: Bug 1177018 - Enable chaos mode for the APZ layerization test. r=kats https://reviewboard.mozilla.org/r/14389/#review13035 Ship It!

Comment on attachment 8626810 [details] MozReview Request: Bug 1177018 - When finding a painted layer for a display item, include event regions in a layer's visible region. r=tn Do you remember the specific situation that we ran into? I recall vaguely but I'd like to remember in detail to make sure we are solving the problem fully.

I gave details over IRC, but for the record I will post them here as well. The attached hit testing tree demonstrates the problem. The page being rendered is this one [1]. In short, the page contains a scrollable div named 'container' that groups everything, and inside it four scrollable subframes, side by side; the first two are divs, the second two are iframes (and each has a scrollable subframe of its own). The problem occurs when tapping on the region of the first iframe (the third scrollable subframe out of the four). The iframe hasn't been layerized yet, so I would expect hit testing to hit the dispatch-to-content region of the parent scrollable, which is 'container'. In the attachment, { l=2, p=4985, v=122 } is the guid of 'container'. The problem is that there is a layer corresponding to it, with hit testing node 0x7ff865d58e80, which is on top of basically everything else, and which has a hit-region (only; no dispatch-to-content region) that covers the area of the first iframe. This layer is eating events destined for the first iframe. With the fix, the layer in question also has a dispatch-to-content region over the area of the first iframe, and the event hits that as desired. Let me know if this is enough detail or if there is anything else I should provide. [1] http://people.mozilla.org/~bballo/test_layerization.html

Comment on attachment 8626810 [details] MozReview Request: Bug 1177018 - When finding a painted layer for a display item, include event regions in a layer's visible region. r=tn The visible above region consists of content in layers above the current one. So we need to Or it with the hit regions of those same layers, not the hit region of the current layer. So I think we'll need to keep track of an "above hit region" to do this, similar to how we do for visible above region.

(In reply to Timothy Nikkel (:tn) from comment #43) > The visible above region consists of content in layers above the current > one. So we need to Or it with the hit regions of those same layers, not the > hit region of the current layer. So I think we'll need to keep track of an > "above hit region" to do this, similar to how we do for visible above region. After discussing this with Timothy, it turns out we don't need Or the visible-above region with anything. Since layers can be moved around relative to each other by the compositor, we have to use clip rects rather than visible regions when computing the visible-above region; similarly, any "hit-above" region that we would copmute would also have to consist of clip rects, but those are already in the visible-above region, so there's no need to Or anything.

Comment on attachment 8626809 [details] MozReview Request: Bug 1177018 - Work around the iframe in which mochitests are run not being scrollable. r=kats Bug 1177018 - Work around the iframe in which mochitests are run not being scrollable. r=kats

Comment on attachment 8626810 [details] MozReview Request: Bug 1177018 - When finding a painted layer for a display item, include event regions in a layer's visible region. r=tn Bug 1177018 - When finding a painted layer for a display item, include event regions in a layer's visible region. r=tn

Comment on attachment 8640875 [details] MozReview Request: Bug 1177018 - Disable smooth scrolling in the APZ layerization test. r=kats Bug 1177018 - Disable smooth scrolling in the APZ layerization test. r=kats

Comment on attachment 8640876 [details] MozReview Request: Bug 1177018 - Only enable APZ test logging for tests that actually use it. r=kats Bug 1177018 - Only enable APZ test logging for tests that actually use it. r=kats This avoids excessive amounts of test data accumulating which can slow tests down. As part of this change, the pref for enabling the logging was made 'Live'.

Comment on attachment 8640877 [details] MozReview Request: Bug 1177018 - Increase the APZ content response timeout to 15 seconds for all tests. r=kats Bug 1177018 - Increase the APZ content response timeout to 15 seconds for all tests. r=kats It has previously been increased for a specific test, but we have found another test that needs it increased. Rather than increasing it for individual tests, increase it for all tests.

Comment on attachment 8640878 [details] MozReview Request: Bug 1177018 - Send mouse move events generated via nsIPresShell::SynthesizeMouseMove() to the child process through a different IPDL message than real mouse move events. r=smaug Bug 1177018 - Send mouse move events generated via nsIPresShell::SynthesizeMouseMove() to the child process through a different IPDL message than real mouse move events. r=smaug This avoids a real event being dropped in favour of a synthesized event via IPDL compression, which is important because synthesized events don't generate 'mousemove' DOM events.

Comment on attachment 8640879 [details] MozReview Request: Bug 1177018 - Enable chaos mode for the APZ layerization test. r=kats Bug 1177018 - Enable chaos mode for the APZ layerization test. r=kats

Comment on attachment 8640878 [details] MozReview Request: Bug 1177018 - Send mouse move events generated via nsIPresShell::SynthesizeMouseMove() to the child process through a different IPDL message than real mouse move events. r=smaug Updated the Part 2 patch to make this change. Carrying r+ for everything else.

Comment on attachment 8626810 [details] MozReview Request: Bug 1177018 - When finding a painted layer for a display item, include event regions in a layer's visible region. r=tn I think you need to use ScaleToOutsidePixels so that the scale is included, instead of just ToNearestPixels. Also because it gets highlighted in red in mozreview I feel I have to point out the trailing spaces. :)

Comment on attachment 8626810 [details] MozReview Request: Bug 1177018 - When finding a painted layer for a display item, include event regions in a layer's visible region. r=tn Bug 1177018 - When finding a painted layer for a display item, include event regions in a layer's visible region. r=tn

Comment on attachment 8640875 [details] MozReview Request: Bug 1177018 - Disable smooth scrolling in the APZ layerization test. r=kats Bug 1177018 - Disable smooth scrolling in the APZ layerization test. r=kats

Comment on attachment 8640876 [details] MozReview Request: Bug 1177018 - Only enable APZ test logging for tests that actually use it. r=kats Bug 1177018 - Only enable APZ test logging for tests that actually use it. r=kats This avoids excessive amounts of test data accumulating which can slow tests down. As part of this change, the pref for enabling the logging was made 'Live'.

Comment on attachment 8640877 [details] MozReview Request: Bug 1177018 - Increase the APZ content response timeout to 15 seconds for all tests. r=kats Bug 1177018 - Increase the APZ content response timeout to 15 seconds for all tests. r=kats It has previously been increased for a specific test, but we have found another test that needs it increased. Rather than increasing it for individual tests, increase it for all tests.

Comment on attachment 8640878 [details] MozReview Request: Bug 1177018 - Send mouse move events generated via nsIPresShell::SynthesizeMouseMove() to the child process through a different IPDL message than real mouse move events. r=smaug Bug 1177018 - Send mouse move events generated via nsIPresShell::SynthesizeMouseMove() to the child process through a different IPDL message than real mouse move events. r=smaug This avoids a real event being dropped in favour of a synthesized event via IPDL compression, which is important because synthesized events don't generate 'mousemove' DOM events.

Comment on attachment 8640879 [details] MozReview Request: Bug 1177018 - Enable chaos mode for the APZ layerization test. r=kats Bug 1177018 - Enable chaos mode for the APZ layerization test. r=kats

Comment on attachment 8640878 [details] MozReview Request: Bug 1177018 - Send mouse move events generated via nsIPresShell::SynthesizeMouseMove() to the child process through a different IPDL message than real mouse move events. r=smaug Updated the Part 2 patch to address comment 53. I also updated the part 6 patch to address comment 39, which I've been meaning to do for a while. Carrying r+ for that.

(In reply to Botond Ballo [:botond] from comment #3) > The problem is that the page content is contained inside an iframe marked > with 'scrolling="no"' [1], which prevents it from being scrollable and thus > disqualifies it from being the target of the "layerize the primary async > scrollable frame" optimization. > > The reason I couldn't reproduce the failure locally was that when running > locally, the *enclosing* top-level document was scrollable, and so *that* > was the target of the optimization, but in infra the top-level document is > not scrollable. > > This can be worked around fairly easily by introducing an additional > scrollable div grouping the ones being tested to catch the optimization. > Alternatively, Timothy suggested removing the 'scrolling="no"' marking from > the mochitest harness. > > [1] > http://mxr.mozilla.org/mozilla-central/source/testing/mochitest/harness. > xul?rev=e63758ec19f8#85 I filed bug 1199023 for this.

https://hg.mozilla.org/integration/mozilla-inbound/rev/c2a30b4f3a7c https://hg.mozilla.org/integration/mozilla-inbound/rev/8e54cc5e6901 https://hg.mozilla.org/integration/mozilla-inbound/rev/486f9a11fcbd https://hg.mozilla.org/integration/mozilla-inbound/rev/112ec6bdaf4e https://hg.mozilla.org/integration/mozilla-inbound/rev/dbb14a5cd2cd https://hg.mozilla.org/integration/mozilla-inbound/rev/a82c39bb97ab https://hg.mozilla.org/integration/mozilla-inbound/rev/40fdd735520d

Backed out in https://hg.mozilla.org/integration/mozilla-inbound/rev/b04dc4d776f9 for b2g debug mochitest-21 assertion failures like https://treeherder.mozilla.org/logviewer.html#?job_id=13350705&repo=mozilla-inbound

The failures in question are this assertion [1] being trigerred on the compositor side. Clearly related to the Part 4 patch, which restricted APZ test logging to just the tests that need it. I have a theory for why things go wrong: - We only increment the paint sequence number on the client side if the logging pref is enabled. Therefore, two transactions could have the same sequence number if the pref is not enabled yet. - On the compositor side, UpdateHitTestingTree() also only calls StartNewPaint() if the logging pref is enabled. - When we set the pref in the test with pushEnvPref(), that takes effect in the parent process slightly earlier than in the child process. Therefore, two transactions for which we didn't increment the sequence number on the client side, could still have StartNewPaint() called for them on the compositor side. [1] https://dxr.mozilla.org/mozilla-central/rev/f61c3cc0eb8b7533818e7379ccc063b611015d9d/gfx/layers/apz/testutil/APZTestData.h#46

Try pushes to test this theory: https://treeherder.mozilla.org/#/jobs?repo=try&revision=0f417bf042ca https://treeherder.mozilla.org/#/jobs?repo=try&revision=796cb02e5820

(In reply to Botond Ballo [:botond] from comment #65) > Try pushes to test this theory: > > https://treeherder.mozilla.org/#/jobs?repo=try&revision=0f417bf042ca > https://treeherder.mozilla.org/#/jobs?repo=try&revision=796cb02e5820 Looks like I was right. Will post an updated Part 4 patch for review.

Comment on attachment 8626809 [details] MozReview Request: Bug 1177018 - Work around the iframe in which mochitests are run not being scrollable. r=kats Bug 1177018 - Work around the iframe in which mochitests are run not being scrollable. r=kats

Comment on attachment 8626810 [details] MozReview Request: Bug 1177018 - When finding a painted layer for a display item, include event regions in a layer's visible region. r=tn Bug 1177018 - When finding a painted layer for a display item, include event regions in a layer's visible region. r=tn

Comment on attachment 8640875 [details] MozReview Request: Bug 1177018 - Disable smooth scrolling in the APZ layerization test. r=kats Bug 1177018 - Disable smooth scrolling in the APZ layerization test. r=kats

Comment on attachment 8640876 [details] MozReview Request: Bug 1177018 - Only enable APZ test logging for tests that actually use it. r=kats Bug 1177018 - Only enable APZ test logging for tests that actually use it. r=kats This avoids excessive amounts of test data accumulating which can slow tests down. As part of this change, the pref for enabling the logging was made 'Live'.

Comment on attachment 8640877 [details] MozReview Request: Bug 1177018 - Increase the APZ content response timeout to 15 seconds for all tests. r=kats Bug 1177018 - Increase the APZ content response timeout to 15 seconds for all tests. r=kats It has previously been increased for a specific test, but we have found another test that needs it increased. Rather than increasing it for individual tests, increase it for all tests.

Comment on attachment 8640878 [details] MozReview Request: Bug 1177018 - Send mouse move events generated via nsIPresShell::SynthesizeMouseMove() to the child process through a different IPDL message than real mouse move events. r=smaug Bug 1177018 - Send mouse move events generated via nsIPresShell::SynthesizeMouseMove() to the child process through a different IPDL message than real mouse move events. r=smaug This avoids a real event being dropped in favour of a synthesized event via IPDL compression, which is important because synthesized events don't generate 'mousemove' DOM events.

Comment on attachment 8640879 [details] MozReview Request: Bug 1177018 - Enable chaos mode for the APZ layerization test. r=kats Bug 1177018 - Enable chaos mode for the APZ layerization test. r=kats

Comment on attachment 8626810 [details] MozReview Request: Bug 1177018 - When finding a painted layer for a display item, include event regions in a layer's visible region. r=tn MozReview just never seems to know when to carry the r+ and when not to...

Comment on attachment 8640876 [details] MozReview Request: Bug 1177018 - Only enable APZ test logging for tests that actually use it. r=kats This is the updated patch I'd like re-reviewed; the only change compared to the last version is the change to ClientLayerManager.cpp.

Comment on attachment 8640878 [details] MozReview Request: Bug 1177018 - Send mouse move events generated via nsIPresShell::SynthesizeMouseMove() to the child process through a different IPDL message than real mouse move events. r=smaug Carrying r+.

For good measure, here's a green-looking full try push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=93ccae52414c

I don't know what's up with mozreview but the review state there doesn't match the state of attachments here.

(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #78) > I don't know what's up with mozreview but the review state there doesn't > match the state of attachments here. Ah - I guess the problem is that :smaug and :tn granted r+ by setting the flag in bugzilla directly, rather than by doing "Ship It!" in MozReview (which then automatically sets the flag). As a result, MozReview thinks they haven't granted review yet, and keeps setting the bugzilla flag to 'r?' every time I reupload the attachment.

Yeah, that happens rather often. MozReview doesn't yet provide better UX for my reviewing habits so I just use 'Download diff' and look at the patch in an editor and then give r-/+ in bugzilla.

https://hg.mozilla.org/integration/mozilla-inbound/rev/332f5423eb08 https://hg.mozilla.org/integration/mozilla-inbound/rev/407a3696279a https://hg.mozilla.org/integration/mozilla-inbound/rev/8849d2f768c7 https://hg.mozilla.org/integration/mozilla-inbound/rev/461a8d8b1b1c https://hg.mozilla.org/integration/mozilla-inbound/rev/c36d0b61a7fb https://hg.mozilla.org/integration/mozilla-inbound/rev/28f8a5676d8a https://hg.mozilla.org/integration/mozilla-inbound/rev/2e4a1822a511

https://hg.mozilla.org/mozilla-central/rev/332f5423eb08 https://hg.mozilla.org/mozilla-central/rev/407a3696279a https://hg.mozilla.org/mozilla-central/rev/8849d2f768c7 https://hg.mozilla.org/mozilla-central/rev/461a8d8b1b1c https://hg.mozilla.org/mozilla-central/rev/c36d0b61a7fb https://hg.mozilla.org/mozilla-central/rev/28f8a5676d8a https://hg.mozilla.org/mozilla-central/rev/2e4a1822a511