Closed Bug 1109354 Opened 9 years ago Closed 9 years ago

prefer Firefox default engines over profile-installed plugins with the same name

Categories

(Firefox :: Search, defect, P3)

defect
Points:
8

Tracking

()

VERIFIED FIXED
Firefox 41
Tracking Status
firefox40 --- verified
firefox41 --- verified

People

(Reporter: ehsan.akhgari, Assigned: florian)

References

Details

(Whiteboard: [hijacking][fxsearch])

Attachments

(1 file, 1 obsolete file)

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?
Flags: needinfo?(florian)
I don't really know this part of the code, moving the ni to Gavin who likely knows.
Flags: needinfo?(florian) → needinfo?(gavin.sharp)
(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.
Flags: needinfo?(gavin.sharp)
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.
Summary: My yahoo search engine was replaced by malware → prefer Firefox default engines over profile-installed plugins with the same name
Points: --- → 8
Flags: qe-verify+
Flags: firefox-backlog+
OS: Mac OS X → All
Hardware: x86 → All
Priority: -- → P3
Whiteboard: [fxsearch][searchhijacking]
Rank: 35
Whiteboard: [fxsearch][searchhijacking] → [hijacking][fxsearch]
Attached patch Patch (obsolete) — Splinter Review
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.
Assignee: nobody → florian
Status: NEW → ASSIGNED
Attachment #8612365 - Flags: feedback?(mconnor)
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.
Attachment #8612365 - Flags: feedback?(mconnor) → feedback+
Attached patch Patch v2Splinter Review
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.
Attachment #8612365 - Attachment is obsolete: true
Flags: needinfo?(mconnor)
Attachment #8621153 - Flags: review?(markh)
(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.
Attachment #8621153 - Flags: review?(markh) → review+
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. ;)
Flags: needinfo?(mconnor)
Attachment #8621153 - Flags: feedback+
(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.
https://hg.mozilla.org/mozilla-central/rev/3dff06c8adbc
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 41
(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!
Flags: needinfo?(margaret.leibovic)
(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
Flags: needinfo?(margaret.leibovic)
(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 :(
Depends on: 1176217
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.
Attachment #8621153 - Flags: approval-mozilla-aurora?
Comment on attachment 8621153 [details] [diff] [review]
Patch v2

We want to improve the search, taking it.
Attachment #8621153 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
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!
Flags: needinfo?(florian)
(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.
Flags: needinfo?(florian)
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.
No longer blocks: 1183091
Status: RESOLVED → VERIFIED
Depends on: 1176216
You need to log in before you can comment on or make changes to this bug.