Steps to reproduce: 1. Add an open search provider. 2. From "Change Search Settings", uncheck it from the list of one-click providers. 3. From chrome://browser/content/search/engineManager.xul remove that provider. 4. Add that provider again. Expected result: this provider you just added is shown in the one-off providers at the bottom of the search suggestion panel. Actual result: that newly added provider is hidden.

[good first bug] ?

I'd be interested in taking a look at this.

Go for it! I've granted you Bugzilla permissions to edit bugs, which means you should be able to assign bugs to yourself when you want. We should also get you Level 1 commit access so you can push your patches to try, feel free to email me or ping on IRC separately for information about that.

I'm going to give you some points to help you get started: The code removing engines in the preference window is here: http://hg.mozilla.org/mozilla-central/annotate/b3f84cf78dc2/browser/components/preferences/search.js#l216 Similar code for the in-content version of the preferences UI is at http://hg.mozilla.org/mozilla-central/annotate/b3f84cf78dc2/browser/components/preferences/in-content/search.js#l270 Given that it's possible for engines to be removed by add-ons, I think it would be safer to also add a check at the time we add new engines. The code that adds engines is at https://hg.mozilla.org/releases/mozilla-beta/annotate/137baee3dda4/browser/base/content/urlbarBindings.xml#l1200

Thank Florian, and thanks Gavin. It looks like what your talking about is insuring that hidden engines that are removed are also removed from the hidden provider list defined at http://mxr.mozilla.org/mozilla-central/source/browser/components/preferences/search.xul#29. Would a call to an appropriate unhide operation work if inserted at the appropriate places (Before removing the engine, do a check to see if it is hidden, if so call the appropriate unhide on it.)? Then for adding an engine we do a check to ensure that the engine is visible and not in the list of hidden providers. Does this sound about right to you Florian?

(In reply to Chris from comment #5) > Thank Florian, and thanks Gavin. > > It looks like what your talking about is insuring that hidden engines that > are removed are also removed from the hidden provider list defined at > http://mxr.mozilla.org/mozilla-central/source/browser/components/preferences/ > search.xul#29. Would a call to an appropriate unhide operation work if > inserted at the appropriate places (Before removing the engine, do a check > to see if it is hidden, if so call the appropriate unhide on it.)? > > Then for adding an engine we do a check to ensure that the engine is visible > and not in the list of hidden providers. > > Does this sound about right to you Florian? It does! :-) http://mxr.mozilla.org/mozilla-central/source/browser/components/preferences/search.js#168 is the code changing the preference with the list of hidden engines.

I'm trying to call into the saveOneClickEngines method from urlbarbinding.xml. Any ideas of how I can do this after the engine is added? Thanks

(In reply to Chris from comment #7) > I'm trying to call into the saveOneClickEngines method from > urlbarbinding.xml. Any ideas of how I can do this after the engine is > added? Thanks I don't think you can call this method (which lives in the preferences UI code) from the urlbarbinding.xml file. You'll need to duplicate this code. It will be a little bit different anyway, as you also need code that loads the value of the preference.

I'm having some issues with removing an engine from the hidden list on add in https://hg.mozilla.org/releases/mozilla-beta/annotate/137baee3dda4/browser/base/content/urlbarBindings.xml#l1200. The steps that I'm taking is: 1: Add a one off engine (I'm using the firefox add-on search) from the search bar 2: Make sure that the engine is set to hidden in search preferences. 3: Remove the engine via chrome://browser/content/search/engineManager.xul 4: Repeat step 1 and notice that the engine is present in the list of one click search providers and is absent from the hidden prefs in prefs.js. 5: Repeat steps 2 and 3. 6: Re-add the engine as in step 1. 7: Notice that the engine is present in the list of one click search providers and has a check-mark by it as expected on the search preferences page, however if you check prefs.js you will notice that it is still present under hidden one-click search providers, and it will disappear from the list of one click search providers the next time firefox starts up. I've checked what's happening and it looks like the preference is being written to after the xml is finished but I don't know what's writing to it or where it's getting it's data from. It also looks like this method isn't being called in the case where the search engine is added through the Firefox add on page. (Need to know what method is being called when a search provider is added through a web page.)

Comment on attachment 8549803 [details] [diff] [review] bug-1106926.patch Review of attachment 8549803 [details] [diff] [review]: ----------------------------------------------------------------- Thanks for the patch! :-) (In reply to Chris from comment #9) > 7: Notice that the engine is present in the list of one click search > providers and has a check-mark by it as expected on the search preferences > page, however if you check prefs.js you will notice that it is still present > under hidden one-click search providers The prefs.js file isn't saved to disk immediately. If you want to check for the value of the preference, instead of looking at the file directly, you can look in about:config. > It also looks like this method isn't being called in the case where the > search engine is added through the Firefox add on page. (Need to know what > method is being called when a search provider is added through a web page.) The prompt shown in that case is displayed by http://mxr.mozilla.org/mozilla-central/source/toolkit/components/search/nsSearchService.js#1475 Unfortunately that code is in toolkit/ and shared with other applications that don't have the concept of hidden one-click providers. Maybe instead of cleaning up the list in urlbarbinding.xml, we could do it from http://hg.mozilla.org/mozilla-central/annotate/cac6192956ab/browser/components/nsBrowserGlue.js#l409 when receiving the "engine-added" notification. ::: browser/base/content/urlbarBindings.xml @@ +1236,3 @@ > let installCallback = { > onSuccess: function(engine) { > + //Get the list of currently hidden engines. nit: Put a space between "//" and the first word. @@ +1243,5 @@ > + hiddenEngines = pref ? pref.split(",") : []; > + } catch(e) { > + hiddenEngines = []; > + } > + //Create a new list for hidden engines that does not include Nit: trailing whitespace @@ +1244,5 @@ > + } catch(e) { > + hiddenEngines = []; > + } > + //Create a new list for hidden engines that does not include > + //the newly added engine if it is present. This can be simplified using: hiddenEngines = hiddenEngines.filter(...) See https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Array/filter

It doesn't look like the engine is being passed in the subject when an engine is added or loaded. I also can't access engine.shown (Shows up as undefined in from http://mxr.mozilla.org/mozilla-central/source/toolkit/components/search/nsSearchService.js#1475). Do you know some way around this?

(In reply to Chris from comment #11) > It doesn't look like the engine is being passed in the subject when an > engine is added or loaded. http://mxr.mozilla.org/mozilla-central/source/toolkit/components/search/nsSearchService.js#3589 looks to me like it's passing the engine. The implementation of notifyAction is at http://mxr.mozilla.org/mozilla-central/source/toolkit/components/search/nsSearchService.js#882 > I also can't access engine.shown (Shows up as > undefined in from > http://mxr.mozilla.org/mozilla-central/source/toolkit/components/search/ > nsSearchService.js#1475). Do you know some way around this? Why do you need that? The urlbarBindings.xml part of your previous patch didn't need it, right? The .shown property only exists in the wrapper used by the cache of the preference code, it's created at http://mxr.mozilla.org/mozilla-central/source/browser/components/preferences/in-content/search.js#289

So what I was doing wrong is that I was not using the QueryInterface method of subject to get the engine. I'm wondering if it might not be better to just add the logic to the observer method in nsBrowserGlue for both add and remove as opposed to having the add be there and the remove be in a completely different place. Is there any circumstances that we care about in which an engine can be added/removed without nsBrowserGlue being notified?

I went ahead and moved the remove logic as per my last comment, let me know if this looks good and I'll get started with writing some regression tests in the meantime.

Comment on attachment 8550993 [details] [diff] [review] bug-1106926.patch Review of attachment 8550993 [details] [diff] [review]: ----------------------------------------------------------------- Thanks for looking at writing a test :-). ::: browser/components/nsBrowserGlue.js @@ +409,5 @@ > }); > #endif > break; > case "browser-search-engine-modified": > + if(data === "engine-added" || data === "engine-removed") { I agree with your idea of handling both cases at the same place, rather than doing some of it in the preferences UI. You'll want to add a comment here in the code explaining why we need to cleanup this list. nit: space between "if" and "(". The other tests around don't seem to use === so let's stick with ==. @@ +417,5 @@ > + engine = subject.QueryInterface(Ci.nsISearchEngine); > + let pref = > + Services.prefs.getCharPref("browser.search.hiddenOneOffs"); > + hiddenEngines = pref ? pref.split(",") : []; > + hiddenEngines = hiddenEngines.filter(x => x !== engine.name); This is the only place where you use the 'engine' variable, so you could as well make that variable contain the engine name instead, to avoid calling the name getter several times. @@ +419,5 @@ > + Services.prefs.getCharPref("browser.search.hiddenOneOffs"); > + hiddenEngines = pref ? pref.split(",") : []; > + hiddenEngines = hiddenEngines.filter(x => x !== engine.name); > + Services.prefs.setCharPref("browser.search.hiddenOneOffs", hiddenEngines.join(",")); > + } catch (ex) { What are the exceptions you expect to catch? QueryInterface could throw an exception if subject doesn't implement the interface, but that should never happen, right? getCharPref will throw if the preference doesn't exist. I'm not sure why I didn't add a default value for that preference near http://mxr.mozilla.org/mozilla-central/source/browser/app/profile/firefox.js#417 but that's something we could do to avoid needing try/catch blocks wherever that preference is used (eg http://hg.mozilla.org/mozilla-central/annotate/6446c26b45f9/browser/base/content/urlbarBindings.xml#l1142).

I'm thinking that using 2 search providers would be ideal. 1. Both of these would be added to the hiddenOneOffs preference. 2. One search engine would then be removed and then a check would be performed to ensure that the other service is still listed in the hiddenOneOffs preference (Ensure that removing a hidden search provider does not remove other search providers from the hidden one offs list). 3. The removed search provider would then be added back and a second check would be performed to ensure that the re-added search provider is not in hiddenOneOffs but that the hidden search provider is still listed in the hiddenOneOffs (Ensure that re-added search providers are listed in the one offs list while making sure that there are no side effects of removing other search providers from hiddenOneOffs). As for the search providers I would prefer to use 2 mock search services instead that are then removed in test cleanup since that would make the test more sturdy as opposed to relying on default providers such as DDG which could conceivably change in the future. I want to put this test in browser/components/search/test since this bug is related to search providers, but I could also see an argument of putting it in browser/components/preferences/test since it deals with preferences.

(In reply to Chris from comment #16) The test description looks good overall, thanks! :-) > I'm thinking that using 2 search providers would be ideal. > 1. Both of these would be added to the hiddenOneOffs preference. > 2. One search engine would then be removed and then a check would be > performed to ensure that the other service is still listed in the > hiddenOneOffs preference (Ensure that removing a hidden search provider does > not remove other search providers from the hidden one offs list). > 3. The removed search provider would then be added back and a second check > would be performed to ensure that the re-added search provider is not in > hiddenOneOffs but that the hidden search provider is still listed in the > hiddenOneOffs (Ensure that re-added search providers are listed in the one > offs list while making sure that there are no side effects of removing other > search providers from hiddenOneOffs). Before 3. you would need to set the pref back to the value it had after 1., otherwise you are not testing that we are actually cleaning up the preference when an engine is added. > As for the search providers I would prefer to use 2 mock search services > instead that are then removed in test cleanup since that would make the test > more sturdy as opposed to relying on default providers such as DDG which > could conceivably change in the future. Yes, that's better. There are already search tests doing this, so you can find existing code samples. > I want to put this test in browser/components/search/test since this bug is > related to search providers Seems fine.

Putting this patch up for review so I can make any changes needed while I wait for level 1 access so I can push to try server. Let me know if we should be adding a default pref for hiddenOneOffs as a part of this patch or if that should be part of it's own bug.

Removed the e10s exclusion from the test case.

Comment on attachment 8554837 [details] [diff] [review] bug-1106926.patch Review of attachment 8554837 [details] [diff] [review]: ----------------------------------------------------------------- Thanks for the updated patch! (In reply to Chris from comment #18) > Let me know if we should be adding a default pref for hiddenOneOffs as a > part of this patch or if that should be part of it's own bug. Adding it as part of this bug is fine. We could also have done it as part of bug 1121417 - would you be interested in working on that bug next? ::: browser/app/profile/firefox.js @@ +402,5 @@ > pref("browser.search.context.loadInBackground", false); > > pref("browser.search.showOneOffButtons", true); > > +// Default for engines not shown in the one offs panel. 'Default' isn't meaningful in this comment as the whole file is setting default values. Suggestion: comma separated list of engines to hide in the search panel ::: browser/components/nsBrowserGlue.js @@ +414,5 @@ > break; > case "browser-search-engine-modified": > + // Check to make sure that hiddenOneOffs pref is properly updated > + // and that engines that are added or removed are visible in the > + // oneOff search provider list. Indent this comment like the code right after it. "properly updated" is a bit vague and we are not really ensuring that removed engines are visible. Suggested phrasing: Ensure we cleanup the hiddenOneOffs pref when removing an engine, and that newly added engines are visible. @@ +418,5 @@ > + // oneOff search provider list. > + if (data == "engine-added" || data == "engine-removed") { > + let engineName = ""; > + try { > + engineName = subject.QueryInterface(Ci.nsISearchEngine).name; What are the cases where you expect this to throw? @@ +423,5 @@ > + } catch (ex) { > + Cu.reportError(ex); > + } > + let pref = > + Services.prefs.getCharPref("browser.search.hiddenOneOffs"); nit: Indent with only 2 spaces. @@ +427,5 @@ > + Services.prefs.getCharPref("browser.search.hiddenOneOffs"); > + let hiddenEngines = pref ? pref.split(",") : []; > + hiddenEngines = hiddenEngines.filter(x => x !== engineName); > + Services.prefs.setCharPref("browser.search.hiddenOneOffs", > + hiddenEngines.join(",")); nit: align this line with the other parameter. ::: browser/components/search/test/browser.ini @@ +3,5 @@ > support-files = > 426329.xml > 483086-1.xml > 483086-2.xml > + 1106926.xml Could this test re-use one of the other engine files already available in this folder? @@ +12,5 @@ > testEngine_mozsearch.xml > > [browser_405664.js] > [browser_426329.js] > +[browser_1106926.js] We now try to give descriptive names to new tests instead of bug numbers. Maybe browser_hiddenOneOffs_cleanup.js ? ::: browser/components/search/test/browser_1106926.js @@ +4,5 @@ > +let ss = Services.search; > +let origHidden = Services.prefs.getCharPref("browser.search.hiddenOneOffs"); > + > +function test() { > + waitForExplicitFinish(); nit: please avoid trailing whitespace (you also have some on line 14 and 58 in this file). @@ +18,5 @@ > + Ci.nsISearchEngine.DATA_XML, "data:image/png;base64,iVBORw0KGgoAAAANSUhEUgAAABAAAAAQCAIAAACQkWg2AAABGklEQVQoz2NgGB6AnZ1dUlJSXl4eSDIyMhLW4Ovr%2B%2Fr168uXL69Zs4YoG%2BLi4i5dusTExMTGxsbNzd3f37937976%2BnpmZmagbHR09J49e5YvX66kpATVEBYW9ubNm2nTphkbG7e2tp44cQLIuHfvXm5urpaWFlDKysqqu7v73LlzECMYIiIiHj58mJCQoKKicvXq1bS0NKBgW1vbjh074uPjgeqAXE1NzSdPnvDz84M0AEUvXLgAsW379u1z5swBen3jxo2zZ892cHB4%2BvQp0KlAfwI1cHJyghQFBwfv2rULokFXV%2FfixYu7d%2B8GGqGgoMDKyrpu3br9%2B%2FcDuXl5eVA%2FAEWBfoWHAdAYoNuAYQ0XAeoUERFhGDYAAPoUaT2dfWJuAAAAAElFTkSuQmCC", > + false); > + ss.addEngine("http://mochi.test:8888/browser/browser/components/search/test/testEngine.xml", > + Ci.nsISearchEngine.DATA_XML, "data:image/png;base64,iVBORw0KGgoAAAANSUhEUgAAABAAAAAQCAIAAACQkWg2AAABGklEQVQoz2NgGB6AnZ1dUlJSXl4eSDIyMhLW4Ovr%2B%2Fr168uXL69Zs4YoG%2BLi4i5dusTExMTGxsbNzd3f37937976%2BnpmZmagbHR09J49e5YvX66kpATVEBYW9ubNm2nTphkbG7e2tp44cQLIuHfvXm5urpaWFlDKysqqu7v73LlzECMYIiIiHj58mJCQoKKicvXq1bS0NKBgW1vbjh074uPjgeqAXE1NzSdPnvDz84M0AEUvXLgAsW379u1z5swBen3jxo2zZ892cHB4%2BvQp0KlAfwI1cHJyghQFBwfv2rULokFXV%2FfixYu7d%2B8GGqGgoMDKyrpu3br9%2B%2FcDuXl5eVA%2FAEWBfoWHAdAYoNuAYQ0XAeoUERFhGDYAAPoUaT2dfWJuAAAAAElFTkSuQmCC", > + false, addEngineCallback); I think this test could be more readable if instead of using callbacks you used a helper like http://mxr.mozilla.org/mozilla-central/source/browser/components/search/test/browser_searchbar_openpopup.js#27 returning a promise. @@ +32,5 @@ > + let removeEngine = ss.getEngineByName("secondengine"); > + ss.removeEngine(removeEngine); > + let hiddenPrefList = Services.prefs.getCharPref("browser.search.hiddenOneOffs").split(","); > + is(hiddenPrefList.length, 1, "hiddenOneOffs has the correct engine count post removal."); > + is(hiddenPrefList.find(x => x == "secondengine"), undefined, https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Array/includes is a shorter way to do what you want here.

pushed to try server https://treeherder.mozilla.org/#/jobs?repo=try&revision=d57b35478ffb

Accidently deleted my test and had to re-add it.

(In reply to Chris from comment #22) > pushed to try server > > https://treeherder.mozilla.org/#/jobs?repo=try&revision=d57b35478ffb In the future, please use http://trychooser.pub.build.mozilla.org/ to select only the jobs you need (in this case you would have needed something like try: -b do -p linux,linux64,macosx64,win32 -u mochitest-bc -t none).

I actually need that job cancelled, it's using a patch that somehow ended up deleting my tests. I've tried doing it myself but it doesn't appear to accept my credentials for it. Is level 1 not sufficient to cancel try jobs?

(In reply to Chris from comment #25) > I actually need that job cancelled I cancelled it. > Is level 1 not sufficient to cancel try jobs? I don't know the answer to that question.

Thanks Florian, sorry for the trouble. New try build: https://treeherder.mozilla.org/#/jobs?repo=try&revision=5ae08c956b8c

Comment on attachment 8556623 [details] [diff] [review] bug-1106926.patch Review of attachment 8556623 [details] [diff] [review]: ----------------------------------------------------------------- ::: browser/components/nsBrowserGlue.js @@ +412,5 @@ > }); > #endif > break; > case "browser-search-engine-modified": > + // Esnure we cleanup the hiddenOneOffs pref when removing - typo: "Ensure", not "Esnure" - there's a trailing space on this line. - The indent isn't fixed. I wrote in my previous comment that this line should be aligned with the next code line. @@ +418,5 @@ > + if (data == "engine-added" || data == "engine-removed") { > + let engineName = ""; > + try { > + engineName = subject.QueryInterface(Ci.nsISearchEngine).name; > + } catch (ex if ex instanceof NS_ERROR_NO_INTERFACE) { I asked before "What are the cases where you expect this to throw?". Do you know any case where we receive this notification with subject that isn't an instance of nsISearchEngine? @@ +426,5 @@ > + Services.prefs.getCharPref("browser.search.hiddenOneOffs"); > + let hiddenEngines = pref ? pref.split(",") : []; > + hiddenEngines = hiddenEngines.filter(x => x !== engineName); > + Services.prefs.setCharPref("browser.search.hiddenOneOffs", > + hiddenEngines.join(",")); nit: remove one space here to have the second parameter aligned with the first one. ::: browser/components/search/test/browser.ini @@ +3,5 @@ > support-files = > 426329.xml > 483086-1.xml > 483086-2.xml > + testEngine_dupe.xml I think this list is alphabetically sorted, so this new item should go right after testEngine.xml. ::: browser/components/search/test/browser_hiddenOneOffs_cleanup.js @@ +1,5 @@ > +/* This Source Code Form is subject to the terms of the Mozilla Public > + * License, v. 2.0. If a copy of the MPL was not distributed with this > + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */ > +let ss = Services.search; > +let sp = Services.prefs; I don't find these 2 variables really helpful, they make the rest of the code harder to read. @@ +6,5 @@ > +let origHidden = sp.getCharPref("browser.search.hiddenOneOffs"); > + > +function test() { > + waitForExplicitFinish(); > + promiseNewEngine("testEngine_dupe.xml"); You need to wait for this promise to resolve. Using helpers that return promises only makes the code more readable if you use add_task and yield to wait for the promises to resolve instead of using .then(). The browser_searchbar_openpopup.js example I mentioned in my previous comment uses this; let me know if you need more help to understand it. @@ +58,5 @@ > + }); > +} > + > +function setPrefs() { > + let resetPrefs = "Foo,secondengine"; This would probably be better as a const at the top of the file.

> I asked before "What are the cases where you expect this to throw?". Do you know any case where we > receive this notification with subject that isn't an instance of nsISearchEngine? I don't believe there should ever be one. I originally got the code for the QueryInterface call at http://mxr.mozilla.org/mozilla-central/source/browser/components/nsBrowserGlue.js#39 and I was leaving it in the try/catch block to be safe. I'll remove it in the next patch if it doesn't look like it's actually possible to reach that notification without an nsISearchEngine for subject. I've incorporated add_task(function* ... to the test case and it's looking a lot nicer now. I should have another patch ready tomorrow.

I removed the try/catch statement since I couldn't think of any time that notification can actually be reached without the subject being an nsISearchEngine. I can re-add it if we want to be absolutely safe. I replaced .then statements with add_task and yield, and I split up the test into two seperate tests, one for removing search engines and one for adding them. I also added some info statements to put out more information regarding what the test is doing and when it's doing it. There shouldn't be any more trailing whitespace or misspellings, and I went through and made sure that the multi-line arguments were all properly aligned.

Updated to address the relavent portions of bug 1121417 since using Services.prefs would affect the observers ability to properly clean up a hidden search service with diacritics included in it's name.

Comment on attachment 8557524 [details] [diff] [review] bug-1106926.patch Review of attachment 8557524 [details] [diff] [review]: ----------------------------------------------------------------- (In reply to Chris from comment #31) > Created attachment 8557524 [details] [diff] [review] > bug-1106926.patch > > Updated to address the relavent portions of bug 1121417 since using > Services.prefs would affect the observers ability to properly clean up a > hidden search service with diacritics included in it's name. I think we should do this change as part of bug 1121417 and not complicate the almost finished patch we have here. I'll review the previous version of the patch. ::: browser/components/nsBrowserGlue.js @@ +422,5 @@ > + let hiddenPref = > + ns.Preferences.get("browser.search.hiddenOneOffs"); > + let hiddenEngines = hiddenPref ? hiddenPref.split(",") : []; > + hiddenEngines = hiddenEngines.filter(x => x !== engineName); > + Services.prefs.setCharPref("browser.search.hiddenOneOffs", If you use Preferences.jsm to read the value of the preference, you also need to use it to write the value.

Comment on attachment 8557222 [details] [diff] [review] bug-1106926.patch Review of attachment 8557222 [details] [diff] [review]: ----------------------------------------------------------------- Looks good, can you please remove the trailing space and push this rewritten test to try? Thanks! ::: browser/components/nsBrowserGlue.js @@ +412,5 @@ > }); > #endif > break; > case "browser-search-engine-modified": > + // Ensure we cleanup the hiddenOneOffs pref when removing trailing whitespace.

Try server push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=08629b3359e4

Try run was successful. Marking this for checkin.

Comment on attachment 8558240 [details] [diff] [review] bug-1106926.patch Please use the checkin-needed keyword.

(In reply to Chris from comment #25) > Is level 1 not sufficient to cancel try jobs? It should be! Odd that it didn't work for you. Perhaps you could request an LDAP password reset from server-ops and see if that helps (email me if you need info on how to do that).

Verified as fixed using Nightly 38.0a1 2015-02-08 under Ubuntu 12.04 LTS 32-bit, Windows 7 64-bit and Mac OS X 10.9.5.

Comment on attachment 8558240 [details] [diff] [review] bug-1106926.patch Approval Request Comment [Feature/regressing bug #]: bug 1088660 (new search bar) [User impact if declined]: One off engines that are removed while hidden will stay hidden when re-added. [Describe test coverage new/current, TreeHerder]: Tests included in the patch. [Risks and why]: low risk; most of the patch is test additions. [String/UUID change made/needed]: none.

Comment on attachment 8558240 [details] [diff] [review] bug-1106926.patch Already on Aurora.

Comment on attachment 8558240 [details] [diff] [review] bug-1106926.patch This looks like a pretty safe fix for an edge case that has been on m-c/Aurora for a month and has been verified. I think it's reasonable to take this fix in Beta 4. Beta+

Patch for uplifting to 37. This needs to land before the fix for bug 11121417 can land in 37.

I had to back this out for hitting mochitest-bc failures. Please rebase your patch on top of mozilla-beta tip. https://hg.mozilla.org/releases/mozilla-beta/rev/89b593b91e5e https://treeherder.mozilla.org/logviewer.html#?job_id=293807&repo=mozilla-beta

ni Chris for rebasing this patch in order to consider this fix for Beta 5 by Thu, Mar 12.

Apologies on that, the version of the patch that I based the branch patch on used the Array.include method which isn't available on beta. I rebased this patch on top of mozilla-beta tip and all search tests are passing. Ryan do you know if it's possible to push the beta branch to the try server?

Sure is :)

Pushed both this patch and the patch for bug 1121417 to try server: https://treeherder.mozilla.org/#/jobs?repo=try&revision=9f287a761cea I didn't run the e10s tests but those should be fine if everything else is fine.

e10s jobs only run on mozilla-central (and will mostly fail on Try if pushing a different branch), so no worries on that one :)

Right, I just remembered that e10s isn't in beta.

Verified as fixed Firefox 37 beta 5 under Ubuntu 12.04 LTS 32-bit, Windows 7 64-bit and Mac OS X 10.9.5.