344523 - AddSearchProvider doesn't check if provider already installed

Status: VERIFIED FIXED

(Firefox :: Search, defect)

Description

[John P Baker]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.8.1b1) Gecko/20060710 Firefox/2.0b1
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.8.1b1) Gecko/20060710 Firefox/2.0b1

The Javascript window.external.AddSearchProvider doesn't check if
the provider is already installed.

Reproducible: Always

Steps to Reproduce:
1. Go to http://www.bristol.ac.uk/search/
2. click on "Add this Provider to Search Box"
3. click again on "Add this Provider to Search Box"

Actual Results:  
Get second prompt "Add a new engine ..."
If confirm adds a second file to searchplugins folder

Expected Results:  
Information that search engine already installed.

cf IE7 beta 3

Comment 1

[John P Baker]

Something that came to me while composing this bug, and have just tried...

In addition if you remove one of these multiple engines, and then restart
Firefox the 'removed' entry will appear to reappear [actually one of the
other copies will take its place]

Comment 2

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

(In reply to comment #1)
> In addition if you remove one of these multiple engines, and then restart
> Firefox the 'removed' entry will appear to reappear [actually one of the
> other copies will take its place]

Yes, that's because the engine is there, it just won't load because you can't load multiple engines with the same name.

How does IE determine whether an engine is already installed? Does it keep track of which site you added the engine from? Or does it just rely on the name?

Comment 3

[John P Baker]

(In reply to comment #2)
> How does IE determine whether an engine is already installed? Does it keep
> track of which site you added the engine from? Or does it just rely on the
> name?

It seems to use the ShortName - which is also the one that appears in the
AddSearchProvider dialogue box (Firefox doesn't include this).

This makes some sort of sense as it is the ShortName that stops the engine
appearing, on the drop down, more than once.

Comment 4

[John P Baker]

(In reply to comment #3)

> It seems to use the ShortName

Cancel this. It, at least, uses both the ShortName and the URL as it will
give the option to replace the old one if the URL changes - or even add
a second provider with the same name (it qualifies the second one with the
site that it was installed from eg |(www.bris.ac.uk) Bristol Uni.|

Comment 5

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

(In reply to comment #3)
> It seems to use the ShortName - which is also the one that appears in the
> AddSearchProvider dialogue box (Firefox doesn't include this).

It does now that bug 341668 is fixed (for beta 2) :)

(In reply to comment #4)
> Cancel this. It, at least, uses both the ShortName and the URL as it will
> give the option to replace the old one if the URL changes - or even add
> a second provider with the same name (it qualifies the second one with the
> site that it was installed from eg |(www.bris.ac.uk) Bristol Uni.|

Thanks for looking into this. We should definitely throw an error, at the very least, instead of silently failing.

Comment 6

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

Attachment: throw an error if the engine is already installed

This just ensures that we don't silently fail, and that we don't leave engines that won't load in the profile. We should definitely improve this dialog to do smart replacing and allow renaming (once the renaming patch lands), but I don't think that's something that can be done in the Firefox 2 timeframe (it would involve a lot of string additions, at the very least).

Comment 7

[Mike Connor [:mconnor]]

Comment on attachment 229674 [details] [diff] [review]
throw an error if the engine is already installed

nit:

if (foo)
  bar;

should have a newline after it

Comment 8

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

mozilla/browser/locales/en-US/chrome/browser/search.properties 	1.14
mozilla/browser/components/search/nsSearchService.js 	1.61

Comment 9

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

Comment on attachment 229674 [details] [diff] [review]
throw an error if the engine is already installed

This is another "don't fail silently and leave junk files in the user's profile dir" bug, and carries a low risk of regression. It adds one localizable string.

Comment 10

[Mike Beltzner [:beltzner, not reading bugmail]]

Comment on attachment 229674 [details] [diff] [review]
throw an error if the engine is already installed

a=drivers. Please land on the MOZILLA_1_8_BRANCH.

Comment 11

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

mozilla/browser/components/search/nsSearchService.js 	1.1.2.49
mozilla/browser/locales/en-US/chrome/browser/search.properties 	1.1.2.11