469613 - Clicks on scrollbar's blank areas (above or below the thumb) don't scroll very long pages

Status: VERIFIED FIXED

(Core :: Layout, defect, P1)

Description

[YUKI "Piro" Hiroshi]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.1b3pre) Gecko/20081214 Shiretoko/3.1b3pre
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.1b3pre) Gecko/20081214 Shiretoko/3.1b3pre

A click on the blank area of the scrollbar should scroll the page by a height of the viewport. In short pages, like the search result of Google, the action works correctly. But, sometimes I saw that the action doesn't work in long pages.

The problem appears on my system with recent nightly builds, and I didn't see it in the builds in the last week. It is possibly a regression.


Reproducible: Sometimes

Steps to Reproduce:
1. Start Shiretoko 3.1b3pre with a clean profile.
2. Go to a long page, like http://urasoku.blog106.fc2.com/blog-entry-570.html
3. Wait for a while, until the page is completely loaded.
4. Click the blank area below the thumb of the vertical scrollbar multiple times.
Actual Results:  
Suddenly stops to scroll. After that, clicks on the scrollbar never work like it got jammed. But, after I've scrolled the page by the mouse wheel, clicks work correctly again.

Expected Results:  
Scrolls smoothly.

When the problem appears, I was building Firefox from the source. It is possibly caused when the CPU is too busy. Of course scrolling by the mouse wheel always works correctly even if the system is slow. The problem appears only with clickings on the scroll bar.

Comment 1

[YUKI "Piro" Hiroshi]

Another case:

1. Start Shiretoko 3.1b3pre with a clean profile.
2. Go to a long page, like http://urasoku.blog106.fc2.com/blog-entry-570.html
3. Scroll to middle of the page by scrolling of the mouse wheel, before the page is completely loaded.
4. Wait for a while, until the page is completely loaded.
5. Click the blank area below the thumb of the vertical scrollbar multiple
times.

Actual Results:  
Suddenly stops to scroll.After that, clicks on the area below the thumb never work. But, after I've clicked the area above the thumb, then, clickings on the area below the thumb work correctly again.

Expected Results:  
Scrolls smoothly.

Comment 2

[YUKI "Piro" Hiroshi]

Note:

+--+
|/\|
|  | <- the blank area above the thumb
|  |
|--|
|==| <- the thumb
|--|
|  |
|  | <- the blank area below the thumb
|\/|
+--+

Comment 3

[Ria Klaassen (not reading all bugmail)]

Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.2a1pre) Gecko/20081214 Minefield/3.2a1pre

I tested the case from comment 1 but was not able to reproduce the problem.

Comment 4

[Ria Klaassen (not reading all bugmail)]

Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.2a1pre) Gecko/20081214 Minefield/3.2a1pre

Both cases from comment 0 and comment 1 work fine for me. That proves nothing; maybe someone else can.
Can you also reproduce it with a new profile or with another mouse?
http://support.mozilla.com/en-US/kb/Basic+Troubleshooting#Make_a_new_profile

Comment 5

[Ria Klaassen (not reading all bugmail)]

(In reply to comment #4)

> Can you also reproduce it with a new profile?

Sorry, you already did.

Comment 6

[YUKI "Piro" Hiroshi]

Hmm... also on my system, sometimes works fine, sometimes doesn't.

Another case:

1. Start Shiretoko 3.1b3pre with a clean profile.
2. Tools => Error Console
3. Copy following code and paste it to "Code:" field:
-------------------------------------------------------------------------------
Components.classes["@mozilla.org/appshell/window-mediator;1"].getService(Components.interfaces.nsIWindowMediator).getMostRecentWindow("navigator:browser").gBrowser.addEventListener('DOMContentLoaded', function(aEvent) { for (var i = 0, maxi = 100; i < maxi; i++) { Application.console.log(aEvent.currentTarget.selectedTab.linkedBrowser.contentDocument.URL); } }, false);
-------------------------------------------------------------------------------
4. Push "Message" button on the Error Console.
5. Keeping the Error Console open, switch to the browser window and go to http://urasoku.blog106.fc2.com/blog-entry-570.html
6. Wait for a while, until the page is completely loaded.
7. Many messages appear in the Error Console.
8. Click the blank area below the thumb of the vertical scrollbar multiple
times (10 or 20 times).

The 3rd step makes CPU busy when the page is loaded on my system. With this condition, the problem often appears. My system is:

* Intel Celeron 2.4GHz
* 992MB RAM
* Windows XP

If your system has higher performance, I guess that changing "100" to a greater number possibly reproduces the problem.

Comment 7

[YUKI "Piro" Hiroshi]

> 7. Many messages appear in the Error Console.
> 8. Click the blank area below the thumb of the vertical scrollbar multiple
times (10 or 20 times).

Of course, you have to wait while the console is receiving messages. Go to 8th step after you get control back.

Comment 8

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

I reproduced only one time with 20081218035829-trunk/WinVista, we need to find the way to reproduce...

Comment 9

[Ria Klaassen (not reading all bugmail)]

Yes, it happened also to me once, at least something similar, on the mozilla central site, I clicked repeatedly on the arrow on top but it seemed stuck. I tried to reproduce it on a new profile but I couldn't.

Comment 10

[Ria Klaassen (not reading all bugmail)]

Markus, could Bug 463042 have caused this?
I was able to reproduce it on 5 Dec 2008, but not on 4 Dec 2008 so far, and your bug is in this range. Although I'm certain of 5 Dec, but not of 4 Dec, because it is hard for me to reproduce.

Comment 11

[Markus Stange [:mstange]]

Yes, it's very likely that bug 463042 has caused this.
I succeeded in reproducing this bug with the steps from comment 6.

Comment 12

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Markus, any progress on this? I can get someone else to take it if you don't have time to work on it.

Comment 13

[Markus Stange [:mstange]]

I think I've found the bug.

Comment 14

[Markus Stange [:mstange]]

Attachment: fix v1

This change in bug 457864 was wrong:

   // do nothing if the we aren't scrolling.
-  if (aDestinationX == mOffsetX && aDestinationY == mOffsetY) {
+  if (aDestinationX == mDestinationX && aDestinationY == mDestinationY) {
     // kill any in-progress smooth scroll

It means that two consecutive calls ScrollTo with the same position result in no scrolling at all because the async scroll is canceled.
My patch reverts that change and adds a test.

Comment 15

[Markus Stange [:mstange]]

pushed to mozilla-central:
http://hg.mozilla.org/mozilla-central/rev/fe759e3fd895

Comment 16

[Jim Jeffery not reading bug-mail 1/2/11]

this was backed out due to mochitest failures, not re-opened ?
changeset: http://hg.mozilla.org/mozilla-central/rev/0ff733711384

Comment 17

[Markus Stange [:mstange]]

I'm currently looking at the test that failed (test_datepicker.xul) to find out whether it was the patch or the test that caused the failure. I haven't got a clue yet.

Comment 18

[Markus Stange [:mstange]]

Another funny thing is that the test only failed on Linux.
http://tinderbox.mozilla.org/showlog.cgi?log=Firefox/1231500485.1231504660.29178.gz#err22

Comment 19

[Markus Stange [:mstange]]

I can't reproduce the test failure in my Ubuntu VM.

Comment 20

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Maybe we should back out the patch that caused the regression and then try to proceed from there?

Comment 21

[Markus Stange [:mstange]]

That's probably the right way to go forward.
Just to make sure that it's not the new test that causes the test failure I relanded the fix without the test today and backed out again; test_datepicker was still failing.

Comment 22

[Robert O'Callahan (:roc) (email my personal email if necessary)]

I'll try to reproduce this in my Linux VM and if that doesn't work I'll do the backout.

Comment 23

[Markus Stange [:mstange]]

Attachment: merged backout patch

This patch backs out bug 457864, bug 462793 and bug 463042.

Comment 24

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Can't reproduce it.

I won't be able to back it out straight away --- Markus, if you can back it out, or someone else can, that would be good.

Comment 25

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

can't reproduce the problem on Linux.

Comment 26

[Markus Stange [:mstange]]

Attachment: fix v2

Smaug discovered a problem with the original patch: Due to the early return when mDestination == aDestination, a synchronous scroll might not be synchronous when there's a pending async scroll to the same position. I've removed the early return and added a test for that case.

Let's hope that this is what caused the test_datepicker failure.

Comment 27

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 358212 [details] [diff] [review]
fix v2

It's worse than that, right? We're actually cancelling the async scroll when we might not have reached the destination, so we'll *never* reach the destination, sync or async.

But shouldn't we do ClampScrollValues before we do this check?

Comment 28

[Markus Stange [:mstange]]

Attachment: fix v3

(In reply to comment #27)
> (From update of attachment 358212 [details] [diff] [review])
> It's worse than that, right? We're actually cancelling the async scroll when we
> might not have reached the destination, so we'll *never* reach the destination,
> sync or async.

Yeah, but that was already fixed in the first patch (attachment 355818 [details] [diff] [review]), wasn't it?

But even the second patch (attachment 358212 [details] [diff] [review]) had a bug - we fail to reset mDestinationX/Y after we cancel the async scroll.

> But shouldn't we do ClampScrollValues before we do this check?

That too. First I considered moving the check down to after ClampScrollValues, but then I realized that at that point there's no real gain in doing an early return. So I just removed the early return completely.
There are other bail-out-when-not-scrolling checks in ScrollToImpl and Scroll, they should be enough.

I also added a third test that ensures that we're resetting mDestinationX/Y properly.

Comment 29

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Markus, can you check this into trunk in the next 24 hours? If I don't hear from you by then I'll check it in myself to make the beta3 freeze.

Comment 30

[Markus Stange [:mstange]]

Landed on mozilla-central:
http://hg.mozilla.org/mozilla-central/rev/a0cf750c586a

I'll resolve this bug if the tests pass.

Comment 31

[Markus Stange [:mstange]]

The linux testrun timed out before test_datepicker could run. Unfortunately I don't have time to wait for another cycle, but peterv will monitor the tree for me.

Comment 32

[Markus Stange [:mstange]]

Tests passed, yay!

Roc, if you have time to land it on 1.9.1 before I can, feel free to do so.

Comment 33

[Markus Stange [:mstange]]

Pushed to 1.9.1: http://hg.mozilla.org/releases/mozilla-1.9.1/rev/924078312e92

Shimoda, the new nightly is out, can you still reproduce the problem?

Comment 34

[YUKI "Piro" Hiroshi]

Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.9.1b3pre) Gecko/20090126 Shiretoko/3.1b3pre
Clicks on scrollbar work correctly on my system too.