I was infected by the searchme malware (see bug 1010080) a while ago and today I found out that it had replaced the yahoo.xml file under profiledir/searchplugins with its own copy which redirected me to a different URL and hence I was not getting the new Yahoo experience. Perhaps we should consider not letting these files override the files that we ship if they have the same name?

I don't really know this part of the code, moving the ni to Gavin who likely knows.

(In reply to :Ehsan Akhgari (not reading bugmail, needinfo? me!) from comment #0) > Perhaps we should consider not letting these files override the files that > we ship if they have the same name? Yes, we probably should. It would also fix a related problem of old user-installed engines being preferred to our defaults. There are a lot of baked in assumptions about the engine location loading priority that might be a bit tricky to unravel, I'd have to think a bit more about other implications this might have.

Note that bug 353056 means that it's impossible to get into this situation apart from the malware or manually-putting-files-in-your-profile cases, so we likely wouldn't be breaking any user expectations here.

Mike, flagging you for feedback for a sanity check, as I think you know (or at least knew at some point!) a lot about the load order of our search plugins. If this all looks fine I'll write a test and request review from someone else later.

Comment on attachment 8612365 [details] [diff] [review] Patch This looks correct, behaviour-wise, to me. The tests are what I'll want to review closely.

This patch changes the load order of search engines. The new order is: - first the distribution engines - then the jar engines - then everything else, in the same order as they were loaded before. To be able to have this order, I had to break the NS_APP_SEARCH_DIR_LIST list in tow, so that I have a separate list for only the distribution engines. I created a new directory service key for this list: NS_APP_DISTRIBUTION_SEARCH_DIR_LIST (do I need a separate review for this change in xpcom/? Or should I just move that define to browser/components/dirprovider/DirectoryProvider.cpp?). This is implemented in browser/ and in mobile/ (I guess I'll need a separate review for that file), afaik they are the only 2 applications that actually have distribution engines that we care about. For unit tests (and other applications like Thunderbird and SeaMonkey), I put try/catch blocks around the code using this new key. The new xpcshell tests ensure that: - a search plugin in the searchplugins/ directory of the profile doesn't override an engine loaded from jar. - a search plugin loaded from an add-on in the profile doesn't override an engine loaded from jar (not directly related to the change made by this patch, but apparently we didn't have any coverage for loading engines from add-ons, so I also added a test covering that). - a search plugin loaded from a distribution DOES override an engine loaded from jar. Each new test has a sync and async version to verify both init code paths of the search service. https://treeherder.mozilla.org/#/jobs?repo=try&revision=663559a720d3 Needinfo on Mike who wanted to have another look at the tests.

(In reply to Florian Quèze [:florian] [:flo] from comment #6) > https://treeherder.mozilla.org/#/jobs?repo=try&revision=663559a720d3 Lots of developer tools oranges on that push, I pushed again based on a newer fx-team revision and it's now green: https://treeherder.mozilla.org/#/jobs?repo=try&revision=144346c2e83e

Comment on attachment 8621153 [details] [diff] [review] Patch v2 Review of attachment 8621153 [details] [diff] [review]: ----------------------------------------------------------------- And I thought the search service was complicated first time I saw it a year or so ago :) But yeah, that's just an objective fact and not a comment on this patch. LGTM and I think the tests are comprehensive and check what's necessary, but please wait for mconnor's feedback. ::: toolkit/components/search/nsSearchService.js @@ +3597,5 @@ > + let distDirs = []; > + let locations; > + try { > + locations = getDir(NS_APP_DISTRIBUTION_SEARCH_DIR_LIST, > + Ci.nsISimpleEnumerator); nit: looks like one extra space is needed to line up correctly @@ +3624,5 @@ > + let otherDirs = []; > + locations = getDir(NS_APP_SEARCH_DIR_LIST, Ci.nsISimpleEnumerator); > + while (locations.hasMoreElements()) { > + let dir = locations.getNext().QueryInterface(Ci.nsIFile); > + // ... but skip the application directory if we are loading from JAR. I wonder if it is worth saying *why* we skip the app dir here? @@ +3635,5 @@ > + // Add dir to otherDirs if it contains any files. > + yield checkForSyncCompletion(iterator.next()); > + otherDirs.push(dir); > + } catch (ex if ex.result != Cr.NS_ERROR_ALREADY_INITIALIZED) { > + // Catch for StopIteration exception. hrm - shouldn't the condition check for that specific exception then? If there's a more subtle reason for it being written this way, please update the comment to reflect what it is.

Comment on attachment 8621153 [details] [diff] [review] Patch v2 The distribution directory bit is neat. We probably need more of those tests, but at least we have one now. ;)

(In reply to Mark Hammond [:markh] from comment #8) > Comment on attachment 8621153 [details] [diff] [review] > Patch v2 > > Review of attachment 8621153 [details] [diff] [review]: > ----------------------------------------------------------------- > > And I thought the search service was complicated first time I saw it a year > or so ago :) But yeah, that's just an objective fact and not a comment on > this patch. > > LGTM and I think the tests are comprehensive and check what's necessary, but > please wait for mconnor's feedback. Thanks for the review! :-) > ::: toolkit/components/search/nsSearchService.js > @@ +3597,5 @@ > > + let distDirs = []; > > + let locations; > > + try { > > + locations = getDir(NS_APP_DISTRIBUTION_SEARCH_DIR_LIST, > > + Ci.nsISimpleEnumerator); > > nit: looks like one extra space is needed to line up correctly Fixed there, and at a second place too. > @@ +3624,5 @@ > > + let otherDirs = []; > > + locations = getDir(NS_APP_SEARCH_DIR_LIST, Ci.nsISimpleEnumerator); > > + while (locations.hasMoreElements()) { > > + let dir = locations.getNext().QueryInterface(Ci.nsIFile); > > + // ... but skip the application directory if we are loading from JAR. > > I wonder if it is worth saying *why* we skip the app dir here? I expended that comment a bit to make it slightly more descriptive, but this will all be cleaned up with bug 1169459 (I haven't requested review there yet because I want to land first all the patches we want to uplift to 40). > @@ +3635,5 @@ > > + // Add dir to otherDirs if it contains any files. > > + yield checkForSyncCompletion(iterator.next()); > > + otherDirs.push(dir); > > + } catch (ex if ex.result != Cr.NS_ERROR_ALREADY_INITIALIZED) { > > + // Catch for StopIteration exception. > > hrm - shouldn't the condition check for that specific exception then? If > there's a more subtle reason for it being written this way, please update > the comment to reflect what it is. Makes sense, but I don't fully know this code, I have just duplicated it, so if we should improve it, I would prefer to do it in a follow-up.

(In reply to Florian Quèze [:florian] [:flo] from comment #6) > This is implemented in browser/ and in mobile/ (I guess I'll need a separate > review for that file), afaik they are the only 2 applications that actually > have distribution engines that we care about. Oops, I just noticed I forgot to request that someone working on Fennec had a look at the (mostly straight forward) changes to mobile/android/components/DirectoryProvider.js. Sorry about that :-/. Margaret, could you please check if this needs a review after the fact, and if so set the r? flag? Thanks!

(In reply to Florian Quèze [:florian] [:flo] from comment #13) > (In reply to Florian Quèze [:florian] [:flo] from comment #6) > > > This is implemented in browser/ and in mobile/ (I guess I'll need a separate > > review for that file), afaik they are the only 2 applications that actually > > have distribution engines that we care about. > > Oops, I just noticed I forgot to request that someone working on Fennec had > a look at the (mostly straight forward) changes to > mobile/android/components/DirectoryProvider.js. Sorry about that :-/. > > Margaret, could you please check if this needs a review after the fact, and > if so set the r? flag? Thanks! These DirectoryProvider.js changes look reasonable, but can you summarize the change in behavior here to make sure I'm clear on what's going on? We actually have a separate dumbed-down Java implementation of the search service that we use in the search activity, and I'm worried we may need to change that as well. Although, looking at this code, it looks like we do already prefer plugins shipped with the browser over ones in the profile directory: http://mxr.mozilla.org/mozilla-central/source/mobile/android/search/java/org/mozilla/search/providers/SearchEngineManager.java#422

(In reply to :Margaret Leibovic from comment #14) Thanks! > These DirectoryProvider.js changes look reasonable, but can you summarize > the change in behavior here to make sure I'm clear on what's going on? The explanation is in comment 6; I should have pointed to it in my previous comment, sorry. > We actually have a separate dumbed-down Java implementation of the search > service that we use in the search activity, and I'm worried we may need to > change that as well. Interesting, I didn't know that at all. Some of the changes I'm preparing in bug 1175218 may need to be ported to the Java implementation. > Although, looking at this code, it looks like we do > already prefer plugins shipped with the browser over ones in the profile > directory: > http://mxr.mozilla.org/mozilla-central/source/mobile/android/search/java/org/ > mozilla/search/providers/SearchEngineManager.java#422 Indeed, it looks like that wasn't consistent with the search service's behavior... and now is! :)

(In reply to Florian Quèze [:florian] [:flo] from comment #15) > > We actually have a separate dumbed-down Java implementation of the search > > service that we use in the search activity, and I'm worried we may need to > > change that as well. > > Interesting, I didn't know that at all. Some of the changes I'm preparing in > bug 1175218 may need to be ported to the Java implementation. Thanks for the heads up. We've had problems with this Java implementation and the region-based logic in this past... we only use this Java implementation for the search activity, which does not have as much usage as the main search in the urlbar, but we hope to continue work on the search activity, so we should make sure we file bugs about this as necessary. This also remind me of bug 1131087, which we have neglected :(

Comment on attachment 8621153 [details] [diff] [review] Patch v2 Approval Request Comment [Feature/regressing bug #]: search hijacking. [User impact if declined]: engine files dropped in the user profile can override default engines that we ship. [Describe test coverage new/current, TreeHerder]: lots of xpcshell tests included in the patch, and the patch reached mozilla-central more than a week ago. [Risks and why]: ok for aurora. [String/UUID change made/needed]: none.

Comment on attachment 8621153 [details] [diff] [review] Patch v2 We want to improve the search, taking it.

Florian, is manual QA coverage needed for this fix? If so, could you please provide a few details on how to verify it? Thank you!

(In reply to Petruta Rasa [QA] [:petruta] from comment #20) > Florian, is manual QA coverage needed for this fix? If so, could you please > provide a few details on how to verify it? Thank you! We have some automated test coverage here, but given the importance of search, it may be worth doing some exploratory testing in addition to the automated tests. I listed above the things that we want to ensure and that the automated tests are expected to verify: (Florian Quèze [:florian] [:flo] from comment #6) > The new xpcshell tests ensure that: > - a search plugin in the searchplugins/ directory of the profile doesn't > override an engine loaded from jar. > - a search plugin loaded from an add-on in the profile doesn't override an > engine loaded from jar (not directly related to the change made by this > patch, but apparently we didn't have any coverage for loading engines from > add-ons, so I also added a test covering that). > - a search plugin loaded from a distribution DOES override an engine loaded > from jar.

Verified as fixed using Firefox 40 beta 2&3 and latest Aurora 42.0a2 2015-07-13 under Win 7 x64, Ubuntu 14.04 x86, Mac OS X 10.9.5. Logged follow-up bug 1183091.