493037 - "aNumLines must be non-zero" assertion hit on mouse wheel input

Status: RESOLVED FIXED

(Core :: Widget: Win32, defect)

Description

[Jim Mathies [:jimm]]

The conditional assertion is in event state manager's CanScrollOn function -

http://mxr.mozilla.org/mozilla-central/source/content/events/src/nsEventStateManager.cpp#477

We either need to filter out zero delta mouse scroll events being sent from nsWindow, or remove this conditional assert. It's fairly common thing you get when debugging builds so it's kind of annoying.

Comment 1

[Jim Mathies [:jimm]]

Attachment: wheel scroll delta equals zero patch.v1

Comment 2

[Ere Maijala (slow)]

Comment on attachment 377889 [details] [diff] [review]
wheel scroll delta equals zero patch.v1

Are you sure ignoring it is the corect thing to do? When do you get the assertion? I mean, are we calculating the delta properly or could there be something wrong?

Comment 3

[Jim Mathies [:jimm]]

(In reply to comment #2)
> (From update of attachment 377889 [details] [diff] [review])
> Are you sure ignoring it is the corect thing to do? When do you get the
> assertion? I mean, are we calculating the delta properly or could there be
> something wrong?

Yes, because we cache the change in a static. This happens whenever our cached value doesn't measure up enough to warrent a scroll event.

http://mxr.mozilla.org/mozilla-central/source/widget/src/windows/nsWindow.cpp#5120

Comment 4

[Ere Maijala (slow)]

Comment on attachment 377889 [details] [diff] [review]
wheel scroll delta equals zero patch.v1

Got it, finally :) By the way, should we also reset the cached delta if the scroll direction changes? Currently we'd need to collect scroll events to counter any old surplus to the previous direction.

Comment 5

[Jim Mathies [:jimm]]

(In reply to comment #4)
> (From update of attachment 377889 [details] [diff] [review])
> Got it, finally :) By the way, should we also reset the cached delta if the
> scroll direction changes? Currently we'd need to collect scroll events to
> counter any old surplus to the previous direction.

Let me add that and test to see how it feels. I'll post a new patch if it works out.

Comment 6

[Jim Mathies [:jimm]]

(In reply to comment #5)
> (In reply to comment #4)
> > (From update of attachment 377889 [details] [diff] [review] [details])
> > Got it, finally :) By the way, should we also reset the cached delta if the
> > scroll direction changes? Currently we'd need to collect scroll events to
> > counter any old surplus to the previous direction.
> 
> Let me add that and test to see how it feels. I'll post a new patch if it works
> out.

That actually didn't have the effect I was expecting - it causes the center scroll point to wander when you repeatedly scroll around a central position. (Due to the loss of scroll data.) So I think we should stick with the original patch.

Comment 7

[Jim Mathies [:jimm]]

http://hg.mozilla.org/mozilla-central/rev/f4d72fac17b4

Comment 8

[Olli Pettay [:smaug][[e-mail]]]

Bustage fix was http://hg.mozilla.org/mozilla-central/rev/90cb997e9927

Comment 9

[Bryan]

Could this have caused wheel scrolling to scroll more page per scroll (not sure that is the best way to describe the issue)?  If so, it's certainly annoying.

~B

Comment 10

[Mark Perris]

I have the same problems, it's scrolling more than it used to, is this by design?
The page zooming with CTRL appears to be accentuated too, it zooms in or out by very large amounts.

Comment 11

[Jim Mathies [:jimm]]

It shouldn't as delta is zero.

This landed on top of bug 487245 though so there might be some bad interaction going on between the two, I'll take a look.

Comment 12

[Jim Mathies [:jimm]]

This patch doesn't appear to be the problem. Filed bug 501390.