783405 - Selecting year of birth in https://esta.cbp.dhs.gov using mouse is very slow

Status: VERIFIED FIXED

(Core :: Layout, defect)

Description

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

Happening on trunk. Had to use keyboard

Comment 1

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

I don't want to debug this before I've done my trip to US, but using FF to fill
the forms in Esta page was hard because of this.

Comment 2

[Lukas Blakk [:lsblakk] use ?needinfo]

It would be nice to get more info here to help inform the tracking request. Let's see if we can narrow this down to a particular regression and get some STR.

Comment 3

[(mostly gone) XtC4UaLL [:xtc4uall]]

FWIW, I see no Issues on Windows 7.
Tested http://hg.mozilla.org/mozilla-central/rev/a79132ac2f05 (2012-08-17) and http://hg.mozilla.org/mozilla-central/rev/360ab7771e27 (2012-08-21).

Comment 4

[Boris Zbarsky [:bzbarsky]]

smaug, can you still reproduce?

Comment 5

[u279076]

I'm able to reproduce this on Ubuntu 11.10 32-bit with Firefox 17.0a2 2012-08-30.

Steps to Reproduce:
1. Open https://esta.cbp.dhs.gov/esta/
2. Click "Apply"
3. Select "Yes" to accept the disclaimer and click "Next"
4. Select "Yes" to accept the Travel Promotion Act and click "Next"
5. Click the "Year" select box
6. Click the "Day" select box and try to scroll

Result:
Firefox because very sluggish.

I'll try to see if this has a regression range.

Comment 6

[u279076]

From mozregression:

Last good nightly: 2012-07-18
First bad nightly: 2012-07-19

Pushlog:
http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=ae22909cef5a&tochange=6d8456a77e57

Comment 7

[Boris Zbarsky [:bzbarsky]]

Could this be at least partly bug 539356?

I really wish we had hourlies from back then.  :(

Comment 8

[Matt Woodrow (:mattwoodrow)]

It's not obviously related to the 539356 patches that landed within that range, those patches were only minor refactorings.

Comment 9

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

Based on profile we end up doing reflow all the time.

Comment 10

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

(In reply to Anthony Hughes, Mozilla QA (:ashughes) from comment #6)
> From mozregression:
> 
> Last good nightly: 2012-07-18
> First bad nightly: 2012-07-19
> 
> Pushlog:
> http://hg.mozilla.org/mozilla-central/
> pushloghtml?fromchange=ae22909cef5a&tochange=6d8456a77e57

Not sure this is correct. http://hg.mozilla.org/mozilla-central/rev/c918ff2f0994 
is not good.

Comment 11

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

http://hg.mozilla.org/mozilla-central/rev/0602e44ac248 is ok.
http://hg.mozilla.org/mozilla-central/rev/57abb5f70e01 is bad.

http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=0602e44ac248&tochange=57abb5f70e01

And yes, beta is certainly broken.

Comment 12

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

Or maybe that regression range is bad too. I got the slowness once with http://hg.mozilla.org/mozilla-central/rev/0602e44ac248

Comment 13

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

http://hg.mozilla.org/mozilla-central/rev/46804c31366b has the problem...

Comment 14

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

Odd. After restarting X server I don't see the bug at all. Fun :/

Comment 15

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

Mats, is it possible that your changes to <select> handling could have caused
endless reflowing in some cases?

Comment 16

[Mats Palmgren (inactive)]

Yeah, bug 575294/bug 726264 would be my first suspect; there was also a
follow-up fix in bug 780661.

The first set landed on m-c Jun 23 05:36:09 2012 -0700:
https://hg.mozilla.org/mozilla-central/pushloghtml?changeset=e3676fde39a8

The follow-up landed on m-c Aug 15 08:23:05 2012 -0700:
https://hg.mozilla.org/mozilla-central/pushloghtml?changeset=f8d76d32e29b

and on aurora Aug 20 16:40:39 2012 -0700:
https://hg.mozilla.org/releases/mozilla-aurora/pushloghtml?changeset=82a02621fc9b

Comment 17

[Mats Palmgren (inactive)]

The first set landed on inbound Jun 22 18:13:52 2012 -0700:
https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?changeset=cc89df1b08ae

Comment 18

[u279076]

Can I please get a little more direction as to what is being requested of QA via qawanted?

Comment 19

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

We need regression range.

Comment 20

[u279076]

The regression range I gave in comment 6 is locally valid for me. Perhaps you can advise me further.

Comment 21

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

Did you re-try? The problem doesn't happen always.
After restarting X server I don't seem to get the problem on this machine, but I certainly did
see it even on the latest beta.
(I know, it is tricky to find the regression range.)

Comment 22

[u279076]

Comment 6 is still a valid regression range for me, even with restarting X server. And yes, I did retry.

Comment 23

[u279076]

smaug asked that I try this on the latest Beta. Firefox 16.0b1 on Ubuntu 11.10 32-bit does not reproduce this bug for me, while the regression range in comment 6 does.

Comment 24

[Mats Palmgren (inactive)]

That range contains a rather large merge from inbound - would it be possible
to narrow this by testing inbound builds?

Also, if it's a hang of some sort, try aborting the process (kill -6) 
and submit the crash data so we get a stack trace.

Comment 25

[u279076]

(In reply to Mats Palmgren [:mats] from comment #24)
> Also, if it's a hang of some sort, try aborting the process (kill -6) 
> and submit the crash data so we get a stack trace.

Crash report from 'kill -6' for Firefox 17.0a1 2012-07-19 which reliably reproduces the hang for me:
https://crash-stats.mozilla.com/report/index/bp-c1680021-7421-4013-b8ca-2a9012120904

Comment 26

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

I certainly saw the problem several times earlier today when testing beta.
Never when using release. But after X server restart I don't see the problem even in Nightly.

Comment 27

[u279076]

Firefox 17.0a1-inbound 2012-07-18 does not reproduce.
Firefox 17.0a1-inbound 2012-07-19 does not reproduce.

I'm not sure where to go from here...

Comment 28

[Mats Palmgren (inactive)]

I can reproduce it in the latest beta on Linux64.
The STR in comment 5 reproduce it if I make the window height ~500px
and place the window at the bottom of the screen.
I strongly suspect my combobox fixes...

Comment 29

[Mats Palmgren (inactive)]

I get the regression range:
2012-06-23-03-05-32-mozilla-central
2012-06-24-03-05-37-mozilla-central

I'll prepare a backout of the bugs in comment 16 for mozilla-beta.

Comment 30

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

I was hoping that we could have some very simple fix. Do we end up some rounding problems or what?

Comment 31

[Mats Palmgren (inactive)]

Attachment: fix

The problem appears to be that the nsAsyncResize event:
http://mxr.mozilla.org/mozilla-central/source/layout/forms/nsComboboxControlFrame.cpp#598
that AbsolutelyPositionDropDown dispatches to adjust the size based on
the current combobox position:
http://mxr.mozilla.org/mozilla-central/source/layout/forms/nsComboboxControlFrame.cpp#682
somehow leads up to a reflow of the *combobox* frame, although the reflow
request is for the list frame (dropdown).

There's two calls to GetAvailableDropdownSpace(), one in 
AbsolutelyPositionDropDown and one in nsListControlFrame::ReflowAsDropdown,
reflowing the combobox made them use different y-positions and thus
different decisions were made on how many items could fit at the current
position leading to self-feeding loop of reflows.

I don't see a simple solution to this.  This patch stores the
y-positions above/below at the time of the AbsolutelyPositionDropDown
call, where we have the correct combobox position.  Those values are
then re-used for the GetAvailableDropdownSpace from ReflowAsDropdown
so that the two call sites are in sync on what position the space
calculation is based on.

I still think backing out is the less risky option from a regression
point of view.  It means we have to live with the original bugs another
cycle though.  What do you think?


https://tbpl.mozilla.org/?tree=Try&rev=5d537c9263bc

Comment 32

[Mats Palmgren (inactive)]

Backing out from beta means reverting bug 780661, bug 575294 (5 parts), bug 726264.
It's a simple backout, reverse applying the patches had no conflicts.

Comment 33

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

Comment on attachment 658330 [details] [diff] [review]
fix

>+++ b/layout/forms/nsComboboxControlFrame.h
>@@ -267,16 +267,23 @@ protected:
>   int32_t               mRecentSelectedIndex;
>   int32_t               mDisplayedIndex;
>   nsString              mDisplayedOptionText;
> 
>   // make someone to listen to the button. If its programmatically pressed by someone like Accessibility
>   // then open or close the combo box.
>   nsCOMPtr<nsIDOMEventListener> mButtonListener;
> 
>+  // The last y-positions used for estimating available space above and
>+  // below for the dropdown list in GetAvailableDropdownSpace.  These are
>+  // reset to nscoord_MIN in AbsolutelyPositionDropDown when placing the
>+  // dropdown at its actual position.  The GetAvailableDropdownSpace call
>+  // from nsListControlFrame::ReflowAsDropdown use the last position.
>+  nscoord               mLastDropDownAboveScreenY;
>+  nscoord               mLastDropDownBelowScreenY;
Could you initialize these in ctor?
Then the change below GetStateBits() & NS_FRAME_FIRST_REFLOW shouldn't be needed, right?

Roc should look at this too (I'm not a layout peer ;) ).

This looks reasonable safe to me, and we have some time before next release.
Could we take this to nightly/aurora/beta asap, and if this causes *any* problems, 
back out immediately as well as the bug which caused this bug.

Comment 34

[Mats Palmgren (inactive)]

Attachment: fix, v2

Moved the initialization to the ctor as suggested.

Comment 35

[Mats Palmgren (inactive)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/8493fd90d1d2

Comment 36

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/8493fd90d1d2

Comment 38

[Tony Mechelynck [:tonymec]]

My contribution to VERIFYing this: bug 788024 (on SeaMonkey for linux-x86_64), which was a suspected duplicate, has disappeared thanks to this fix. :-)

Comment 39

[Mats Palmgren (inactive)]

Comment on attachment 658544 [details] [diff] [review]
fix, v2

[Approval Request Comment]
Bug caused by (feature/regressing bug #): bug 575294
User impact if declined: hard to interact with comboboxes and poor performance on some pages
Testing completed (on m-c, etc.): on m-c since 2012-09-06.  Ted says it fixed the perf problem he saw in SeaMonkey.
Risk to taking this patch (and alternatives if risky): This code is rather error prone so any change carries some risk.  The alternative is to backout bug 780661, bug 575294, bug 726264 - which should be a relatively low risk thing to do since the affected code is isolated and does not affect much else.  Applying the patches in reverse order doesn't produce any conflicts.
String or UUID changes made by this patch: none

Comment 40

[Mats Palmgren (inactive)]

Comment on attachment 658544 [details] [diff] [review]
fix, v2

[Approval Request Comment]
See comment 39, and comment 33.

In addition, I spoke with Jet about this bug and we agreed that taking
this for beta 3, and if any problem remains do a backout in beta 4,
is the best option (assuming there will also be a beta 4).

Comment 41

[Alex Keybl [:akeybl]]

Comment on attachment 658544 [details] [diff] [review]
fix, v2

[Triage Comment]
Approving for Aurora 17 given where we are in the cycle, and the amount of time we have to find regressions.

I'm concerned about finding regressions here prior to FF16 being released to the public. Approving for Beta 16 as well, but any help with what to look out for in terms of regressions would be helpful.

Comment 42

[Mats Palmgren (inactive)]

https://hg.mozilla.org/releases/mozilla-aurora/rev/4995e0cb5e30
https://hg.mozilla.org/releases/mozilla-beta/rev/e410223dd330

Comment 43

[Mats Palmgren (inactive)]

(In reply to Alex Keybl [:akeybl] from comment #41)
> I'm concerned about finding regressions here prior to FF16 being released to
> the public. Approving for Beta 16 as well, but any help with what to look
> out for in terms of regressions would be helpful.

Look for anything odd involving comboboxes, in particular the position and size
of its dropdown menu.  For targeted testing, try resizing the window height
and moving it to different locations on the screen and then interact with
the combobox.  Especially near the bottom of the screen where the dropdown
menu opens upwards if there's more room there.  Try also scrolling the page
so that the combobox moves to a new screen location and interact with it again.
Pay attention to any latency or CPU spikes.

Comment 44

[Mats Palmgren (inactive)]

> status-firefox18: --- → fixed

I thought we reserved status-firefox18:fixed for bugs that we fix
on a branch, and use Milestone:mozilla18 for bugs we fix on trunk?

Comment 45

[Alex Keybl [:akeybl]]

(In reply to Mats Palmgren [:mats] from comment #44)
> > status-firefox18: --- → fixed
> 
> I thought we reserved status-firefox18:fixed for bugs that we fix
> on a branch, and use Milestone:mozilla18 for bugs we fix on trunk?

It's much easier for everybody to query tracked bugs when the status flag is explicitly set.

Comment 46

[Mats Palmgren (inactive)]

(In reply to Alex Keybl [:akeybl] from comment #45)
> It's much easier for everybody to query tracked bugs when the status flag is
> explicitly set.

Fine with me, but it's a change in bug tracking procedures I wasn't aware of.
Was it discussed or explained somewhere? I guess Milestone is useless now then?

Comment 47

[Alex Keybl [:akeybl]]

(In reply to Mats Palmgren [:mats] from comment #46)
> (In reply to Alex Keybl [:akeybl] from comment #45)
> > It's much easier for everybody to query tracked bugs when the status flag is
> > explicitly set.
> 
> Fine with me, but it's a change in bug tracking procedures I wasn't aware of.
> Was it discussed or explained somewhere? I guess Milestone is useless now
> then?

I think it happened organically over time - I'll send a quick email to dev-planning. I don't think the status flag needs to be set for trunk unless the bug is tracking for that release.

Comment 48

[Mats Palmgren (inactive)]

> I think it happened organically over time

I disagree this has "happened".  Here's a few bugs that were resolved FIXED
in the last few days, with resp. owners.

bug 789691 	[e-mail] 	 
bug 789711 	[e-mail] 	 
bug 789741 	[e-mail] 	         
bug 789904 	[e-mail] 	 
bug 789960 	[e-mail] 	 
bug 790043 	[e-mail] 	 
bug 790093 	[e-mail] 
bug 790123 	[e-mail] 	 
bug 790164 	[e-mail]    
bug 790283 	[e-mail] 	 
bug 790285 	[e-mail] 

All of them used Milestone:mozilla18, none of them used status-firefox18:fixed

Comment 49

[Mats Palmgren (inactive)]

Oh, I saw your post in dev.planning now - so it's just for bugs with
tracking-* flag set.  OK, makes sense now.

Comment 50

[Tony Mechelynck [:tonymec]]

(In reply to Mats Palmgren [:mats] from comment #46)
[...]
> Fine with me, but it's a change in bug tracking procedures I wasn't aware of.
> Was it discussed or explained somewhere? I guess Milestone is useless now
> then?

Milestone is still useful when you're looking at a bug which was fixed in the past, and "Trunk" now may be different from what version "Trunk" was when the bug was fixed: it tells you that anything _later_ than the Milestone was fixed together with it (and if the fix comes unstuck in such a "later" version, it's a regression).

In olden times (when the star product of Mozilla was the Suite) the Milestone used to be set as a goal of "when this bug _will_ (hopefully) be fixed". But bugs were forgotten now and again, and when learning my QA job I regularly came across open bugs whose Milestone was ludicrously in the past. Nowadays the Milestone is a "history" setting, set together with RESOLVED FIXED to tell future passersby when this bug _was_ fixed. This is IMHO much saner, and useful even though we may also set "affected" "fixed" or "verified" for what is now Trunk.

Comment 51

[Mihaela Velimiroviciu (:mihaelav)]

Mozilla/5.0 (X11; Linux i686; rv:16.0) Gecko/20100101 Firefox/16.0 (beta 3)

Verified the fix on the above build: selecting items from drop downs seem to work fine now.

Comment 52

[u279076]

Thanks Mihaela. Since this seemed to be a bit elusive in finding the regression range would you mind re-verifying this on the next couple of days' builds?

Comment 53

[Mihaela Velimiroviciu (:mihaelav)]

Verified the fix on Mozilla/5.0 (X11; Linux i686; rv:17.0) Gecko/17.0 Firefox/17.0 beta 1

Comment 54

[Simona Badau, Desktop QA]

Mozilla/5.0 (X11; Linux i686; rv:18.0) Gecko/20100101 Firefox/18.0

Verified as fixed on Firefox 18 beta 2 - selecting items from drop downs works properly now.