Looks like Private Windows abuse the autocomplete API by disabling *all* autocomplete actions, when it only wants to disable switch-to-tab. This is killing all new autocomplete functionality in private windows.

Marco: This is an autocomplete hard blocker, so needs done this iteration.

Blair, do you have a suggested way to handle this? That may help someone willing to jump into fixing this bug. I don't think we have support to disable single actions currently, right?

FTR, disabling switch-to-tab in private windows was done in bug 816527.

How about this? I feel like adding a mechanism to disable arbitrary actions would be a little over-engineered.

Should probably make that work with the old nsAutocomplete as well.

I assume we might want a test here, checking switch to tab is disabled but another action is not... That would be useful regardless of the approach we take. fwiw, I agree we should not go crazy about whitelisting actions.

Comment on attachment 8501921 [details] [diff] [review] 0001-Bug-1075450-Disable-only-switch-to-tab-instead-of-al.patch, v2 Review of attachment 8501921 [details] [diff] [review]: ----------------------------------------------------------------- So, first, we need a test. the original patch implementing this should have not been r+ed without a test. Or we would have catched this regression first! this should also fix this code in UnifiedComplete, by removing the switchToTabQuery: let queries = [ this._adaptiveQuery, this._switchToTabQuery, this._searchQuery ]; and the equivalent in nsPlacesAutoComplete. note that Bug 530209 is basically doing the same thing (you can look at the patch there for how it's doing), so if that lands first this could reuse that code, or viceversa. The approach is ok imo. note that this approach (adding more to searchparams) was discarded in bug 816527, but that imo was because at that time we only had a single action, so it was indeed easier to disable actions completely than to special case switch to tab. I think today this approach makes sense cause we have many actions. ::: browser/base/content/browser.js @@ +6946,5 @@ > } > > if (gURLBar && > !PrivateBrowsingUtils.permanentPrivateBrowsing) { > // Disable switch to tab autocompletion for private windows while here, please fix this trailing space. @@ +6947,5 @@ > > if (gURLBar && > !PrivateBrowsingUtils.permanentPrivateBrowsing) { > // Disable switch to tab autocompletion for private windows > // (not for "Always use private browsing" mode) ...and rephrase to remove the pointless parenthesis :) @@ +6949,5 @@ > !PrivateBrowsingUtils.permanentPrivateBrowsing) { > // Disable switch to tab autocompletion for private windows > // (not for "Always use private browsing" mode) > + let value = gURLBar.getAttribute("autocompletesearchparam") || ""; > + if (value.split(" ").indexOf("disable-switch-to-tab") == -1) { it would be simpler to use !value.contains("disable-switch-to-tab"), the likely we might have other strings matching this is so low it's not even worth paying the cost of split. ::: toolkit/components/places/UnifiedComplete.js @@ +523,5 @@ > : Prefs.emptySearchDefaultBehavior; > + > + let params = new Set(searchParam.split(" ")); > + this._enableActions = params.has("enable-actions"); > + this._disableSwitchToTab = params.has("disable-switch-to-tab"); Hm, if we'd have the patch from bug 530209, we could just remove the "openpage" behavior... Maybe worth filing a follow-up to investigate merging this with that. what about this._switchToTabEnabled = this._enableActions && !params.has... @@ +1155,5 @@ > // If actions are enabled and the page is open, add only the switch-to-tab > // result. Otherwise, add the normal result. > let url = escapedURL; > let action = null; > + if (this._enableActions && !this._disableSwitchToTab && openPageCount > 0) { and here just check this._switchToTabEnabled && openPageCount ::: toolkit/components/places/nsPlacesAutoComplete.js @@ +493,5 @@ > fixupSearchText(this._originalSearchString.toLowerCase()); > > + let params = new Set(aSearchParam.split(" ")); > + this._enableActions = params.has("enable-actions"); > + this._disableSwitchToTab = params.has("disable-switch-to-tab"); ditto

Comment on attachment 8501921 [details] [diff] [review] 0001-Bug-1075450-Disable-only-switch-to-tab-instead-of-al.patch, v2 Review of attachment 8501921 [details] [diff] [review]: ----------------------------------------------------------------- (Sorry I saw this so late - didn't get a needinfo) I'd rather have a general "private-window" param added, as a more generic solution that can handle any other actions/behaviour that may be impacted by private windows in the future.

(In reply to Blair McBride [:Unfocused] from comment #9) > I'd rather have a general "private-window" param added, as a more generic > solution that can handle any other actions/behaviour that may be impacted by > private windows in the future. Though, "private" or "private-window" would lie, cause it's not set in permanent private browsing mode. It's sort of risky to rely on a lie.

Er, oh. "disable-private-actions"? Either way, something that disables things by trait, rather than disables one specific thing.

Addressed all comments and added a test for "disable-switch-to-tab" and actions disabled. Do you think we still need a test with a real private window? I guess that would have to be a m-bc then?

A couple things: 1. we should use "disable-private-actions" as Blair suggested. 2. I think we should wait for bug 530209 at this point, it makes easier to fix this more properly. 3. I think the test might be fine. Surely a b-c test would involve more code and provide a better behavioral testing, but it would also much more expensive to make right (since it involves multiple windows and private-browsing)

the truth is this doesn't test that private-browsing windows set the correct params, having a test that just checks that would basically complete the coverage, without crazy interaction with autocomplete popups. So I wonder if we couldn't just touch browser_tabMatchesInAwesomebar_perwindowpb.js... does that test pass with your patch actually?

(In reply to Marco Bonardo [::mak] (needinfo? me) from comment #13) > 1. we should use "disable-private-actions" as Blair suggested. Will do. > 2. I think we should wait for bug 530209 at this point, it makes easier to > fix this more properly. SGTM. (In reply to Marco Bonardo [::mak] (needinfo? me) from comment #14) > So I wonder if we couldn't just touch > browser_tabMatchesInAwesomebar_perwindowpb.js... does that test pass with > your patch actually? I am sure we can adapt that... and no it doesn't pass. I don't quite understand why though, isn't that supposed to test behavior in normal non-private windows? The test name indicates otherwise... Waiting for bug 530209 then to continue.

(In reply to Tim Taubert [:ttaubert] from comment #15) > I am sure we can adapt that... and no it doesn't pass. I don't quite > understand why though, isn't that supposed to test behavior in normal > non-private windows? The test name indicates otherwise... yeah, now I'm really confused about what that test is testing... its name doesn't make sense at all.

Geez, I'm glad I'm not the only one that's been confused by that test. So I just tracked down what happened: * First we had browser_tabMatchesInAwesomebar.js * That original test included global PB test coverage * Thn we added support for per-window PB, but it was behind a compile time config, MOZ_PER_WINDOW_PRIVATE_BROWSING * So bug 806708 duplicated that test, introducing browser_tabMatchesInAwesomebar_perwindowpb.js which *removed* all PB testing when per-window PB was enabled, because browser_bug816527.js tests that * Then we removed the compile-time config, so per-window PB was always enabled * Then browser_tabMatchesInAwesomebar.js was removed, because it tested global PB mode, not the new per-window PB So now we have: * browser_tabMatchesInAwesomebar_perwindowpb.js which is really browser_tabMatchesInAwesomebar.js * browser_bug816527.js which is really browser_tabMatchesInAwesomebar_perwindowpb.js

Comment on attachment 8506284 [details] [diff] [review] 0001-Bug-1075450-Disable-only-switch-to-tab-instead-of-al.patch, v3 Thanks for investigating the tests madness! OK, let's clarify status of the bug. The approach is f+ but we need to address blair's comment, we'd like to wait for Alex patch (so we can use removeBehavior(openpage), and we need to clarify the test situation. I think at this point it's worth to do the renaming that Blair implicitly suggested * browser_tabMatchesInAwesomebar_perwindowpb.js to browser_tabMatchesInAwesomebar.js * browser_bug816527.js to browser_tabMatchesInAwesomebar_perwindowpb.js and we can try to add a test for this in the NEW browser_tabMatchesInAwesomebar_perwindowpb.js ?

Comment on attachment 8526650 [details] [diff] [review] 0001-Bug-1075450-Disable-some-Awesomebar-actions-for-priv.patch, v4 Review of attachment 8526650 [details] [diff] [review]: ----------------------------------------------------------------- ::: browser/base/content/test/general/browser_tabMatchesInAwesomebar_perwindowpb.js @@ +91,5 @@ > + > + // Execute the selected action. > + controller.handleEnter(true); > + }); > + }); These lines and above are the only real changes I made to browser_bug816527.js. All the other changes are just noise from renaming the tests :/

Patch without renaming tests.

Separate patch to rename tests.

VCS are stupid sometimes.

Comment on attachment 8526668 [details] [diff] [review] 0001-Bug-1075450-Disable-some-Awesomebar-actions-for-priv.patch, v5 Review of attachment 8526668 [details] [diff] [review]: ----------------------------------------------------------------- it looks good, not making cause you said still looking into the test. ::: browser/base/content/test/general/browser_bug816527.js @@ +79,5 @@ > + urlbar.value = testURL; > + controller.startSearch(testURL); > + > + // Wait for the Awesomebar popup to appear. > + waitForAwesomebarPopup(aDestWindow, function () { fwiw, we have promiseSearchComplete that maybe could help you here.. actually I just figured its name is not very nice cause "search" could also refer to the searchbar or fayt. but I'd not rename it here cause I don't want to bitrot Blair (hope those patches land soon...) (we also have promisePopupShown, promisePopupHidden, and promisePopupEvent) @@ +81,5 @@ > + > + // Wait for the Awesomebar popup to appear. > + waitForAwesomebarPopup(aDestWindow, function () { > + // Wait until there are at least two matches. > + waitForCondition(() => controller.matchCount > 1, function () { I wonder if this could be used to make promiseSearchComplete reliable on linux... does it work for you there? @@ +86,5 @@ > + // Select the second match. > + // TODO Remove one of those when bug 1067903 lands and the value > + // of the second entry starts with "moz-action:switchtab,". > + controller.handleKeyNavigation(KeyEvent.DOM_VK_DOWN); > + controller.handleKeyNavigation(KeyEvent.DOM_VK_DOWN); you should notify Blair of this change or he could rely on a previous green try and get orange.

(In reply to Marco Bonardo [::mak] (needinfo? me) from comment #26) > ::: browser/base/content/test/general/browser_bug816527.js > @@ +79,5 @@ > > + urlbar.value = testURL; > > + controller.startSearch(testURL); > > + > > + // Wait for the Awesomebar popup to appear. > > + waitForAwesomebarPopup(aDestWindow, function () { > > fwiw, we have promiseSearchComplete that maybe could help you here.. > actually I just figured its name is not very nice cause "search" could also > refer to the searchbar or fayt. but I'd not rename it here cause I don't > want to bitrot Blair (hope those patches land soon...) > (we also have promisePopupShown, promisePopupHidden, and promisePopupEvent) Ah, promisePopupShown() looks good, using that. > @@ +81,5 @@ > > + > > + // Wait for the Awesomebar popup to appear. > > + waitForAwesomebarPopup(aDestWindow, function () { > > + // Wait until there are at least two matches. > > + waitForCondition(() => controller.matchCount > 1, function () { > > I wonder if this could be used to make promiseSearchComplete reliable on > linux... does it work for you there? Yeah, my test seems to work fine on Linux. I can work on promiseSearchComplete() for bug 1073339 as a follow-up. > @@ +86,5 @@ > > + // Select the second match. > > + // TODO Remove one of those when bug 1067903 lands and the value > > + // of the second entry starts with "moz-action:switchtab,". > > + controller.handleKeyNavigation(KeyEvent.DOM_VK_DOWN); > > + controller.handleKeyNavigation(KeyEvent.DOM_VK_DOWN); > > you should notify Blair of this change or he could rely on a previous green > try and get orange. I can wait until bug 1067903 lands, hope that happens soon.

Try run of latest patch: https://tbpl.mozilla.org/?tree=Try&rev=bfc0adef87aa

Comment on attachment 8527284 [details] [diff] [review] 0001-Bug-1075450-Disable-some-Awesomebar-actions-for-priv.patch, v6 Will update the patch now that the default action is selected by default.

Simplified the test further.

Comment on attachment 8527564 [details] [diff] [review] 0001-Bug-1075450-Disable-some-Awesomebar-actions-for-priv.patch, v7 Review of attachment 8527564 [details] [diff] [review]: ----------------------------------------------------------------- ::: browser/base/content/test/general/browser_bug816527.js @@ +88,2 @@ > > + // Wait until there are at least two matches. this comment likely needs a brief update

https://hg.mozilla.org/integration/fx-team/rev/a093db3222c6 https://hg.mozilla.org/integration/fx-team/rev/f7d199020d74 https://hg.mozilla.org/integration/fx-team/rev/8ec21ff65ce3

https://hg.mozilla.org/mozilla-central/rev/a093db3222c6 https://hg.mozilla.org/mozilla-central/rev/f7d199020d74 https://hg.mozilla.org/mozilla-central/rev/8ec21ff65ce3

Verified fixed on Nightly 37.0a1 (2014-12-01) and Aurora 36.0a2 (2014-12-01) using Windows 7 64-bit, Ubuntu 12.04 LTS 32-bit and Mac OS X 10.9.5 with the 'browser.urlbar.unifiedcomplete' pref set to true.