Closed Bug 1298254 Opened 8 years ago Closed 8 years ago

Intermittent gfx/layers/apz/test/mochitest/test_scroll_inactive_flattened_frame.html | nested scrollframe should have scrolled

Categories

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

Product:
Core
Component:
Panning and Zooming
Version:
51 Branch
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla52
Tracking Flags:
Tracking Status
firefox49 --- unaffected
firefox50 --- unaffected
firefox51 --- fixed
firefox52 --- fixed

People

(Reporter: intermittent-bug-filer, Assigned: kats)

References

Details

(Keywords: intermittent-failure, Whiteboard: [gfx-noted])

Attachments

(2 files)

Bug 1298254 - Ensure that the flush-apz-repaints codepath also waits for pending SetTargetAPZCNotification messages on the controller thread before returning.
58 bytes, text/x-review-board-request
dvander
: review+
Details
Bug 1298254 - Cleanup patch to replace a class with NewRunnableMethod.
58 bytes, text/x-review-board-request
dvander
: review+
gchang
: approval-mozilla-aurora+
Details
Priority: -- → P3
Whiteboard: [gfx-noted]
Version: unspecified → 51 Branch
This may or may not be a regression from bug 1286179, and may be fixed by bug 1298401. Not enough data to really tell yet but I'll mark it as a dependency so I can track it better.
Depends on: 1298401
This doesn't appear to be fixed by 1298401, it shows up in my try push at [1] which has that fix.

[1] https://treeherder.mozilla.org/#/jobs?repo=try&revision=b2f9d5411f9e&selectedJob=26519226
No longer depends on: 1298401
Looking at when it started it's likely to be a regression from bug 1289650.
Blocks: 1289650
I think the related bugs seem to suffer from the same root cause - in all cases sendWheelAndPaint is returning before the layerization has taken effect and the wheel event is processed on the APZ side.
See Also: → 1299537, 1298252
Looks like the same underlying problem as I described in bug 1256128 comment 26 - the flushApzRepaints can "overtake" the SetTargetAPZCNotification call, and so the callback passed to the flush can run before the wheel event is done being processed.
Just to be more explicit about how bug 1289650 regressed this, here's the before and after:

Before bug 1289650:
- SetTargetAPZCNotification arrived on the compositor thread, it was redispatched to the controller thread (the main thread), and then the repaint request resulting from it was sent out on the main thread.
- flushApzRepaints arrived on the compositor thread, and the response was posted back on the main thread
So, as long as flushApzRepaints call was made after the target APZC notification, the response would always arrive back to the content process after the repaint request.

After bug 1289650:
- SetTargetAPZCNotification arrived on the compositor thread, it was redispatched to the controller thread (the main thread), and then the repaint request resulting from it was posted back to the compositor thread.
- flushApzRepaints arrived on the compositor thread, and the response was posted back onto the compositor thread
So in this case, even if the flushApzRepaints call was made after the target APZC notification, the response could arrive back sooner, because it wouldn't stop and wait for the "main thread" bit of the target APZC notification.

My patch corrects this by making the flushApzRepaints post to the controller thread, the same as the SetTargetAPZCNotification. So now they follow the same path again, and so the old guarantee is maintained.
Blocks: 1298252
See Also: 1298252 → 
Comment on attachment 8794395 [details]
Bug 1298254 - Ensure that the flush-apz-repaints codepath also waits for pending SetTargetAPZCNotification messages on the controller thread before returning.

https://reviewboard.mozilla.org/r/80872/#review79540
Attachment #8794395 - Flags: review?(dvander) → review+
Comment on attachment 8794396 [details]
Bug 1298254 - Cleanup patch to replace a class with NewRunnableMethod.

https://reviewboard.mozilla.org/r/80874/#review79542
Attachment #8794396 - Flags: review?(dvander) → review+
Pushed by kgupta@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/cc20cf366866
Ensure that the flush-apz-repaints codepath also waits for pending SetTargetAPZCNotification messages on the controller thread before returning. r=dvander
https://hg.mozilla.org/integration/autoland/rev/41af6593a975
Cleanup patch to replace a class with NewRunnableMethod. r=dvander
https://hg.mozilla.org/mozilla-central/rev/cc20cf366866
https://hg.mozilla.org/mozilla-central/rev/41af6593a975
Status: NEW → RESOLVED
Closed: 8 years ago
status-firefox52: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla52
OrangeFactor is looking good, no new failures since the patches landed. Will request uplift.
Actually since the first patch (which is the actual fix) is only exercised in test code, I'll uplift with a=test-only. I'll leave the second patch (which just a simple refactoring) on m-c.
Comment on attachment 8794396 [details]
Bug 1298254 - Cleanup patch to replace a class with NewRunnableMethod.

Approval Request Comment
[Feature/regressing bug #]: bug 1289650
[User impact if declined]: none. there were test failures caused by bug 1289650 and i uplifted the fix (which is the other patch) already as a=test-only. This is a ride-along refactoring that RyanVM suggested I uplift as well to make future uplifts less conflict-prone
[Describe test coverage new/current, TreeHerder]: there are automated tests exercising this code already
[Risks and why]: no risk, simple refactoring
[String/UUID change made/needed]: none
Attachment #8794396 - Flags: approval-mozilla-aurora?
Comment on attachment 8794396 [details]
Bug 1298254 - Cleanup patch to replace a class with NewRunnableMethod.

Fix a test failure related to APZ. Take it in 51 aurora.
Attachment #8794396 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: