340331 - Allow installation of multiple search plugins with the same name

Status: RESOLVED WONTFIX

(Firefox :: Search, defect)

Description

[:Gavin Sharp [email: [e-mail]]]

Currently, if you try to install a search plugin with the same name as an existing plugin, the installation fails silently. This is also problematic for people who "delete" a shipped engine and then try to install a new one with the same name, since the deletion of a shipped engine really only hides it.

This will probably be fixed by a solution to bug 335102.

Comment 1

[Joey Minta]

Attachment: use _location instead

This patch moves us to using _location as the unique identifier for a search engine.  We strip out non-alphanumeric characters, making it safe to use as a preference.  Since the _location for the default engines will vary by installation, we also use some migration code to preserve the default ordering for engines for new profiles.  This assumes that we don't ship by default 2 engines with the same name, but should allow for multiple engines with the same name to later be installed.

Comment 2

[Joey Minta]

Flipping the dependency here.  You can't rename engines until we stop caring about the name, which this bug does.

Comment 3

[Joey Minta]

Comment on attachment 224909 [details] [diff] [review]
use _location instead

Cancelling review request, I'd rather land the mozStorage bits first, and fix this right the first time.

Comment 4

[Joey Minta]

Attachment: move to ids

This patch moves to using ids in our internal store.  As such, we replace getEngineByName with getEngineById, and add some additional migration code, since several other prefs relied on names.  We now also prompt the user if they already have a search engine with a particular name installed, so I'm asking for UI review on the strings.

Comment 5

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 226177 [details] [diff] [review]
move to ids

>+  get id() {
>+    // Make sure we strip out any ugly characters from the file path, since
>+    // we'll use this for prefs and such
>+    return sanitizeName(this._id);

Doesn't passing the filename through sanitizeName means that two engines with names that differ only by characters that sanitizeName strips out will have the same ID?

>+      if (!installAnyway) {
>+        engineMetadataService.setAttr(aEngine, "hidden", true);
>+        return;
>+      }

Why bother hiding it if we're not installing it?

>+    this._sortedEngines = this._sortedEngines.filter(function(a) { return a; });

Can you add a comment for why this is needed?

>+    // Normally it's not safe to search for engines by name, since a user may
>+    // have installed 2 engines with the same name.  With a clean profile,
>+    // however, we know this is safe.

What if someone deletes their storage.sdb file? Or if a distributor wants to ship a Firefox install with multiple identically named plugins? I guess those are edge cases.

>+    // Migrate the defaultenginename and currentenginename to ids as well.
>+    // At first startup we know that engines have unique names, but this might
>+    // be changed by the user.
>+    const defPref = BROWSER_SEARCH_PREF + "defaultenginename";
>+    const curPref = BROWSER_SEARCH_PREF + "selectedEngine";
>+    var defaultEngineName = getLocalizedPref(defPref, "");
>+    var currentEngineName = getLocalizedPref(curPref, "");

Shouldn't we just change the default prefs instead of having this code convert them at the first startup? That will be possible once bug 341976 lands.

I'm starting to think that allowing multiple engines with the same name is too much trouble. We can support search engine renaming while keeping unique names (keying by ID, and having the name be an attribute). People trying to install an engine that has the same name as an already-installed (and possibly hidden) engine is probably going to be pretty uncommon, and that situation might be better solved in another way (e.g. by prompting for another name). That would allow us to keep "getEngineByName" (which I think is a much more useful external API than getEngineByID) and would reduce the need for some of the assumptions this patch adds to _convertOldPrefs.

Most of this patch lays the groundwork for bug 335102, so it's relevant either way, but I'm starting think that this bug as summarized should be WONTFIX. What do you think?

Comment 6

[Régis Caspar]

(In reply to comment #5)
> I'm starting to think that allowing multiple engines with the same name is too
> much trouble. We can support search engine renaming while keeping unique names
> (keying by ID, and having the name be an attribute). People trying to install
> an engine that has the same name as an already-installed (and possibly hidden)
> engine is probably going to be pretty uncommon, and that situation might be
> better solved in another way (e.g. by prompting for another name). That would
> allow us to keep "getEngineByName" (which I think is a much more useful
> external API than getEngineByID) and would reduce the need for some of the
> assumptions this patch adds to _convertOldPrefs.
FWIW, I think you're right, in the patch I made (see bug 335102#c4), I just made the search engine manager ignore the renaming when no name or the name of an existing engine is given (that's why it wrks with name keying). 
Just for my information, is there any metadata other than "hidden", "alias", "name" and order that could exists? 

Comment 7

[Joey Minta]

(In reply to comment #5)
> Doesn't passing the filename through sanitizeName means that two engines with
> names that differ only by characters that sanitizeName strips out will have the
> same ID?
Yes.  I wanted something that would be absolutely safe for preferences.

> 
> Why bother hiding it if we're not installing it?
For some reason these were ending up in the store on restart.  It seemed that at this point the engine was already saved (I think it already had a file), so we loaded it the next time around on init().  Marking it hidden ensured that that didn't happen.

> What if someone deletes their storage.sdb file? Or if a distributor wants to
> ship a Firefox install with multiple identically named plugins? I guess those
> are edge cases.
:-( Not sure the best way to handle this yet.  I might be ok on punting this question.

> Shouldn't we just change the default prefs instead of having this code convert
> them at the first startup? That will be possible once bug 341976 lands.
Once bug 341976 lands this is certainly possible and probably the right thing to do.  It does make it a bit more difficult for localizers though.

> 
> Most of this patch lays the groundwork for bug 335102, so it's relevant either
> way, but I'm starting think that this bug as summarized should be WONTFIX. What
> do you think?
> 
I can buy that, although we're still going to need to do special casing to avoid the problem of re-installing a hidden engine.  Or can we avoid that by fixing bug 341833?  If so, then sure, we can mark this WONTFIX.

Comment 8

[Joey Minta]

As I understand it, we're WONTFIXing this bug, in favor of allowing renaming but forcing search engines to have unique names when this happens.  pamg, are you ok with that?  Gavin also suggested keeping ._originalName around, which might be helpful for your api.

Comment 9

[Pam Greene]

(In reply to comment #8)
> As I understand it, we're WONTFIXing this bug, in favor of allowing renaming
> but forcing search engines to have unique names when this happens.  pamg, are
> you ok with that?  Gavin also suggested keeping ._originalName around, which
> might be helpful for your api.

Meaning that engines will still be identified by names rather than URLs?  That will be fine for all existing functionality, but it'll prevent bug 340604 (which, however, isn't very high priority).

Comment 10

[:Gavin Sharp [email: [e-mail]]]

(In reply to comment #9)
> Meaning that engines will still be identified by names rather than URLs?  That
> will be fine for all existing functionality, but it'll prevent bug 340604
> (which, however, isn't very high priority).

Engines are now uniquely identified by relative path on the hard drive, and not URLs. I'm not sure how what we use as the unique identifier for an engine matters for that bug. Either way, we can keep the URI from which the engine was installed as an attribute on the engine object.

Comment 11

[Pam Greene]

(In reply to comment #10)
I'm not sure how what we use as the unique identifier for an engine
> matters for that bug. Either way, we can keep the URI from which the engine was
> installed as an attribute on the engine object.

Sure, as long as there's a method that takes a URI and returns whether that engine is already known, that's entirely sufficient.

Comment 12

[Joey Minta]

Attachment: move to ids v2

This patch addresses the review comments from the previous patch, as well as fixing some js-assertions when installing a new engine by switching away from .name and using ._originalName.

Comment 13

[Joey Minta]

Comment on attachment 226661 [details] [diff] [review]
move to ids v2

wrong bug!