Closed Bug 1093571 Opened 5 years ago Closed 4 years ago

Delay when using search bar on New Tab screen

Categories

(Firefox :: Search, defect, P3)

33 Branch
defect
Points:
5

Tracking

()

VERIFIED FIXED
Firefox 44
Tracking Status
firefox44 --- verified

People

(Reporter: swaldman, Assigned: glob)

References

Details

(Whiteboard: [bug][fxsearch])

Attachments

(1 file, 1 obsolete file)

User Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:33.0) Gecko/20100101 Firefox/33.0
Build ID: 20141011015303

Steps to reproduce:

1. Open a new tab
2. Type a search term into the search box on the page
3. Immediately press enter or click "search", before search suggestions have appeared


Actual results:

Nothing happens - no search, and no visual feedback - for a period of up to ~5 seconds. After this delay, the search is performed as normal. Speculating, it seems likely that nothing is done until the search suggestions have appeared - hence the time delay may depend on network conditions.

If the user switches to another open tab during this delay, then when the search results eventually appear they are displayed not in the new tab where the search was done, but in the active tab that the user subsequently switched to. This can result in a page that the user is working in being unexpectedly lost to become the results page of a search engine.


Expected results:

The search should be performed immediately, even if suggestions have not yet been displayed. If the user switches to another tab while a search is happening, the results should be displayed in the tab that the search was started from, not the one currently active.
Did you make a test in safe mode (https://support.mozilla.org/en-US/kb/troubleshoot-firefox-issues-using-safe-mode) and with a fresh profile (https://support.mozilla.org/en-US/kb/troubleshoot-firefox-issues-using-safe-mode)?
Component: Untriaged → Search
Flags: needinfo?(swaldman)
I have now! Problem persists both in Safe Mode and with a fresh profile.
Flags: needinfo?(swaldman)
Do you see the delay for every search engine selected? 

In addition, do you see some error messages in the console for your testcases? (Ctrl+Shit+K)
No error messages in the console.

I've tried with three search engines now (DuckDuckGo, Google and Wikipedia). All exhibit this delay and the consequent results-in-the-wrong-tab bug. However, it may be unrelated to waiting for search suggestions. I found that Wikipedia gave me back suggestions quicker than the others, before I could press enter, but I still got the delay.
Thanks for the report, Simon, and for following up with additional testing.

To initialize the search box, Firefox sends messages back and forth between itself and the new tab page. When you search, the new tab page sends a message to Firefox, and the search is only triggered once Firefox processes the message.

If the search does eventually occur without further action, that might suggest that for some reason Firefox is taking a really long time to process the message from the new tab page. This typically should occur in milliseconds, and not take 5 seconds, but maybe something very strange is occurring in your case. Have you noticed other similar responsiveness issues with Firefox?
Flags: needinfo?(swaldman)
Hmm, curious. No, I don't have other responsiveness issues. 5 seconds is my guess at the longest it's taken - it's probably more usually <2 seconds. But still orders of magnitude above what you expect.

I've just tried on another computer, and not seen it there - so it seems to be specific to the one machine. That is.... awkward, as I have no idea what's different and hence how to replicate elsewhere! Sorry... unless you can suggest specific things about the computer or the Windows and/or Firefox configuration that might cause it, I fear I can't help much more.
Flags: needinfo?(swaldman)
I can reproduce both parts of this bug by quickly typing gibberish and hitting enter.

The delay part: ContentSearch.jsm processes incoming messages and their outgoing responses in FIFO order.  When you start typing, ContentSearch first receives a message requesting suggestions, and then when you hit enter, it receives a message to search.  ContentSearch doesn't actually start the search -- the search message isn't processed -- until it receives suggestions over the network and it sends the suggestions message response.

The wrong tab part: ContentSearch._onMessageSearch has access to the xul:browser that requested the search (via msg.target), but instead it gets the chrome window and uses it to load the search URI (browserWin.loadURI).  That's dumb.  It should call loadURI on msg.target or something equivalent instead.  The fix for this should be much simpler than the fix for the delay part, so I'll file a new bug for it.  http://mxr.mozilla.org/mozilla-central/source/browser/modules/ContentSearch.jsm?rev=6818a0733662#208
See Also: → 1096534
I filed bug 1096534 for the wrong tab part.

Let's focus this bug on the delay part.  Clearly ContentSearch should not wait for suggestions before responding to search messages.  The reason for ContentSearch's message queue, if I recall correctly, is to ensure that client pages see responses to their messages in the same order that the messages were sent.  I don't think we want to get rid of that property.

So I guess whenever ContentSearch receives a search message from a browser, it should cancel suggestions that it's waiting on for that browser, cancel queued up suggestions messages for that browser (so that those suggestions aren't actually fetched), and then queue up the search message like usual.  (The design assumption is that there will be at most one search box per client page.)  The reason for canceling queued up suggestions messages instead of removing them is that the client page should still probably receive responses because it may be expecting them.
Status: UNCONFIRMED → NEW
Points: --- → 5
Ever confirmed: true
Flags: firefox-backlog+
OS: Windows 7 → All
Hardware: x86_64 → All
I'm able to reproduce it too by typing a long chain of random characters in the search field and pressing enter quickly.
Duplicate of this bug: 1097667
Blocks: 1028985
Flags: qe-verify+
Rank: 37
Priority: -- → P3
Whiteboard: [bug][fxsearch]
This might be fixed with bug 1171344.
Duplicate of this bug: 1187606
Attached patch 1093571.patch (obsolete) — Splinter Review
Assignee: nobody → glob
Status: NEW → ASSIGNED
Attachment #8674061 - Flags: review?(adw)
Comment on attachment 8674061 [details] [diff] [review]
1093571.patch

Review of attachment 8674061 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks!  This looks good, but a couple of things.  It could be the case that ContentSearch is currently waiting on suggestions when it gets a Search message.  In that case, it should cancel the suggestions fetch.  In other words, you need to cancel not only queued up GetSuggestions but also the GetSuggestions currently being serviced.

You can stop a suggestions fetch by calling controller.stop(): http://mxr.mozilla.org/mozilla-central/source/toolkit/components/search/SearchSuggestionController.jsm#170

I guess you could set this._currentSuggestionController = controller in _onMessageGetSuggestions, and that way you'd have a handle to the controller in _cancelSuggestions.

Second, we should probably only send SuggestionsCancelled if suggestions are actually canceled.
Attachment #8674061 - Flags: review?(adw) → feedback+
Attached patch 1093571.patchSplinter Review
Attachment #8674061 - Attachment is obsolete: true
Attachment #8678253 - Flags: review?(adw)
Comment on attachment 8678253 [details] [diff] [review]
1093571.patch

Review of attachment 8678253 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks! :-)
Attachment #8678253 - Flags: review?(adw) → review+
awesome!

i'm not a firefox dev, so i'm not sure if this requires a try run.  if it does, can you please kick one off for me please?

is this fix one that needs to ride the trains, or is it one that could be considered for inclusion in the next aurora/beta release?
Here's a try run:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=0ee248ec3270

Hmm, personally I'd be more comfortable with this riding the trains.  The patch is small and well understood, and this code has existing tests, but it does touch a couple of visible search points, so there's some potential for accidentally introducing other fairly visible problems, especially since async messaging is involved.
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/0ea84b4619dc
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 44
I have reproduced this bug on Nightly 36.0a1 (2014-11-04) on ubuntu 14.04 LTS, 32 bit!

The bug's fix is now verified on Latest Nightly 45.0a1!

Build ID: 20151117030242
User Agent: Mozilla/5.0 (X11; Linux i686; rv:45.0) Gecko/20100101 Firefox/45.0

[bugday-20151118]
Reproduced on Firefox 43.0b4, build ID: 20151116155110 by entering a very long term in the search bar located in the newtab page and pressing enter.

Confirming this fix on latest DevEdition, build ID: 20151117004023.
Testing was performed on Mac OS X 10.8.5, Ubuntu 14.04 x64 and Windows 10 x64.

Thanks Khalid for verifying this on Nighty 45 also!
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.