306576 - Search plugins URLs should be standardized on *secure* mozilla.org servers

Status: RESOLVED FIXED

(Firefox :: Search, defect, P1)

Description

[Benjamin Smedberg]

Search plugin URLs, especially the localized search plugin URLs, are set to all
kinds of bad things. I would rather we just didn't have any search plugin
updates for our default searchplugins, but if we're going to have them, they
should be a standardized URL on a secure mozilla.org server (not www.mo).

Mconnor and I already discussed this, and for this cycle the URL should be:

https://addons.mozilla.org/searchplugins/updates/<searchplugin>.src

In addition, all the localized searchplugins should have unique names, i.e.
instead of google-france searchplugin being named "google.src" it should be
"google-fr.src".

Comment 1

[O. Atsushi (Torisugari)]

When this bug is fixed, bug 284456 will disappear
without any client-side hack.

Comment 2

[Jeferson Hultmann]

(In reply to comment #0)
> In addition, all the localized searchplugins should have unique names, i.e.
> instead of google-france searchplugin being named "google.src" it should be
> "google-fr.src".


When Firefox 1.0 users install version 1.5, wouldn't they have duplicated search
engines?

e.g. "google.src" (from 1.0) + "google-fr.src" (from 1.5).

Comment 3

[Zibi Braniecki [:zbraniecki][:gandalf]]

Won't it give us a list of shortcommings like 
1) When mozilla.org is down, our users can't use searchengines
2) When mozilla.org is slow, searchplugins works slow
etc.?

I'm not sure what bad things did you mean? Maybe there's another way to fix them
without subordinating searchplugins to mozilla.org servers.

Comment 4

[Pavel Franc - Mozilla.cz]

I don't like this idea at all. Although you prevent bad l10n teams to set their
 localized search plugin URLs to all kinds of bad things, you also prevent the
updating mechanism to work.

By now the search plugins of Czech Firefox are hosted directly on the search
engine provider (i.e. Seznam.cz plugin is on Seznam.cz server). So if the search
engine is changed, the administrator can easily change the search plugin, and
these changes are automaticaly propagated to users. If all search plugins are on
a.m.o there will be no automatical update. The plugin will just stop working,
users will complain, we (l10n team) have to download the new plugin from the
server, fill a bug, nag on IRC to get someone with enouch rights to fix it ...

So please, if we're going to have the search plugin updates, allow them to be
also hosted on the server of origin.

Comment 5

[Axel Hecht]

"Bad things" include most google plugins referring to the english version (which
is a 404, so nothing bad happens).

On top of that, the search plugin is part of a Mozilla product, and changing that
on the descretion of another entitity may sound easy, but from a product 
development point, this is a daunting thing. We did have problems with search
engines before, and it's good to control the updates.

Note that this is only about the update URLs, the functionality of the plugins
is not affected by mozilla.org server uptimes.

Comment 6

[Kadir Topal [:atopal]]

The idea sounds good, but I would rather use "google.de" and not "google-de". It
looks a lot more familiar for users.

Comment 7

[Mike Connor [:mconnor]]

I don't expect end-users will be seeing google-de.src vs google.de.src and
getting confused, especially since this is just filenames.

Sure, its slightly less convenient for individuals to have to go through an
approvals process to get these uploaded.  That said, we want to ensure that we
control this aspect of our product for official builds, for the same reason that
we're providing official builds ourselves.  I seriously doubt that searchplugin
updates are occuring so frequently that this is going to represent a major
hardship for server operators.  Or that things will change in a timeframe such
that it is not possible to get new searchplugins to the server before the system
change.

As for getting new versions uploaded, I expect that this would be handled via
bugzilla as a mozilla.org Server Ops change request, with the new searchplugin
attached for transparency purposes.  Not a lot of work to file a bug and attach
a file.

Comment 8

[Kadir Topal [:atopal]]

(In reply to comment #7)
> I don't expect end-users will be seeing google-de.src vs google.de.src and
> getting confused, especially since this is just filenames.

Sorry my bad, but what about comment #2? Having double entries for updaters
would be pretty bad.

Comment 9

[Mike Connor [:mconnor]]

Yes, this is a problem since we can't do clean updates automagically because of
people installing into weird places.

I could be ok with punting on that naming convention for now since we want to
make search "better" in 2.0, unless someone has a better solution.

Comment 10

[Grey Nicholson (they/them)]

Now that search plugins can be installed by extensions, I suggest:

The update declarations are removed from the standard search plugins and each of
them is packaged into an extension, which is hosted by mozilla.org (presumably
on addons.m.o). The default search plugins are provided as default extensions to
Firefox. Any updates are handled by the extension update system.

This would give the ability to uninstall search plugins and the ability to
disable their installation (bug 289826) for free. It would also ensure that the
old src file is removed during an update before the new one is added, and that
the icon is always available and updated properly (because it would be inside
the XPInstaller).

Comment 11

[Axel Hecht [:Pike]]

Requesting beta 2 blocking, this has quite some fallout for l10n.

Comment 12

[Benjamin Smedberg]

Pike, I'm on vacation next week, are you interested in taking/driving this?

Comment 13

[Axel Hecht [:Pike]]

taking over for now.

Comment 14

[Axel Hecht [:Pike]]

Could we get a sign-off of the URL scheme proposed by Benjamin in the initial 
comment from UMO server folks?

I'd like to improve this situation in our 39+ locales soon.

I'm following mconnor in that these filenames are not user-visible. I don't see
a good way around the duplicate entries for users updating without not fixing
bugs. Having that work as it should is subject of bug 232272 or bug 235852, which
according to gavin is due to 2.0.

Comment 15

[Kadir Topal [:atopal]]

Really, giving users double entries in their search plugin list, without a user
iterface to delete them (which, by the, way is one of the most stupid things
I've ever seen) should be a no-go.

Comment 16

[Jeferson Hultmann]

Is there a reason why the URL should not be:

https://addons.mozilla.org/searchplugins/updates/ab-CD/<searchplugin>.src ?
-------------------------------------------------^^^^^

no need to rename files, no duplicate entries.

Comment 17

[Axel Hecht [:Pike]]

yes, having wikipedia.src as file name for all wikipedia plugins makes it
impossible to have more than one wikipedia plugin installed.
That's why this naming scheme is used on mycroft as well.

Comment 18

[Benjamin Smedberg]

Attachment: Blow away old searchplugins, rev. 1 [checked in on trunk and 1.8 branch]

Comment 19

[Axel Hecht [:Pike]]

Ok, some explanation on how we resolve this bug in total:
This bug is blocking 1.5, but not 1.5beta2, as we decided to blow away the 
existing search engine plugins in the installer. This comes with several benefits
for most of our users:
- Starting with 1.5, newly added search plugins will be added to the profile,
not the installation directory, so for those few that had custom search engine
plugins installed, they found the right time to update to the new scheme.
- Our release notes always recommended blowing away the previous install 
completely, so following our recommended install path, it doesn't change anything.
- Even with the rename of search engine plugins, we won't end up with doubled
entries.

This is profile migration for a new feature, to some extent, and yes, it 
involves user action, but that's the way it is. We will add a note to the 
release notes for this.

Localizers, please adjust your search engine plugins for this. And please
cvs remove search engine plugins that are not in list.txt, too.

Chris, Rafael, Benjamin, we should probably use unique names for the 
search engine plugins, too, right? Like Google.com, Google.de etc?

Comment 20

[Axel Hecht [:Pike]]

Forgot to add, the URL scheme

https://addons.mozilla.org/searchplugins/updates/plugin[-ab-CD].src

got signed off by Mike Morgan and Chris Beard and is the one to be used.

Comment 21

[Axel Hecht [:Pike]]

Attachment: move the update URL over to https://addons

Chris, Mike, these are the URLs.

Are you OK with the update interval? It's daily for google, every 3 days for 
the others. I'd like to get that signed off both from the trademarks and the
server side.

Comment 22

[Mike Connor [:mconnor]]

Comment on attachment 198217 [details] [diff] [review]
move the update URL over to https://addons

why is the google interval shorter? I don't think its going to be updated any
more often than the others.  Other than that, looks ok.

Comment 23

[Axel Hecht [:Pike]]

A further note to localizers, there is no problem with having 404s on addons 
unless we actually want to update a particular search engine plugin.

Comment 24

[O. Atsushi (Torisugari)]

(In reply to comment #23)
> no problem with having 404s

Except hitting www.mozilla.org every time they use a search plugin
instead of every 3 days.

Comment 25

[Axel Hecht [:Pike]]

(In reply to comment #24)

> Except hitting www.mozilla.org every time they use a search plugin
> instead of every 3 days.

Could you give us an lxr link for that? I'd really like to know which code is
responsible for that.

Comment 26

[O. Atsushi (Torisugari)]

(In reply to comment #25)
> Could you give us an lxr link for that? I'd really like to know which code is
> responsible for that.

http://lxr.mozilla.org/mozilla/source/xpfe/components/search/src/nsInternetSearchService.cpp#5202
>5202     if (httpStatus != 200)  return(NS_ERROR_UNEXPECTED);

That line means we are going to exit from OnStopRequest(...) *before*
calling validateEngineNow(). Then localstore.rdf will never remember
Last-Update-Check-Date as long as the server returns 404. When
localstore.rdf doesn't know that date, updateCheckDays=3 is of no use.

Comment 27

[Mike Connor [:mconnor]]

Is this a known bug?  Because that seems like fairly broken behaviour to me. 
Can we just call validateEngineNow() on a 404?  I don't think that'd be a
problem, and the patch would be trivial.

Comment 28

[O. Atsushi (Torisugari)]

(In reply to comment #27)
> Is this a known bug?  Because that seems like fairly broken behaviour to me. 
> Can we just call validateEngineNow() on a 404?  I don't think that'd be a
> problem, and the patch would be trivial.

No, it's not filed yet. It was a bit difficult to find this issue until
bug 290254 fixed, but this behavior itself exists since a long long time ago.

And it's resonable in a sense, as we failed that check anyway. I'm not too
sure we should write down Last-Check-Date even on an invalid update check.
Though I agree this behavior sucks in total.

And yes, the patch should be trivial.

Comment 29

[Darin Fisher]

Regarding rev. 1 patch, you guys realize that software update does not know how
to remove directories, right?  You're okay with the searchplugins directory
being removed at install time only, right?

Comment 30

[Benjamin Smedberg]

Yes, at the current point in time updates within the 1.5 timeframe are not a big
problem.

Comment 31

[O. Atsushi (Torisugari)]

If each localized build would have completely localized search plugins,
we don't need searchconfig.properties, witch adds extra options to the
querry URLs, any longer?

http://lxr.mozilla.org/mozilla/source/other-licenses/branding/firefox/content/searchconfig.properties
http://lxr.mozilla.org/mozilla/source/browser/base/branding/searchconfig.properties

As for what "client=firefox" is doing, please take a look at bug 310075.

Comment 32

[Axel Hecht [:Pike]]

Comment 31 is unrelated, the stuff in other-licenses is merely to ease the 
trademarks review process.

Comment 33

[Axel Hecht [:Pike]]

Due to me being at EuroOSCON (and offline there), this is really late.
But we need this fix to do trademarks reviews.

Comment 34

[Axel Hecht [:Pike]]

landed on the branch. I hope we're OK with the daily update of the google plugin.

Comment 35

[Axel Hecht [:Pike]]

Attachment: eBay.src should update from eBay.src instead of ebay.src

This is a trivial patch which I'd like to get into RC1 still. We should really
use case sensitive filenames for update. Zilch risk.

Comment 36

[Mike Connor [:mconnor]]

Comment on attachment 200721 [details] [diff] [review]
eBay.src should update from eBay.src instead of ebay.src

oy, good catch

Comment 37

[Axel Hecht [:Pike]]

checked in attachement 200721 on the 1.8 branch.

Comment 38

[Daniel C]

Did attachment 198217 [details] [diff] [review] and attachment 200721 [details] [diff] [review] land on the trunk? Comment 34 and comment 37 don't mention it, nor do the attachment names. If so, can this bug be resolved?

Comment 39

[Axel Hecht [:Pike]]

Attachment: updated patch to the trunk

attachement 198217 and 200721 merged together for check in on the trunk.

Comment 40

[Axel Hecht [:Pike]]

landed on the trunk

Comment 41

[Axel Hecht [:Pike]]

(In reply to comment #29)
> Regarding rev. 1 patch, you guys realize that software update does not know how
> to remove directories, right?  You're okay with the searchplugins directory
> being removed at install time only, right?

The good news is, at least when doing an incremental update, the patching
routine removes the old plugins and adds the new ones. So folks updating
from b2 to rc1 won't end up with duplicate entries :-), other folks will likely
start with an installer build of 1.5 and be good this way around.

Comment 42

[Ryan Flint [:rflint] (ping via IRC for reviews)]

*** Bug 318196 has been marked as a duplicate of this bug. ***

Comment 43

[Jon Smith]

(In reply to comment #42)
> *** Bug 318196 has been marked as a duplicate of this bug. ***
> 

The old user installed plugins should be moved to their user profile instead of being deleted altogether with the new upgrade.