340122 - Don't offer <link rel="search"> discovered engines if their title matches an existing search plugin

Status: RESOLVED FIXED

(Firefox :: Search, defect)

Description

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

Engines discovered with <link rel="search"> can have a title, and in most cases that title will match the engine's name. We should not offer those engines in the "discovered engines" part of the dropdown since they are likely already installed.

This doesn't solve cases where the <link>'s title doesn't match the engine name, but that's a lot harder to do (since we need to download the engine definition file to know), and I suspect that just doing this will fix the majority of cases.

Comment 1

[Pam Greene]

I'm a bit concerned about the opportunity this would open for a malicious engine A to effectively disable the discovery of engine B by giving its <link> the same title as B's uses.  (Of course, they'd be disabling their own discovery for people who had encountered engine B first, but if A were already in the list or available in other ways it might not be a zero-sum prospect.)

Comment 2

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

I'm not sure I understand. I'm proposing not showing "Add Technorati" if you already have an engine named "Technorati" installed. Malicious engine A would need to be installed for you not to see identically-named engine B when visiting B.com.

Comment 3

[Pam Greene]

I had a fancy example all written out, but on second thought I think it's okay.  There's some risk that an engine could give their search description a title that overlaps with another engine's <link title=...>, either maliciously or accidentally, but since they'd have to live with that as their name in the menu as well I suppose it'll be rare.

Comment 4

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

Pam, does that mean you'd like to wontfix/invalidate this bug? Note that my proposal in bug 339735 comment 7 might end up being a satisfactory workaround for this. BTW, what happens when we have two search engines with the same name? Can that happen?

Comment 5

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

(In reply to comment #4)
> BTW, what happens when we have two search engines with the same name? Can that
> happen?

No, it can't. At the moment, if you try to install an engine with the same display name as a currently installed engine, the install fails silently (1.5 behavior was to replace the existing engine without notice). If you copy the files manually into one of the search plugin directories, the first one that's loaded wins. This sucks, and I'll file a bug about it if I haven't already. It will probably depend on bug 335102.

Comment 6

[Pam Greene]

(In reply to comment #4)
> Pam, does that mean you'd like to wontfix/invalidate this bug?

No, I changed my mind.  I think the proposal is a good one.  Engines might end up overlapping, either deliberately or accidentally, but that won't be nearly as common as someone returning to a page when they've already added that engine, and bug 335102 or its offspring will ultimately take care of the problem.

Comment 7

[Mike Connor [:mconnor]]

pam, do you have cycles to take this?

Comment 8

[Pam Greene]

Attachment: Fixes problem

Also updates the menu and the button when the engine is added to the list.

Note that two places, marked in comments, will need to be updated when titles are no longer used as search-engine identifiers (bug 335102).

Comment 9

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

Comment on attachment 224810 [details] [diff] [review]
Fixes problem

>Index: base/content/browser.js

>+      var searchService = Components.classes["@mozilla.org/browser/search-service;1"]
>+                                    .getService(Components.interfaces.nsIBrowserSearchService);
>+      if (searchService &&
>+          searchService.getEngineByName(target.title)) {

Why null check searchService here? If this is null, we want to know with an exception (and probably wouldn't get here anyways)!

>Index: components/search/content/search.xml

>+              // Remove this engine from the list.  Since arrays have no
>+              // random-access deletion and the list is likely to be very short,
>+              // simply rebuild the array with the matching item omitted.
>+              // XXX This will need to be changed when engines are identified
>+              // by URL; see bug 335102.
>+              var newEngines = [];
>+              var removeTitle = aTarget.getAttribute("title");
>+              var browser = getBrowser().mCurrentBrowser;
>+              var browserEngines = browser.engines;
>+              for (var i = 0; i < browserEngines.length; i++) {
>+                if (browserEngines[i].title != removeTitle)
>+                  newEngines.push(browserEngines[i]);
>+              }
>+              browser.engines = newEngines;

I think this could be replaced by:
var removeTitle = aTarget.getAttribute("title");
var browser = getBrowser().mCurrentBrowser;
for (var i = 0; i < browser.engines.length; ++i){
  if (browser.engines[i].title == removeTitle) {
    browser.engines.splice(i, 1);
    break;
  }
}

Comment 10

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

I think you need to null check browser.engines, too, right? If onLinkAdded isn't called I think it's null.

Comment 11

[Pam Greene]

(In reply to comment #9)
> Why null check searchService here? If this is null, we want to know with an
> exception (and probably wouldn't get here anyways)!

From what I've seen, some users of a service null-check it after a getService() and some don't.  (For example, nsSidebar.js#178 does; browser.js#3022 doesn't.)  It's not obvious to me which is better, so I've been using the guideline that if the action is important, I'll leave out the null-check and let it throw, but if it doesn't matter much, I'll null-check and let it skip the action instead.  Is there a better convention to follow?

I suppose, on second examination, that adding the engine to the list is important.  (Removing it afterwards isn't, but that's beside the point.)  So I'll take out the test.

>   if (browser.engines[i].title == removeTitle) {
>     browser.engines.splice(i, 1);
>     break;
>   }

Ah, thanks.  I learn more Javascript every day.

(In reply to comment #10)
> I think you need to null check browser.engines, too, right? If onLinkAdded
> isn't called I think it's null.

The user just chose one of the engines to add to the list, so if browser.engines is null, something's wrong.  Am I missing a situation where that's not true?  I'll add an explanatory comment in any case.

Comment 12

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

(In reply to comment #11)
> (In reply to comment #9)
> > Why null check searchService here? If this is null, we want to know with an
> > exception (and probably wouldn't get here anyways)!
> 
> From what I've seen, some users of a service null-check it after a
> getService() and some don't.

I've been told getService in JS will either throw or succeed, so null checking it's return value serves no purpose. I think the places that do this were written by people who were expecting it to behave like do_GetService in C++. The case in nsSidebar was me copying some bad code :(. Could you also fix the other instance in onEnginePopupCommand?

> The user just chose one of the engines to add to the list, so if
> browser.engines is null, something's wrong.  Am I missing a situation where
> that's not true?  I'll add an explanatory comment in any case.

Ah, indeed! I guess we can assume that it's not possible to have that run without closing the menu, which in turns rebuilds it when it reopens.

Comment 13

[Mike Connor [:mconnor]]

Comment on attachment 224810 [details] [diff] [review]
Fixes problem

I'll review this with gavin's comments addressed

Comment 14

[Pam Greene]

Attachment: Patch addressing Gavin's comments

Comment 15

[Mike Connor [:mconnor]]

Comment on attachment 224907 [details] [diff] [review]
Patch addressing Gavin's comments


>       <method name="init">
>         <body><![CDATA[
>           this._popup = document.getAnonymousElementByAttribute(this, "anonid",
>                                                             "searchbar-popup");
>+          this._button = document.getAnonymousElementByAttribute(this, "anonid",
>+                                                            "search-go-button");

So, this doesn't make sense to me, why not just stick these in the <field/> like for _stringBundle and _textbox?

Ironically, the <field/> defs blow 80 chars away, so weird wrapping here is somewhat pointless.

>           if (aTarget.getAttribute("class").indexOf("addengine-item") != -1) {
>             var searchService = Components
>                       .classes["@mozilla.org/browser/search-service;1"]
>                       .getService(Components.interfaces.nsIBrowserSearchService);

Hmm, zany bogus indentation.  The following is preferred where needed (I know this is existing, but it should get fixed while we're in the neighbourhood.

             var searchService = 
               Components.classes["@mozilla.org/browser/search-service;1"]
                         .getService(Components.interfaces.nsIBrowserSearchService);

r+a=me with style nit + the <field> comment explained or fixed.

Comment 16

[Pam Greene]

(In reply to comment #15)
> >       <method name="init">
> >         <body><![CDATA[
> >           this._popup = document.getAnonymousElementByAttribute(this, "anonid",
> >                                                             "searchbar-popup");
> >+          this._button = document.getAnonymousElementByAttribute(this, "anonid",
> >+                                                            "search-go-button");
> 
> So, this doesn't make sense to me, why not just stick these in the <field/>
> like for _stringBundle and _textbox?

Good question.  Gavin's patch for bug 232272 (-> r1.37.2.11) moved it, and I was following the pattern.  I'll check with him and either add a comment explaining why this is needed or move them both to <field/> initialization.

>              var searchService = 
>                Components.classes["@mozilla.org/browser/search-service;1"]
>                         
> .getService(Components.interfaces.nsIBrowserSearchService);

Done.

Comment 17

[Pam Greene]

Attachment: Patch as checked in

Indentation fixed; _popup and _button initialization moved to <field> declarations.

Comment 18

[Pam Greene]

fixed-1.8-branch, fixed-on-trunk

Comment 19

[Ria Klaassen (not reading all bugmail)]

Is it a known bug (can't find it) that alt + down arrow does not call the engines list anymore? Since about 11 June. Or has the key changed?

Comment 20

[Ria Klaassen (not reading all bugmail)]

Between 1.9a1_2006061214 and 1.9a1_2006061216 so it could point to this bug.

Comment 21

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

(In reply to comment #19)
> Is it a known bug (can't find it) that alt + down arrow does not call the
> engines list anymore? Since about 11 June. Or has the key changed?

Nope, you're right, the patch here did cause that bug. I filed bug 342645 for that.