327932 - (search-update) Implement search engine update system

Status: RESOLVED FIXED

(Firefox :: Search, defect)

Description

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

To go along with the new search service, a plugin updating system needs to be implemented. I'm going to be collecting requirements on what this system needs to do and plans on how it should be implemented. It will probably behave in mostly the same way as the current update system.

Comment 1

[Axel Hecht [:Pike]]

Adding that comment here, scenario:
A user adds a sherlock plugin, and that has an update. How can we make the updater
pick that up?

Comment 2

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

(In reply to comment #1)
> Adding that comment here, scenario:
> A user adds a sherlock plugin, and that has an update. How can we make the
> updater pick that up?

I planned on supporting updating for sherlock files too, using the same <browser update=""> tag that is currently used.

Comment 3

[Mike Connor [:mconnor]]

+ for evaluation, gavin to post followup to d-a-f about whether this really matters in the new world.

Comment 4

[Mycroft Project (CC)]

Yes it matters (though I admit to not really understanding your new world reference).

Unless you have some miraculous way of ensuring web developers never change from a php to asp search page or never change the parameters for that search. While Google et al. big names may be reasonably constant, this is not true for the majority of smaller sites. I would try and give you some numbers for search plugins on Mycroft that have been updated in the last 6 months but it would take me longer than is worth it. Suffice to say that if users are forced to redownload the plugin because the current one doesn't work it's going to drive them mad I should think... (And you can't manually update a plugin at the moment anyway I don't think - unless this has been recently changed)

Comment 5

[Joey Minta]

There's a lot of metadata here that we're going to need to track.  I'd much rather be doing that in mozStorage.  This bug depends on bug 335101, but I'd rather not try to untangle the dependency trees here so that bugzilla will let me add that.

Comment 6

[Joey Minta]

I think I'm going to need to remove and then add the search engine when an update happens and this engine will have the same name, so this depends on bug 340331.  (Again, Bugzilla won't let me mark this.)

Comment 7

[Joey Minta]

Attachment: paranoid work in progress

I almost lost my work on this bug once, so I'm throwing this here for safe keeping.  Seems mostly functional, but needs substantial cleanup.  (Note that some checkins happened underneath me, so there are parts of this diff that aren't relevant.  There's also extra debug code that I'll remove.)  Still, the general idea should be clear if people wanted to comment.

Comment 8

[Joey Minta]

Attachment: update service v1

I think this does it.  However, this update stuff is extremely hard to test thoroughly, so any additional QA help would be greatly appreciated.  The trick here is that a lot of code-paths are reused (and gavin encouraged me to re-use them again) so we have to pass around some additional callbacks that not only perform extra actions for an update, but also signal that the code-path shouldn't follow normal paths for things like error reporting.

Comment 9

[Mycroft Project (CC)]

I realise I could probably get the following from the patch but my code reading skills aren't great and I suspect an answer won't take very long if you've written the code ;)

1. Am I right in saying that for Sherlock plugins the update information is specified in the same way?

2. Can you give a quick example of how the info would be specified in an OpenSearch description - I see updateCheckDays becomes updateInterval and I presume these are all specified at the same level as the URL tag (ie no <browser&rt; kludges...

3. It is necessary to feed the old mechanism with content-length and last-modified info... is there any change (in particular further requirements) to this?

Many thanks,

By the way... I'm not sure I've seen this mentioned somewhere else or not... is it going to be possible to force an update (I'm imagining a right click command) I'm thinking of cases where I know  an update has happened and I don't want to wait updateInterval days or where I want to test that the update path (and the system at the other end when it's mine) is working properly...

Comment 10

[Mike Connor [:mconnor]]

Comment on attachment 226679 [details] [diff] [review]
update service v1

>+// Check for engines to update every 1 day
>+pref("browser.search.updateinterval", 1);

nit: you can just take out the 1 in the comment, kinda awkward

> /**
>+ * Wrapper for nsIPrefBranch::getIntPref.
>+ * @param aPrefName
>+ *        The name of the pref to get.
>+ * @returns aDefault if the requested pref doesn't exist.
>+ */
>+function getIntPref(aName, aDefault) {
>+  var prefB = Cc["@mozilla.org/preferences-service;1"].
>+              getService(Ci.nsIPrefBranch);
>+  try {
>+    return prefB.getIntPref(aName);
>+  } catch (ex) {
>+    return aDefault;
>+  }
>+}

I think its generally cleaner to use the following style at the end of function, since it makes the fallback more explicit.  That said, providing a wrapper function seems wasteful in this case since its only called once from init(), so I'd just say inline this unless there's a compelling reason to keep it beyond consistency with the rest of the 

  try {
    return prefB.getIntPref(aName);
  } catch (ex) {}

  return aDefault;

>+    if (aUpdateCallback) {
>+      aUpdateCallback(aEngine);
>+    } else {
>+      // Write the engine to file
>+      aEngine._serializeToFile();

unnecessary brackets on the if

>+        case "updateInterval":
>+          this.updateInterval = child.textContent;
>+          break;

should be this._updateInterval

>-    var sherlockLines, searchSection, sourceTextEncoding;
>+    var sherlockLines, searchSection, sourceTextEncoding, browserSection;
>     try {
>       sherlockLines = sherlockBytesToLines(this._data);
>       searchSection = getSection(sherlockLines, "search");
>+      browserSection = getSection(sherlockLines, "browser");
>+
>       sourceTextEncoding = parseInt(searchSection["sourcetextencoding"]);
>       if (sourceTextEncoding) {
>         // Re-convert the bytes using the found sourceTextEncoding
>         sherlockLines = sherlockBytesToLines(this._data, sourceTextEncoding);
>         searchSection = getSection(sherlockLines, "search");
>       }
>     } catch (ex) {
>       // The conversion using the default charset failed. Remove any non-ascii
>       // bytes and try to find a sourceTextEncoding.
>       var asciiBytes = this._data.filter(function (n) {return !(0x80 & n);});
>       var asciiString = String.fromCharCode.apply(null, asciiBytes);
>       sherlockLines = asciiString.split(NEW_LINES).filter(isUsefulLine);
>+      browserSection = getSection(sherlocklines, "browser");
>       searchSection = getSection(sherlockLines, "search");
>       sourceTextEncoding = parseInt(searchSection["sourcetextencoding"]);
>       if (sourceTextEncoding) {
>         sherlockLines = sherlockBytesToLines(this._data, sourceTextEncoding);
>         searchSection = getSection(sherlockLines, "search");
>+        browserSection = getSection(sherlocklines, "browser");

should be sherlockLines instead of sherlocklines here.

why do we call getSection again in the else block case and not in the if block?

>@@ -1746,22 +1790,39 @@ Engine.prototype = {
>   get _id() {
>     ENSURE_WARN(this._file, "No _file for id!", Cr.NS_ERROR_FAILURE);
> 
>     if (this._file.parent.equals(getDir(NS_APP_USER_SEARCH_DIR)))
>       return "[profile]/" + this._file.leafName;
> 
>     if (this._file.parent.equals(getDir(NS_APP_SEARCH_DIR)))
>       return "[app]/" + this._file.leafName;
>-
>     // We're not in the profile or appdir, so this must be an extension-shipped
>     // plugin. Use the full path.
>     return this._file.path;
>   },

why remove the newline here? just don't


>+var engineUpdateService = {
>+  init: function eus_init() {
>+    var tm = Cc["@mozilla.org/updates/timer-manager;1"]
>+               .getService(Ci.nsIUpdateTimerManager);

if using the Cc prefix, period on the end, and getService aligns directly with Cc

>+    // Interval is stored in days
>+    var seconds = interval*24*60*60;

spaces around operators, do we want to calculate this on every startup? just use 86400 otherwise.


>+  addEngine: function eus_addEngine(aEngine) {
>+    LOG("adding engine to update service:"+aEngine.updateURL+','+aEngine.updateIcon);
>+    if (!aEngine.updateURL && !aEngine.updateIcon) {
>+      // We don't have anything to check
>+      return;
>+    }

no brackets, just do return; // nothing to check

the old code didn't support icon updates if the engine didn't have an update check, iirc, so we can simplify this to just checking if there's an updateURL

>+    var props = ["updateInterval", "updateURL", "updateIcon"];
>+    for each (var prop in props) {
>+      if (aEngine[prop])
>+        engineMetadataService.setAttr(aEngine, prop, aEngine[prop]);
>+    }

>+    var interval = engineMetadataService.getAttr(aEngine, "updateinterval") || 7;

7?  so we'll check daily to see if there are engines due for updating, with a default of a seven day timeframe that isn't configurable.  Kinda makes the pref less useful, no?  Also, if we're checking once per day, especially not on a slack timer, we're possibly making the update interval every two days for a one day interval.  Maybe the 'check to see if we should check' interval can be shorter than a day, since it should be a cheap check.  Maybe make it in hours and default to 6, and make sure the timer is a slack timer.

>+    var milliseconds = interval*24*60*60*1000;

same question as before, why repeatedly calculate 24*60*60*1000?  just use the hardcoded value, and spaces around operators.

>+    engineMetadataService.setAttr(aEngine, "updateexpir", Date.now()+milliseconds);

spaces around operators please

I just realized that we don't sanity-check in the parsing to ensure that we're getting an integer (or even just a number).  Fix that!

>+  notify: function eus_Notify(aTimer) {
>+    LOG("notify called");
>+    // Our timer has expired, but unfortunately, we can't get any data from it.
>+    // Therefore, we need to walk our engine-list, looking for expired engines
>+    var searchService = Cc["@mozilla.org/browser/search-service;1"]
>+                          .getService(Ci.nsIBrowserSearchService);

same style nit about the Cc prefix.

>+    var currentTime = Date.now();
>+    for each (engine in searchService.getEngines({})) {
>+      engine = engine.wrappedJSObject;
>+      LOG("checking engine:"+engine.name);
>+      var expirTime = engineMetadataService.getAttr(engine, "updateexpir");
>+      LOG("expirTime:"+expirTime+', currentTime:'+currentTime);
>+      if (!expirTime || expirTime > currentTime)
>+        continue;

add a newline here

Comment 11

[Axel Hecht [:Pike]]

Regarding the firefox.js comment, it'd be nice to make it say that it's "days", at least. Other updateintervals are in seconds or other arbitrary units.

Comment 12

[Joey Minta]

(In reply to comment #10)
> 
> >+    var props = ["updateInterval", "updateURL", "updateIcon"];
> >+    for each (var prop in props) {
> >+      if (aEngine[prop])
> >+        engineMetadataService.setAttr(aEngine, prop, aEngine[prop]);
> >+    }
> 
> >+    var interval = engineMetadataService.getAttr(aEngine, "updateinterval") || 7;
> 
> 7?  so we'll check daily to see if there are engines due for updating, with a
> default of a seven day timeframe that isn't configurable.  Kinda makes the pref
> less useful, no?
The idea here is that all rational engines, if they're including an updateURL, will also include an update interval.  The 7 default is just bullet-proofing in the case that an engine is weird and doesn't include the interval.  I don't really see any need to expose this to the user, as we don't expose a general way to over-ride update intervals for engines.

Comment 13

[Joey Minta]

Attachment: update service v2

This *should* address all of mconnor's comments, but I won't have a stable enough internet connection to test this fully until tomorrow morning.  If someone else wants to step up and smoke test this, simply change |var seconds| to a small number and set app.update.interval in about:config to a small number.  Then install the food-network engine from 'Get More Engines', as this has valid update info.  I'll ask for review once this has been smoketested.

Comment 14

[Axel Hecht [:Pike]]

Regarding the fallback update frequency, that should rather be longer than shorter. In case a server has trouble, asking more often rather than less has proven to be extremely painful. If we'd change the week, I could go for a month rather than a day;-).

Comment 15

[Joey Minta]

Comment on attachment 228073 [details] [diff] [review]
update service v2

Carrying forward super-review+ from mconnor.  I still think that 7 is the right way to go, since this is what all of the example update xmls use, and the shorter timeframe could be relevant in the case of a security problem.

Comment 16

[Mike Schroepfer]

The patch v2 seems to have the update interval set to 6 hrs (!) which is way too frequent. 

Also - how much of this re-uses existing code?  Given the number of strange bugs we've had with SoftwareUpdate (i.e. hour of terror) I'm a little nervous about sneaking in something like this.  Can we re-use timers and other code from SoftwareUpdate?

Comment 17

[Joey Minta]

(In reply to comment #16)
> The patch v2 seems to have the update interval set to 6 hrs (!) which is way
> too frequent.
The patch checks for expired engines (a relaitively cheap call) every 6 hours, but engines will only expire at the interval specified in their xml files (usually 7 days).  This allows us to ensure that we always update engines at their desired update frequency.
 
> 
> Also - how much of this re-uses existing code?  Given the number of strange
> bugs we've had with SoftwareUpdate (i.e. hour of terror) I'm a little nervous
> about sneaking in something like this.  Can we re-use timers and other code
> from SoftwareUpdate?
> 
The timer service used here was the one rob_strong pointed me to as the most reliably tested and least likely to leak.  It's the same one EM uses, afaik.

Comment 18

[Axel Hecht [:Pike]]

Without reading (well, understanding) the patch, what happens on a server error? Do we wait for the plugin-specified period of time or do we check every 6 hours?
I think we fixed that back then to make failure be a valid check, not sure if this impl does so, too. This posed a serious problem in the old scheme.

Comment 19

[Joey Minta]

(In reply to comment #18)
> Without reading (well, understanding) the patch, what happens on a server
> error? Do we wait for the plugin-specified period of time or do we check every
> 6 hours?
Failure is a valid check here (all the requests are async and the updateexpir is bumped right after we send them).  We'll wait another 7 days (or whatever the interval is) before checking again.

Comment 20

[Joey Minta]

Gavin and I concerned that landing this patch at this late stage is going to be rushing things.  It can be done, but at substantial risk.  How critical do drivers consider this for beta1?

Comment 21

[Darin Fisher]

> The timer service used here was the one rob_strong pointed me to as the most
> reliably tested and least likely to leak.  It's the same one EM uses, afaik.

In fact, nsIUpdateTimerManager is the timer that software update uses :-)

Comment 22

[Mike Schroepfer]

You should take the time to do this right - I've updated the target milestone to b2.  Since this won't be enabled for shipped plugins this shouldn't be a big deal.

Comment 23

[Mike Shaver (:shaver emeritus)]

(In reply to comment #22)
> Since this won't be enabled for shipped plugins this shouldn't be a big
> deal.

It won't?  Why is that?  Being able to update the plugins we shipped with has been very helpful, in a bacon-saving sort of way, in the past.

Comment 24

[Mike Schroepfer]

When did we update a search plugin?  

Doing so will break the next partial update and require us to integrate the change into the full update so as to not clobber it.  The broader question is why should search engine plugins be any different than any other code shipped by default in the browser - i.e. updated through software update.

(In reply to comment #23)
> (In reply to comment #22)
> > Since this won't be enabled for shipped plugins this shouldn't be a big
> > deal.
> 
> It won't?  Why is that?  Being able to update the plugins we shipped with has
> been very helpful, in a bacon-saving sort of way, in the past.
> 

Comment 25

[Mike Shaver (:shaver emeritus)]

(In reply to comment #24)
> When did we update a search plugin?  

In 1.0.8-ish, to fix some problems with a few regions' search codes.

> Doing so will break the next partial update and require us to integrate the
> change into the full update so as to not clobber it.

Why would it do that?  Don't they live in the profile directory now, when updated by the user?

If the user removes a search plugin via the manager, do they also bust the update model?  Will they have the plugin put back on them when the get the full update?

Comment 26

[Axel Hecht [:Pike]]

searchplugins used to be non-fatal parts of updates for a long time, I have no idea where that is hooked up, but we should check that that's still the case.

Regarding removed plugins, aren't those just marked as such in the profile data and not physically removed from the install? I thought/hoped so.

But yes, it sounds like someone that know what full and incremental updates do should have a pow-wow with someone that understands the new scenarios of our search engines.

Shipping search engines with update info gives us a chance to change them without having to do a security fire drill. That aside, if we upload a change to a plugin on the server, it should surely land in our sources and be part of the next update, too. As it should be part of new installs.
Depending on how things really work in the install and profile with the two update paths (application and search plugin update), can that confuse the search service?

Comment 27

[Joey Minta]

We've been treating app-shipped engines as immutable.  We set special markers on them to hide them from users if a user chooses to remove one or more of them.  App-shipped engines currently do not come with any update information and it was my impression that using the app-update system would be the preferred method of updating those engines, partly to avoid the conflicts between update systems folks have outlined here.  Gavin may want to comment more on this.

Note also that it's possible to disable search-engine updates in the preferences separate from app-updates.  This seems to indicate that we should update even app-shipped engines using the engine system, since it will respect the user's preference, but it also could make it more likely for users to miss a security update for an engine.

Comment 28

[Joey Minta]

Attachment: update service v3

I have significantly more confidence in this patch.  Now includes only single downloading of an engine to update and the use of _setIcon for icon updates.  The edge case is still where we have both an icon update and an engine update, but I don't see a way around that.

Comment 29

[Joey Minta]

Attachment: update service v4

Moving around some code-flow after discussion with Gavin on IRC.  We're now using notifyAction with a reference to the old engine, rather than adding a method on the search service.  This means we'll also pass through _addEngineToStore (where it is a bit tricky to avoid their safety checks) to register the updated engine.

Comment 30

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

Attachment: update service v5

I've done a bit of work on this, I found a few somewhat unrelated bugs that doing this exposed. This has been fairly well tested with sherlock files, but it hasn't been tested with XML files (they should be essentially the same). I tested "engine after icon" and "icon after engine" update scenarios to try and catch any potential race conditions.

Comment 31

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

Attachment: update service v6

Tested this with XML based plugins, this is ready for review.

Comment 32

[Joey Minta]

Comment on attachment 229125 [details] [diff] [review]
update service v6

    } catch (ex) {
       // The conversion using the default charset failed. Remove any non-ascii
       // bytes and try to find a sourceTextEncoding.
       var asciiBytes = this._data.filter(function (n) {return !(0x80 & n);});
       var asciiString = String.fromCharCode.apply(null, asciiBytes);
       sherlockLines = asciiString.split(NEW_LINES).filter(isUsefulLine);
       searchSection = getSection(sherlockLines, "search");
+      browserSection = getSection(sherlockLines, "browser");
       sourceTextEncoding = parseInt(searchSection["sourcetextencoding"]);
       if (sourceTextEncoding) {
         sherlockLines = sherlockBytesToLines(this._data, sourceTextEncoding);
         searchSection = getSection(sherlockLines, "search");
+        browserSection = getSection(sherlockLines, "browser");
       } else
         ERROR("Couldn't find a working charset", Cr.NS_ERROR_FAILURE);
     }
As I read this, we don't need the first browserSection assignment.  The searchSection is needed to try and get the encoding, but we'll either find the encoding, and get the browserSection, or not find it, and error out.

+    var updateCheckDays = parseInt(browserSection["updatecheckdays"]);
+    if (!isNaN(updateCheckDays))
+      this._updateInterval = updateCheckDays;
+    this._updateURL = browserSection["update"];
+    this._iconUpdateURL = browserSection["updateicon"];
I think we're better off setting the default here in the case of isNaN(updateCheckDays), so that anyone else who might care about this value doesn't have to do the same null checks we currently do in the update service.

+    function appendTextNode(aDoc, aNameSpace, aLocalName, aValue) {
+      if (!aValue)
+        return null;
+      var node = aDoc.createElementNS(aNameSpace, aLocalName);
+      node.appendChild(aDoc.createTextNode(aValue));
+      aDoc.documentElement.appendChild(node);
+      aDoc.documentElement.appendChild(aDoc.createTextNode("\n"));
+      return node;
+    }
+
Since |doc| is in your scope, you can use that and save a param.  Or would that leak?

+      var imageNode = appendTextNode(doc, OPENSEARCH_NS_11, "Image",
+                                     this._iconURI.spec);
+      imageNode.setAttribute("width", "16");
+      imageNode.setAttribute("height", "16");
     }
Can we ever have a case where _iconURI is non-null, but the spec is?  That would error out badly here.

The browserSection comment is the main one. r=jminta with that, and with a passing glance at the rest.

Comment 33

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

Attachment: update service v6.1

(In reply to comment #32)
> As I read this, we don't need the first browserSection assignment.  The
> searchSection is needed to try and get the encoding, but we'll either find the
> encoding, and get the browserSection, or not find it, and error out.

Oh, good catch! You're right, I removed that.

> +    var updateCheckDays = parseInt(browserSection["updatecheckdays"]);
> +    if (!isNaN(updateCheckDays))
> +      this._updateInterval = updateCheckDays;
> +    this._updateURL = browserSection["update"];
> +    this._iconUpdateURL = browserSection["updateicon"];
> I think we're better off setting the default here in the case of
> isNaN(updateCheckDays), so that anyone else who might care about this value
> doesn't have to do the same null checks we currently do in the update service.

I took advantage of the fact that NaN evaluates to false in both _parseAs methods so that we always assign to _updateInterval, falling back to the default value. I also changed the null check in scheduleNextUpdate to an ENSURE_WARN, since it should now never be null.

> +    function appendTextNode(aDoc, aNameSpace, aLocalName, aValue) {
...
> +    }
> Since |doc| is in your scope, you can use that and save a param.  Or would
> that leak?

I had originally defined it in the global scope and didn't bother changing it when I moved it. Made this change.

> +      var imageNode = appendTextNode(doc, OPENSEARCH_NS_11, "Image",
> +                                     this._iconURI.spec);
> +      imageNode.setAttribute("width", "16");
> +      imageNode.setAttribute("height", "16");
>      }
> Can we ever have a case where _iconURI is non-null, but the spec is?  That
> would error out badly here.

I removed the check I made when I wrote this because I didn't such a case was possible, but I added it back. Better safe than sorry.

Comment 34

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

Comment on attachment 229302 [details] [diff] [review]
update service v6.1

<mconnor> gavin_: ok, moa=me, just land it on trunk

Comment 35

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

Attachment: update service v6.2

I found one bug in the previous patch, the relevant change is in _addEngineToStore (removing the engine from the hash before updating it's name). These changes are the only thing I'm asking for review for, the other changes are just merge changes from the patch for bug 341668.

Comment 36

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

Bugzilla's interdiff isn't much use here, I found http://gavinsharp.com/tmp/327932.html more useful. It's a diff of 6.1 vs 6.2.

Comment 37

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

Checked in on the trunk.

mozilla/browser/components/search/nsSearchService.js 	1.56
mozilla/browser/components/search/nsIBrowserSearchService.idl 	1.13
mozilla/browser/app/profile/firefox.js 	1.132

Comment 38

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

Attachment: branch rollup

This is against current branch, with the fix for bug 345404 rolled in.

Comment 39

[Joey Minta]

Comment on attachment 230591 [details] [diff] [review]
branch rollup

Nominating for approval.  This has been on trunk since last Thursday.  The one severe regression reported in bug 345404 has been rolled into this patch.  The search engine update code is fairly self contained, so risk to other code should be small.  Getting this onto branch for further testing of the actual update code would be ideal.

The patch has been fairly thoroughly tested by gavin and myself, with some help from zach too.

Comment 40

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

Comment on attachment 230591 [details] [diff] [review]
branch rollup

a=drivers. Please land on the MOZILLA_1_8_BRANCH.

Comment 41

[Joey Minta]

Landed on branch with some minor conflict tidying.
Checking in browser/app/profile/firefox.js;
/cvsroot/mozilla/browser/app/profile/firefox.js,v  <--  firefox.js
new revision: 1.71.2.53; previous revision: 1.71.2.52
done
Checking in browser/components/search/nsIBrowserSearchService.idl;
/cvsroot/mozilla/browser/components/search/nsIBrowserSearchService.idl,v  <--  nsIBrowserSearchService.idl
new revision: 1.1.2.14; previous revision: 1.1.2.13
done
Checking in browser/components/search/nsSearchService.js;
/cvsroot/mozilla/browser/components/search/nsSearchService.js,v  <--  nsSearchService.js
new revision: 1.1.2.50; previous revision: 1.1.2.49
done