Last Comment Bug 1109354 - prefer Firefox default engines over profile-installed plugins with the same name
: prefer Firefox default engines over profile-installed plugins with the same name
Status: VERIFIED FIXED
[hijacking][fxsearch]
:
Product: Firefox
Classification: Client Software
Component: Search (show other bugs)
: unspecified
: All All
P3 normal 35 (vote)
: Firefox 41
Assigned To: Florian Quèze [:florian] [:flo] (PTO until February 27)
:
: Florian Quèze [:florian] [:flo] (PTO until February 27)
Mentors:
Depends on: 1176216 1176217
Blocks: 1183091
  Show dependency treegraph
 
Reported: 2014-12-09 15:35 PST by :Ehsan Akhgari
Modified: 2015-08-05 15:56 PDT (History)
9 users (show)
gavin.sharp: firefox‑backlog+
ryanvm: in‑testsuite+
gavin.sharp: qe‑verify+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: 8
Has Regression Range: ---
Has STR: ---
verified
verified


Attachments
Patch (15.13 KB, patch)
2015-05-28 09:37 PDT, Florian Quèze [:florian] [:flo] (PTO until February 27)
mconnor: feedback+
Details | Diff | Splinter Review
Patch v2 (31.89 KB, patch)
2015-06-11 12:04 PDT, Florian Quèze [:florian] [:flo] (PTO until February 27)
markh: review+
mconnor: feedback+
sledru: approval‑mozilla‑aurora+
Details | Diff | Splinter Review

Description User image :Ehsan Akhgari 2014-12-09 15:35:58 PST
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?
Comment 1 User image Florian Quèze [:florian] [:flo] (PTO until February 27) 2014-12-09 23:36:08 PST
I don't really know this part of the code, moving the ni to Gavin who likely knows.
Comment 2 User image :Gavin Sharp [email: gavin@gavinsharp.com] 2014-12-12 17:13:20 PST
(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.
Comment 3 User image :Gavin Sharp [email: gavin@gavinsharp.com] 2014-12-12 17:15:00 PST
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.
Comment 4 User image Florian Quèze [:florian] [:flo] (PTO until February 27) 2015-05-28 09:37:44 PDT
Created attachment 8612365 [details] [diff] [review]
Patch

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 5 User image Mike Connor [:mconnor] 2015-06-05 09:16:10 PDT
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.
Comment 6 User image Florian Quèze [:florian] [:flo] (PTO until February 27) 2015-06-11 12:04:37 PDT
Created attachment 8621153 [details] [diff] [review]
Patch v2

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.
Comment 7 User image Florian Quèze [:florian] [:flo] (PTO until February 27) 2015-06-12 08:09:30 PDT
(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 8 User image Mark Hammond [:markh] 2015-06-14 18:48:02 PDT
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 9 User image Mike Connor [:mconnor] 2015-06-15 08:17:46 PDT
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. ;)
Comment 11 User image Florian Quèze [:florian] [:flo] (PTO until February 27) 2015-06-15 09:36:06 PDT
(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.
Comment 12 User image Wes Kocher (:KWierso) 2015-06-15 18:25:52 PDT
https://hg.mozilla.org/mozilla-central/rev/3dff06c8adbc
Comment 13 User image Florian Quèze [:florian] [:flo] (PTO until February 27) 2015-06-16 07:36:26 PDT
(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!
Comment 14 User image :Margaret Leibovic 2015-06-16 14:40:00 PDT
(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
Comment 15 User image Florian Quèze [:florian] [:flo] (PTO until February 27) 2015-06-16 14:47:24 PDT
(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! :)
Comment 16 User image :Margaret Leibovic 2015-06-18 08:55:21 PDT
(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 17 User image Florian Quèze [:florian] [:flo] (PTO until February 27) 2015-06-24 14:16:13 PDT
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 18 User image Sylvestre Ledru [:sylvestre] 2015-06-26 10:59:49 PDT
Comment on attachment 8621153 [details] [diff] [review]
Patch v2

We want to improve the search, taking it.
Comment 19 User image Ryan VanderMeulen [:RyanVM] 2015-06-26 11:28:50 PDT
https://hg.mozilla.org/releases/mozilla-aurora/rev/52f8ab7477a5
Comment 20 User image Petruta Rasa [QA] [:petruta] 2015-07-03 05:34:30 PDT
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!
Comment 21 User image Florian Quèze [:florian] [:flo] (PTO until February 27) 2015-07-03 06:07:55 PDT
(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.
Comment 22 User image Petruta Rasa [QA] [:petruta] 2015-07-13 06:42:11 PDT
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.

Note You need to log in before you can comment on or make changes to this bug.