[Affected versions]: - Nightly 55.0a1 [Affected platforms]: - Windows 10 x64, Ubuntu 16.04, Mac OS X 10.12 [Steps to reproduce]: 1. Launch Nightly with a new profile 2. Open a new tab and write "d" in the URL bar 3. Open a new tab and write "d" in the URL bar [Expected result]: - The URL drop-down should be displayed each time. [Actual result]: - Step 3 - the URL drop-down is not displayed anymore. For more details please see the attached screencast. [Regression range]: Last good revision: 6d4744a6c81c498cc81cad11b4ead64660a3d269 First bad revision: aa48bb3f494410de514041e8c06ca15c38680181 Pushlog: https://hg.mozilla.org/integration/autoland/pushloghtml?fromchange=6d4744a6c81c498cc81cad11b4ead64660a3d269&tochange=aa48bb3f494410de514041e8c06ca15c38680181 Looks like the following bug has the changes which introduced the regression: https://bugzilla.mozilla.org/show_bug.cgi?id=1362866

Huh. Interestingly, if you choose a different letter other than "d" in step 3, the AwesomeBar shows up again. Any idea what's happening here, mak? Is this more fallout from me setting _urlbarFocused to true on new tabs?

(In reply to Mike Conley (:mconley) from comment #1) > Huh. Interestingly, if you choose a different letter other than "d" in step > 3, the AwesomeBar shows up again. > > Any idea what's happening here, mak? Is this more fallout from me setting > _urlbarFocused to true on new tabs? I think it's more the fact you don't blur and focus the location bar anymore, so it may thing it's still the same search and doesn't search again. It sounds like a not handled case that before was handled through focus/blur events.

note, _urlbarFocused is no more more used by the location bar code, I removed all the calls since they became pointless for our needs.

(In reply to Marco Bonardo [::mak] from comment #2) > I think it's more the fact you don't blur and focus the location bar > anymore, so it may thing it's still the same search and doesn't search > again. It sounds like a not handled case that before was handled through > focus/blur events. That sounds likely. Do you know if there's another way I can clear that state without doing a blur / re-focus?

I can have a look on Monday, I don't recall all the code paths off-hand.

[Tracking Requested - why for this release]: Make sure this doesn't somehow slip to beta in 55.

Investigating...

tracking as new regression in 55.

This is actually due to an optimization in the autocompletecontroller where we don't repeat a just executed search (and we don't start a new search. Look for "repeatingPreviousSearch"). We should probably open the autcomplete popup though if we got results in the previous query and it's currently closed.

Comment on attachment 8872406 [details] Bug 1368074 - Autocomplete popup is not re-opened when repeating the same search in a new tab. https://reviewboard.mozilla.org/r/143914/#review147642 This looks great! I figure some of this (wrt the casting, the <= on an unsigned) is cleaning up some warnings? ::: toolkit/components/autocomplete/nsAutoCompleteController.cpp:295 (Diff revision 2) > newValue.Length() > 0 && > repeatingPreviousSearch) { > + // Don't search again for the same string, but, if the popup is closed and > + // we got results, we may want to open it. > + if (!mResults.IsEmpty() && mRowCount) { > + OpenPopup(); Should we check to see if the popup is already open before attempting to re-open it? Or do we _know_ it's closed if we've entered this branch in a way that I don't understand? ::: toolkit/components/autocomplete/nsAutoCompleteController.cpp:672 (Diff revision 2) > return NS_OK; > > nsCOMPtr<nsIAutoCompleteInput> input(mInput); > bool isOpen = false; > input->GetPopupOpen(&isOpen); > - if (!isOpen || mRowCount <= 0) { > + if (!isOpen || mRowCount == 0) { Heh, yep - turns out mRowCount is unsigned. ::: toolkit/components/autocomplete/tests/unit/test_repeated_search_opens_popup.js:35 (Diff revision 2) > + await promiseSearch; > + > + Assert.equal(controller.searchStatus, The way that Promises resolve means that there's _no_ opportunity for a search to have started and been completed by the time we assert here, right? ::: toolkit/components/autocomplete/tests/unit/test_repeated_search_opens_popup.js:52 (Diff revision 2) > + // Simulate closing the popup. > + input.popupOpen = false; > + > + // Re-run the same search, it should still open the popup, but the search > + // won't be executed again. > + await runSearchAndCheck("test", false); Forgive me for not knowing the tests under autocomplete very well, but is it necessary to close the popup after the test has completed?

(In reply to Mike Conley (:mconley) from comment #12) > This looks great! I figure some of this (wrt the casting, the <= on an > unsigned) is cleaning up some warnings? not warnings, just unclear or unsafe code. Casting a signed integer to unsigned when we know it's >= 0 is safer than casting an unsigned to signed without checking its value. > ::: toolkit/components/autocomplete/nsAutoCompleteController.cpp:295 > Should we check to see if the popup is already open before attempting to > re-open it? Or do we _know_ it's closed if we've entered this branch in a > way that I don't understand? Actually, this code never did that check, and indeed all the consumers do that check on their own. We could certainly centralize the check but not here (since the controller is shared between any autocomplete field in Firefox it's risky to change the behavior). For example: http://searchfox.org/mozilla-central/rev/66d9eb3103e429127c85a7921e16c5a02458a127/browser/components/search/content/search.xml#713 http://searchfox.org/mozilla-central/rev/66d9eb3103e429127c85a7921e16c5a02458a127/toolkit/content/widgets/autocomplete.xml#838 http://searchfox.org/mozilla-central/rev/66d9eb3103e429127c85a7921e16c5a02458a127/browser/base/content/urlbarBindings.xml#1794 > toolkit/components/autocomplete/tests/unit/test_repeated_search_opens_popup. > js:35 > (Diff revision 2) > > + await promiseSearch; > > + > > + Assert.equal(controller.searchStatus, > > The way that Promises resolve means that there's _no_ opportunity for a > search to have started and been completed by the time we assert here, right? There's nothing starting a second search, we expect only one to run due to the handleText() call, and yes, the promise would proceed at the first search and the likely for 2 searches to fit is very low (provided we'd start 2, but then we should wait for both). > toolkit/components/autocomplete/tests/unit/test_repeated_search_opens_popup. > > + // Re-run the same search, it should still open the popup, but the search > > + // won't be executed again. > > + await runSearchAndCheck("test", false); > > Forgive me for not knowing the tests under autocomplete very well, but is it > necessary to close the popup after the test has completed? The nice things is that since it's an xpcshell test there is no real popup involved. We're simulating everything through the autocomplete xpidl interface, we know that the popupOpen setter has been invoked, what happens from that point on depends on the consumer. If the popupOpen setter of the Location bar would be broken we'd see quite a lot of failures. This is less expensive and more intermittent-safe than a complete mochitest-browser test.

(In reply to Marco Bonardo [::mak] from comment #13) > not warnings, just unclear or unsafe code. Casting a signed integer to > unsigned when we know it's >= 0 is safer than casting an unsigned to signed > without checking its value. Yep, gotcha. > > Actually, this code never did that check, and indeed all the consumers do > that check on their own. We could certainly centralize the check but not > here (since the controller is shared between any autocomplete field in > Firefox it's risky to change the behavior). Okay, understood. > The nice things is that since it's an xpcshell test there is no real popup > involved. We're simulating everything through the autocomplete xpidl > interface, we know that the popupOpen setter has been invoked, what happens > from that point on depends on the consumer. If the popupOpen setter of the > Location bar would be broken we'd see quite a lot of failures. > This is less expensive and more intermittent-safe than a complete > mochitest-browser test. Ah yes, thanks for clearing that up. It's not something I've seen too often - mochitest-browser seems to be the testing suite of choice for front-end. Neat to see this kind of design for low-level testability though.

hm, looks like some passwordmgr test is unhappy. The downside of sharing a single controller across all the form autocompletes around... TEST-UNEXPECTED-FAIL | toolkit/components/passwordmgr/test/mochitest/test_autocomplete_https_upgrade.html | TypeError: can't access dead object [log…] TEST-UNEXPECTED-FAIL | toolkit/components/passwordmgr/test/mochitest/test_autofocus_js.html | Check popup is now closed - got true, expected false [log…] TEST-UNEXPECTED-FAIL | toolkit/components/passwordmgr/test/mochitest/test_autofocus_js.html | Unexpected autocomplete popupshown event [log…] TEST-UNEXPECTED-FAIL | toolkit/components/passwordmgr/test/mochitest/test_basic_form_autocomplete.html | Test timed out. [log…] Exactly all these tests have bugs filed for timing out intermittently... I'll have to check what the passwordmgr is doing wrong :(

I suspect my approach won't bring anywhere. I think the password manager likes to fill up values when the popup is closed, and since the value was selected-filled before, it constantly hits this path. I could make the new code be specific for the awesomebar, but that's not great for technical debt. While investigating this though, I figured another detail, when we open a new tab we invoke URLBarSetURI() to reset the urlbar value to an empty string. The controller doesn't know about this, so its searchstring value sticks to the old one. This means the controller applies the optimization based on outdated values. Thus I'm now trying to reset the controller searchstring when value changes, so it can take its decisions with more confidence. Let's see where this approach brings me.

After a long investigation and some headache, I think there's no way to solve this globally without taking big risks in breaking other autocomplete consumers. Every time I tried something, some other consumer started failing. Thus, I reverted to the initial idea of just restoring the previous behavior. The old blur/focus was in practice detaching and reattaching the autocomplete controller, on those 2 operations the controller resets a bunch of internal caches. Thus I think the safest way forward is to listen for TabSelect and call detach/attach, so simulate what was happening before. If we had a perf win from not doing that work, this may revert that, but it's the less regression-prone path.

Comment on attachment 8872406 [details] Bug 1368074 - Autocomplete popup is not re-opened when repeating the same search in a new tab. Mike, could you please re-review this? I had to completely change the approach, as explained in the previous comments, and that means in the end I just restored the previous behavior in the browser and wrote a mochitest-browser. I'd like to fix this deeper down the code, but autocomplete has so many consumers with special requirements that it's very hard and regression-prone. this is a Try run: https://treeherder.mozilla.org/#/jobs?repo=try&revision=a76fa59905a4dc6dda8713a0f44f5f8b2da1c7b3 I also compared Talos and it didn't show anything of interest

Comment on attachment 8872406 [details] Bug 1368074 - Autocomplete popup is not re-opened when repeating the same search in a new tab. https://reviewboard.mozilla.org/r/143914/#review148332 I think this is a fine approach. Thanks for dealing with this mak, and sorry for causing this regression in the first place. :( ::: commit-message-cce4d:4 (Diff revision 3) > +*** > +additional fixes > + > +MozReview-Commit-ID: 9H1GvbwWLMv Nit - can probably remove this part of the commit message.

Pushed by mak77@bonardo.net: https://hg.mozilla.org/integration/autoland/rev/38d34be6d674 Autocomplete popup is not re-opened when repeating the same search in a new tab. r=mconley

Build ID: 20170601030206 User Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:55.0) Gecko/20100101 Firefox/55.0 Verified as fixed on Firefox Nightly 55.0a1 on Windows 10 x 64, Mac OS X 10.12 and Ubuntu 16.04 x64.