Closed Bug 1210057 Opened 10 years ago Closed 10 years ago

Intermittent test_moz_mouse_pixel_scroll_event.html | prepareScrollUnits: gScrollable96.wheelHorizontalLine may be illegal value, got 10

Categories

(Core :: Panning and Zooming, defect)

Product:
Core
Component:
Panning and Zooming
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla47
Tracking Flags:
Tracking Status
firefox45 --- unaffected
firefox46 --- fixed
firefox47 --- fixed
firefox-esr45 --- unaffected

People

(Reporter: KWierso, Assigned: tnikkel)

References

Details

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

Attachments

(3 files)

Make test wait for paint
1.91 KB, patch
kats
: review+
lizzard
: approval-mozilla-aurora+
Details | Diff | Splinter Review
Schedule paint on apz aware listener added
3.29 KB, patch
smaug
: review+
lizzard
: approval-mozilla-aurora+
Details | Diff | Splinter Review
"Schedule paint on apz aware listener added" for aurora
3.81 KB, patch
Details | Diff | Splinter Review
This is probably a regression from turning on APZ on Linux. Most likely a similar issue to bug 1209942.
Blocks: apz-linux
Component: DOM: Events → Panning and Zooming
Blanket gfx-noted on APZ intermittent failures so they can be found more easily on https://brasstacks.mozilla.com/orangefactor/
Whiteboard: [gfx-noted]
This appears to be caused by the mozmousepizelscroll handling getting installed after the page is fully painted, so the event regions on the layer tree do not contain the information that a wheel event shouldn't scroll the page. Here is a try run where I force the page to paint after the mozmousepizelscroll handler is added to verify my theory as correct https://treeherder.mozilla.org/#/jobs?repo=try&revision=e1b7303a1c01 So to fix this we just need to make sure that adding a mozmousepizelscroll triggers a paint.
Are you still working on this? The bug I started to look at seems to have the same root cause (not very surprising, it's another failure mode of the same test). I can work on the patch if you are busy with image lib stuff.
Depends on: 1210312
Flags: needinfo?(tnikkel)
Yeah, I was hoping to get back to this soon. All the failures in this test are likely the same issue and if they aren't probably best to just wait until the known issue is fixed.
Flags: needinfo?(tnikkel)
Blocks: 1210312
No longer depends on: 1210312
My naiva patch to fix this didn't seem to fix it https://treeherder.mozilla.org/#/jobs?repo=try&revision=d5a60741590d hmmmm
Couple of things that are likely problems: -mTarget isn't a nINode, so that chunk does nothing for the event handler added in this test -SchedulePaint will schedule a paint, but the paint will likely not have started by the time the test starts sending events that need to be handled correctly (and so need the updated layer tree)
(In reply to Timothy Nikkel (:tnikkel) from comment #28) > Couple of things that are likely problems: > -mTarget isn't a nINode, so that chunk does nothing for the event handler > added in this test That's not really a problem, we can get the presshell and ask for a paint a different way for that case, but it's still not enough because of... > -SchedulePaint will schedule a paint, but the paint will likely not have > started by the time the test starts sending events that need to be handled > correctly (and so need the updated layer tree) Not really sure what we can do here. In order for this to work we'd need to do a sync paint here, and then wait until that layer tree is transferred over to the parent process to. But that is a no-go because we install event handlers from frame construction, and we cannot paint during frame construction. Options: 1) is it only tests (with special privileges that web content doesn't have) that can send events that are "apz aware"? I don't think so... 2) force a full paint and layer tree sync before processing an apz aware event.
Option 1 is good - web content generally cannot send events through APZ, so in the worst case they register a listener and then the *user* has to provide the input event before the next repaint in order to tickle this bug. I think triggering a repaint is good enough here, and we can modify the test to wait for the pending paint before proceeding.
Attachment #8717362 - Flags: review?(bugmail.mozilla)
Assignee: nobody → tnikkel
Attachment #8717363 - Flags: review?(bugs)
Comment on attachment 8717362 [details] [diff] [review] Make test wait for paint Review of attachment 8717362 [details] [diff] [review]: ----------------------------------------------------------------- Nice, thanks! The other patch more properly belongs on bug 1131188, but I guess you can just dupe it over.
Attachment #8717362 - Flags: review?(bugmail.mozilla) → review+
Comment on attachment 8717363 [details] [diff] [review] Schedule paint on apz aware listener added > if (IsApzAwareEvent(aTypeAtom)) { > nsCOMPtr<nsINode> node = do_QueryInterface(mTarget); > if (node) { > node->SetMayHaveApzAwareListeners(); > } >+ SchedulePaintForTarget(); So we QI to nsINode in this 'if'. >+EventListenerManager::SchedulePaintForTarget() >+{ >+ nsIDocument* doc = nullptr; >+ if (nsCOMPtr<nsINode> node = do_QueryInterface(mTarget)) { >+ doc = node->OwnerDoc(); >+ } And then again here. In general would be good to avoid extra QI calls, since those are slow virtual calls. But this shouldn't be too hot code path. So we don't need to reschedule paint when a node, which has apz related listener, from another document is adopted to our document?
Attachment #8717363 - Flags: review?(bugs) → review+
(In reply to Olli Pettay [:smaug] (high review load) from comment #35) > So we QI to nsINode in this 'if'. > > And then again here. In general would be good to avoid extra QI calls, > since those are slow virtual calls. > But this shouldn't be too hot code path. I moved the SetMayHaveApzAwareListeners call into schedule paint function (and renamed that function ProcessApzAwareEventListenerAdd) so we could just do one QI. > So we don't need to reschedule paint when a node, which has apz related > listener, from another document is adopted to our document? The insertion notifications into the new document will trigger frame construction and that will trigger painting if necessary. display: none can't receive events, and nodes that aren't in the dom tree can't receive events, so we should be fine I think. ni to make sure you agree.
Flags: needinfo?(bugs)
Ok, right, and we don't care to optimize here to check for only nodes which do have or will have a layout object. We just re-paint always. Ok, fine.
Flags: needinfo?(bugs)
https://hg.mozilla.org/mozilla-central/rev/cccdf7779f2c https://hg.mozilla.org/mozilla-central/rev/b1a7043faedd
Status: NEW → RESOLVED
Closed: 10 years ago
status-firefox47: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla47
Can you please nominate this for Aurora approval?
status-firefox45: --- → unaffected
status-firefox46: --- → affected
status-firefox-esr45: --- → unaffected
Flags: needinfo?(tnikkel)
Depends on: 1253052
I realized we should probably do this a little better, so I wrote bug 1253052. I'll ask for uplift on that too when it lands.
Flags: needinfo?(tnikkel)
Comment on attachment 8717362 [details] [diff] [review] Make test wait for paint Approval Request Comment [Feature/regressing bug #]: none really? [User impact if declined]: sheriffs have to star this failure a lot [Describe test coverage new/current, TreeHerder]: it's a test [Risks and why]: safe, it's a test [String/UUID change made/needed]: none
Attachment #8717362 - Flags: approval-mozilla-aurora?
Comment on attachment 8717363 [details] [diff] [review] Schedule paint on apz aware listener added Approval Request Comment [Feature/regressing bug #]: turning on apz [User impact if declined]: if a webpage registers event listeners for some events that usually cause scrolling the page won't act right [Describe test coverage new/current, TreeHerder]: this big is about fixing a test failure [Risks and why]: pretty safe, just asks for an extra paint sometimes [String/UUID change made/needed]: none
Attachment #8717363 - Flags: approval-mozilla-aurora?
Comment on attachment 8717363 [details] [diff] [review] Schedule paint on apz aware listener added Fixing a test failure from APZ, may help scrolling. Please uplift to aurora.
Attachment #8717363 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Flags: needinfo?(tnikkel)
Comment on attachment 8717362 [details] [diff] [review] Make test wait for paint test fix, ok to land on aurora
Attachment #8717362 - 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: