1283329 - perceptible pause before navigating when pasting a link and quickly hitting "enter"

Status: VERIFIED FIXED

(Firefox :: Address Bar, defect, P1)

Description

[Dana Keeler (she/her) (use needinfo) [:keeler]]

STR: copy a link, paste it in the urlbar, and quickly hit enter
Expected results: Firefox jumps to it and starts navigating
Actual results: There's a perceptible pause where the browser appears to do nothing before then beginning navigation

Note that if you paste the link, wait a bit for the drop-down with "<url> - Visit", and then hit enter, the pause doesn't seem to occur.

Comment 1

[Cosmin Muntean [:cmuntean], Ecosystem QA]

Firefox: 50.0a1, Build ID  20160703030210
User Agent  Mozilla/5.0 (X11; Linux x86_64; rv:50.0) Gecko/20100101 Firefox/50.0

I have tested this issue on the latest Firefox (47.0.1) release and latest Nightly (50.0a1) build. I have copied a link, pasted in URL bar and quickly pressed "Enter" key after which the "New Tab" starts connecting to the pasted websites. The "pause" that you described appear before the tab title changes from "New Tab" to "Connecting" or after?

Comment 2

[Dana Keeler (she/her) (use needinfo) [:keeler]]

It was before the title changed. In any case, I'm having trouble reproducing this at the moment (for example, it doesn't seem to happen on a new profile, and even in the profile it was happening in, it seems to have gotten a bit better). I suspect without much evidence that this may have been due to either a slow DNS responder or some search suggestion server that wasn't as zippy as it should have been. I'll investigate further and reopen this if I can find something more concrete and repeatable.

Comment 3

[Simon Lindholm]

I'm seeing this as well, with search suggestions disabled. STR that reproduce for me:
- copy the string "http://example.com/?asdhjkwdfsk" (or anything else that's not a substring of >= 10 history entries)
- open a new tab, wait for a second
- quickly strike "Ctrl+V, Enter"

Happens in a new profile iff I transfer my old places.sqlite to it.

Comment 4

[Cosmin Muntean [:cmuntean], Ecosystem QA]

Firefox: 50.0a1, Build ID  20160724030208
User Agent  Mozilla/5.0 (X11; Linux x86_64; rv:50.0) Gecko/20100101 Firefox/50.0

Hi Simon,
I have retested this issue on Ubuntu 14.04 x64 and Windows 7 x64 with the latest Firefox (47.0.1) release and latest Nightly (50.0a1) build. I have disabled the search suggestion option and I have followed your provided steps, but I could not reproduce the issue. Firefox behaves the same like I described in comment 1.

The issue is still reproducible on the latest Nightly or latest Firefox release with a new clean profile without transferring your old "places.sqlite"?

Comment 5

[Simon Lindholm]

No, the pause is not perceptible in a new profile without my old places.sqlite -- the places database needs to be large enough for that to happen. My current places.sqlite is 61 MB and has a moz_places table with 102686 rows.

Comment 6

[Cosmin Muntean [:cmuntean], Ecosystem QA]

Hi Simon,

I did some investigation regarding "places.sqlite" and I found this forum post: https://support.mozilla.org/en-US/questions/969977?page=1. It seams that the places.sqlite file should be no larger than 10 MB.
I have verified a couple of profiles I have and a few of my team mates and each of them had places.sqlite files of 10 MB no matter how much history and bookmarks they had.

Considering this, your file may be corrupted since it has 61 MB. The user on the forum also mentioned about one file of 30 MB. 

As such I would suggest doing the following:
1. Create a new Firefox Sync account.
2. Synchronize your account with your default profile.
3. Disconnect your Firefox account from the default profile.
4. Create a new Firefox profile.
5. Synchronize your new Firefox profile with the Firefox account (which you created at Step 1).
6. Disconnect your Firefox account.

Can you please try these steps in order to see if the "places.sqlite" will return to a normal value and if the issue is no longer reproducible?

Comment 7

[Simon Lindholm]

Sure, I can experiment with recreating my places.sqlite and see how much it helps. (Does Firefox Sync preserve history?)

However, 61 MB is not an abnormal value -- it's exactly what nsPlacesExpiration.js specifies as DATABASE_MAX_SIZE (it gets dynamically lowered to less than that when disk space or memory is low). And the right fix for this surely isn't to speed up database queries to reduce the pause, it's to allow navigation even before all history-search SQL queries have completed, since "enter" behavior is not dependent on those anyway.

Comment 8

[Simon Lindholm]

A new profile with my existing Firefox Sync account connected to it gets a places.sqlite of size 5.5 MB and 9359 rows in the moz_places table (meaning, Sync does transfer history, but not all of it). The delay there is perceivable but not really annoying.

Comment 9

[Simon Lindholm]

I'm also seeing this behavior if I raise browser.urlbar.delay to something larger, say 10000. _matchFirstHeuristicResult runs quickly (at most ~50ms), but somehow it doesn't cause any heuristic result to be shown until the search has fully finished.

Comment 10

[Simon Lindholm]

It's not always slow; in fact, it seems to be slow exactly when the popup was previously closed. So, STR:

1. Set browser.urlbar.delay to 5000 in about:config.
2. Open a new tab; make sure the url bar is blank and no popup is open.
3. Press "a".
4. Either:
 a) Press Enter. Firefox will take ~5 seconds to respond.
 b) Wait 5 seconds until it responds, then press Ctrl+A, "b", Enter. The navigation to some URL starting with "b" will be immediate.
or
 c) Wait 5 seconds until it responds, then click outside so that the popup closes. Select the whole URL from before and press "b", Enter. Navigation will be slow again.

Comment 11

[Simon Lindholm]

In case it matters (because, well, popups, you never know...), this is on Linux (Ubuntu 16.04).

Comment 12

[Marco Bonardo [:mak]]

(In reply to Simon Lindholm from comment #7)
> However, 61 MB is not an abnormal value -- it's exactly what
> nsPlacesExpiration.js specifies as DATABASE_MAX_SIZE

right, my DB is 70MB, that's normal. We are doing work to reduce that a little bit but it's not the issue.

(In reply to Simon Lindholm from comment #10)
> It's not always slow; in fact, it seems to be slow exactly when the popup
> was previously closed. So, STR:
> 
> 1. Set browser.urlbar.delay to 5000 in about:config.

does this really have an effect? like if you set it to 10, it will wait 10 seconds?
It should not have an effect on the time it takes to handle Enter, that's only used to start the SECOND query, after the first one is done.
But it may be interesting to investigate if something is broken regarding that, if it really can cause a predictable delay and it's not a case.

that said, I'm working on some fixes to the delayed Enter behavior in bug 1301093 and we should look at this once we properly handle it.
The reason to delay Enter is predictability, every time you type something and press enter, we must ensure you always get the same result. If we'd not wait sometimes you could end up on a google search, sometimes on a totally different page.
Sometimes though we take too much time to return the first result and that's what we should investigate (bug 1301560 is similar, as well as bug 1279864).

Comment 13

[Simon Lindholm]

> does this really have an effect? like if you set it to 10, it will wait 10 seconds?

Yes, if I set it to 10000 (it's in milliseconds) I see no results until after 10 seconds, not even the first result*, and Enter does nothing. So something is broken there. Hopefully it might be the same bug as the others. Did you try the STR?

(* well, if the first result is an inline completion, that does get shown in the location bar, but the popup is still empty, and Enter still does nothing until 10 seconds have passed and the popup has become non-empty.)

I tried a try build from bug 1301093, and it didn't seem help with this case (not sure I should have expected it to).

Comment 14

[Marco Bonardo [:mak]]

that's useful, I'm going to investigate why the delay prefs affects us, since it shouldn't, that may cause interesting findings.

Comment 15

[Marco Bonardo [:mak]]

The problem looks related to the fact we call .invalidate instead of ._invalidate. Subtle difference, the former ignores the call if the popup is closed (ooops!).
So in some cases we end up populating the popup only at the second match, instead of the first one, if the first one comes when the popup is still closed (that's why sometimes it is fast, the popup is already open). the second match is delayed by browser.urlbar.delay, but even more interesting it will be slower, maybe even much slower.

Now, the world would be a nice place if it was enough to change 1 char, unfortunately that causes us to miscalculate the popup size, so I need to figure out something more...
Taking for now, if we can fix this it may remove a lot of the delay users are seeing.

Comment 16

[Marco Bonardo [:mak]]

In a few hours from now, this should make some test builds with a possible fix for this issue. I'll ask you to test them once they are ready, and let me know. I think this fix could make a big difference.
Though I still have to try to write a test.
https://treeherder.mozilla.org/#/jobs?repo=try&revision=f4a5ad22db07
I think builds donwload address will be:
http://ftp.mozilla.org/pub/firefox/try-builds/[e-mail]-f4a5ad22db074fd2cb2dda8e214c752438931f45/

Comment 17

[Marco Bonardo [:mak]]

ehr, looks like there's a loop now, so after some searches the build may hang, I'm still looking into that, but still the build should be good to check the delay (I will make new ones once I have a test)

Comment 18

[Simon Lindholm]

Wonderful! I applied the patches locally, and they do seem to help. Monkey-testing the awesomebar a bit, I do see the following oddity happening:

1. Restart the browser, it helps with timing I think
2. Type some letter and then enter *very* quickly, preferably with some heavy page currently loading, so that inline completion and navigation happens before the awesomebar has a chance to open (which now happens more asynchronously, it looked like?)

As a consequence of this:
* the awesomebar opened with 0 entries and stayed open even after navigation. It closed when trying to take a screenshot, though.
* I got some "too much recursion" errors spewed to my terminal: https://gist.github.com/simonlindholm/4e794a285a59a553a1618b71ef2b625d
Would be nice to have a clearer stack trace here, but I don't have one...
* As a result of the recursion errors, invariants break all over the place, and the awesomebar stops working. At one time it gave no errors, another it repeatedly claimed "TypeError: rows[(numRows - 1)] is undefined" at autocomplete.xml:1199:17, i.e. this line:
> let lastRowRect = rows[numRows - 1].getBoundingClientRect();

(Hm, maybe test failures will show some clearer version of this...)

As an aside, it sometimes happened when I typed "a" "enter" really quickly at the beginning of the browser session that I got sent to a Google search for "a" rather than the expected inline auto-completion.

(And more of a pre-existing feature request, but: tab (or down-arrow) does not get the same delayed effect as enter, so "somestring<tab><enter>" is unpredictable. Noticable when fuzz-testing, at least.)

Comment 19

[Marco Bonardo [:mak]]

yes, tab is not delayed, probably it should be, that adds another layer of complexity, so not going to look into that now. I have a new patch that solves the recursion and has a test.
https://treeherder.mozilla.org/#/jobs?repo=try&revision=df7c2e4ca6ad

Comment 20

[Marco Bonardo [:mak]]

btw, if you only apply this patch, you lack a bunch of changes from bug 1301093 so the delay doesn't work. Would be better to test the whole set of patches (thus wait for the try builds to be on the ftp).

Comment 21

[Marco Bonardo [:mak]]

builds:
https://archive.mozilla.org/pub/firefox/try-builds/[e-mail]-df7c2e4ca6ad273bc57520fc82dd15ad944bd40b/

Comment 22

[Simon Lindholm]

Those patches were in the try build, so I did apply them, but I may have forgotten to rebuild the C++ files... Anyway, the last try build seems to work great, and fixes all the problem I saw.

Comment 24

[Marco Bonardo [:mak]]

Attachment: Bug 1283329 - Perceptible pause when quickly typing and hitting enter in the urlbar.

Review commit: https://reviewboard.mozilla.org/r/79248/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/79248/

Comment 25

[Marco Bonardo [:mak]]

note: I'm not a 100% sure about what's up in adjustSiteIconStart, the fact is that the item seems to be missing the binding just after we append it, the setTimeout shouldn't hurt based on my testing, but we should keep our eyes open just in case.

Comment 26

[Drew Willcoxon :adw]

(In reply to Marco Bonardo [::mak] from comment #15)
> The problem looks related to the fact we call .invalidate instead of
> ._invalidate. Subtle difference, the former ignores the call if the popup is
> closed (ooops!).

I think that's my fault... sorry everyone...

> So in some cases we end up populating the popup only at the second match,
> instead of the first one, if the first one comes when the popup is still
> closed (that's why sometimes it is fast, the popup is already open). the
> second match is delayed by browser.urlbar.delay

Oh wow, nice catch.

Comment 27

[Drew Willcoxon :adw]

Comment on attachment 8791968 [details]
Bug 1283329 - Perceptible pause when quickly typing and hitting enter in the urlbar.

https://reviewboard.mozilla.org/r/79248/#review77894

Comment 28

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/89d35548d4fc
Perceptible pause when quickly typing and hitting enter in the urlbar. r=adw

Comment 29

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/89d35548d4fc

Comment 30

[Marco Bonardo [:mak]]

[Tracking Requested - why for this release]: The work in this bug and bug 1301093 largely improves the user interaction with the awesomebar, especially the responsiveness, for which we got various bug reports from 48 on. Since the awesomebar is the primary go-to target to browser, improving the interaction sounds important enough.

I will try to make a merged patch for uplift in the enxt 24 hours so we can uplift early.

Comment 31

[Marco Bonardo [:mak]]

I'm moving the uplift to a separate bug, cause it requires multiple patches to be coalesced and is worth a separate discussion.

Comment 32

[Cristian Comorasu, QA [:ccomorasu], Releae Desktop QA - Please contact [e-mail]]

I reproduced this bug using Fx 50.0a1, build ID: 20160629030209, on Ubuntu 14.04 LTS x64.
I can confirm this issue is fixed, I verified using Fx 50.0b5, build ID: 20161006105459 and Fx 51.0a2, build ID: 20161007004004, on Windows 10 x64, mac OS X 10.10.5 and Ubuntu 14.04 LTS. There were no noticeable differences between the navigation delays.I also verified the issue by changing the  "browser.urlbar.delay" pref integer value. I will close this issue as verified.

Cheers!