Closed Bug 722332 Opened 12 years ago Closed 12 years ago

add async initialization API to nsIBrowserSearchService

Categories

(Firefox :: Search, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
Firefox 16

People

(Reporter: Yoric, Assigned: Yoric)

References

Details

(Keywords: addon-compat, dev-doc-needed, Whiteboard: [Snappy:P1])

Attachments

(2 files, 29 obsolete files)

13.08 KB, patch
Details | Diff | Splinter Review
10.33 KB, patch
Gavin
: review+
Details | Diff | Splinter Review
Sub-bug extracted from bug 699856.
When it comes to patching individual consumers of the new async API, I think you can roll those changes into a single patch.
(In reply to Gavin Sharp (use gavin@gavinsharp.com for email))
> Comment on attachment 591036 [details] [diff] [review]
> Asynchronous API (shallow changes)
> 
> >diff --git a/netwerk/base/public/nsIBrowserSearchService.idl b/netwerk/base/public/nsIBrowserSearchService.idl
> 
> >+interface nsIObserver;
> 
> I think I mentioned this earlier, but I think you should add a new interface
> for this rather than re-using nsIObserver.

Well, the word "overkill" did appear in your suggestion. But if you think it necessary, I will do that.

> > interface nsIBrowserSearchService : nsISupports
> 
> >+  bool init([optional] in nsIObserver aObserver);
> 
> Let's not make this return bool, since the next step will be making
> initialization always-async, so there's no point.

Well, there is the point that some clients can decide to bailout and retry later if initialization is not complete yet.

Would you prefer two functions:
- void init([optional] in nsISearchServiceInitObserver aObserver)
- readonly attribute bool isInitialized
?
 
> >diff --git a/toolkit/components/search/nsSearchService.js b/toolkit/components/search/nsSearchService.js
> 
> >+Components.utils.import("resource://gre/modules/schedule.jsm");
> 
> What's the bug for this API?

bug 692420

> >+Components.utils.import("resource:///modules/devtools/Promise.jsm");
> 
> I don't see this being used.

True. I expected to use this at a later stage, but it is not necessary at this stage.

> >+//Why exactly do we have a prototype for something that is a singleton?
> > SearchService.prototype = {
> 
> Standard boilerplate. We could get rid of it but there isn't much benefit.

Well, it is quite confusing to see, for instance, a prototype containing an array.

> >+  init: function SRCH_SVC__init(aObserver) {
> >+    let initialization = this._initialization;
> >+    if (!initialization) {
> 
> I don't see this._initialization being set to null, where is that supposed
> to happen?

In the constructor, about 10 lines higher.


> >   get currentEngine() {
> 
> >-    if (!this._currentEngine || this._currentEngine.hidden)
> >+    if (!this._currentEngine || this._currentEngine.hidden) {
> >       this._currentEngine = this.defaultEngine;
> >+    }
> 
> omit this change

As you wish.

> 
> > var engineMetadataService = {
> 
> >   get mDB() {
> 
> >-      return this._mDB = this._migrateOldDB();
> >+      this._mDB = this._migrateOldDB();
> >+      this._commit();
> >+      return this._mDB;
> 
> Couldn't _migrateOldDB just call _commit?

It could. Would you prefer that?
Depends on: JSScheduleAPI
(In reply to Gavin Sharp (use gavin@gavinsharp.com for email) from comment #1)
> When it comes to patching individual consumers of the new async API, I think
> you can roll those changes into a single patch.

OK. You haven't replied to my question regarding the API, by the way.

As you prefer a new interface for this initialization callback, I would tend to go for something reusable, say |nsICallback| or |nsIInitCallback|. Is this compatible with what you had in mind?

Also, I would like to have an attribute to tell me whether initialization is complete, so as to be able to bailout from calls when it is not. Is that ok with you?
(In reply to David Rajchenbach Teller [:Yoric] from comment #3)
> OK. You haven't replied to my question regarding the API, by the way.

Which question?

> As you prefer a new interface for this initialization callback, I would tend
> to go for something reusable, say |nsICallback| or |nsIInitCallback|. Is
> this compatible with what you had in mind?

No, I think you should just use a one-off specific interface. nsIBrowserSearchCallback or whatever.

> Also, I would like to have an attribute to tell me whether initialization is
> complete, so as to be able to bailout from calls when it is not. Is that ok
> with you?

I'm not sure I understand why any client would want to use that. Can you give some specific use cases?
(In reply to Gavin Sharp (use gavin@gavinsharp.com for email) from comment #4)
> (In reply to David Rajchenbach Teller [:Yoric] from comment #3)
> > OK. You haven't replied to my question regarding the API, by the way.
> 
> Which question?

Never mind, you have now answered.

> > As you prefer a new interface for this initialization callback, I would tend
> > to go for something reusable, say |nsICallback| or |nsIInitCallback|. Is
> > this compatible with what you had in mind?
> 
> No, I think you should just use a one-off specific interface.
> nsIBrowserSearchCallback or whatever.

Ok. 

> > Also, I would like to have an attribute to tell me whether initialization is
> > complete, so as to be able to bailout from calls when it is not. Is that ok
> > with you?
> 
> I'm not sure I understand why any client would want to use that. Can you
> give some specific use cases?

See attachment 591044 [details] [diff] [review] and attachment 591040 [details] [diff] [review] of bug 699856. Barring any misunderstanding of my part, these clients can handle nicely the fact that the search service is not ready to give out information yet. Letting them check whether initialization has been completed looks like the best way to keep this refactoring bounded.
Attached patch Asynchronous API (obsolete) — Splinter Review
Attaching an asynchronous API. Note that, as we agreed in bug 699856, the implementation itself is mostly asynchronous. For the moment, the idea is just to stabilize the API, adapt the tests and update the clients, before proceeding to the actual change of implementation. Also, the asynchronous aspects depend in the JS Schedule API (bug 692420).
Attached patch Clients of nsSearchService (obsolete) — Splinter Review
Attaching a version of the clients of nsSearchService adapted to either wait until initialization is complete before proceeding or bailout and fail if initialization is not complete yet.
Attachment #600038 - Flags: review?(gavin.sharp)
Attachment #600048 - Flags: review?(gavin.sharp)
Comment on attachment 600038 [details] [diff] [review]
Asynchronous API

The API changes look good to me.

nit: I'd remove the "service" in nsIBrowserSearchServiceInitObserver, I think - it's a bit of a mouthful (XPCOM will do that to you!)

I find the implementation changes that depend on the Schedule API a little confusing, mostly because I'm not familiar with that API. I would prefer that they be simpler and use .bind() or .call(), rather than depending so much on Schedule magic (e.g. I don't know what "trap" does).
Attachment #600038 - Flags: review?(gavin.sharp) → feedback+
Attachment #600048 - Flags: feedback?(mnoorenberghe+bmo)
(In reply to Gavin Sharp (use gavin@gavinsharp.com for email) from comment #9)
> Comment on attachment 600038 [details] [diff] [review]
> Asynchronous API
> 
> The API changes look good to me.
> 
> nit: I'd remove the "service" in nsIBrowserSearchServiceInitObserver, I
> think - it's a bit of a mouthful (XPCOM will do that to you!)

No problem. I personally would have called it just nsIInitObserver, mind you.

> I find the implementation changes that depend on the Schedule API a little
> confusing, mostly because I'm not familiar with that API. I would prefer
> that they be simpler and use .bind() or .call(), rather than depending so
> much on Schedule magic (e.g. I don't know what "trap" does).

The idea is to enforce asynchronicity, which neither |bind| or |call| will do. Actually, I am pretty certain that any rewrite of a synchronous API into something asynchronous is bound to be confusing. Schedule was developed mostly to factorize the most common patterns of such rewrites, hence make them less confusing.

By the way, method |trap| landed yesterday, as part of bug 711126. It is an asynchronous counterpart for |catch|: it catches exceptions thrown by a queued function or raised externally by a call to |reject|.
Attached patch Asynchronous API (obsolete) — Splinter Review
Same API, after having addressed the suggestions of Gavin, both on BZ and on IRC
Attachment #606228 - Attachment is obsolete: true
Attachment #600048 - Attachment is obsolete: true
Attachment #600048 - Flags: review?(gavin.sharp)
Attachment #600048 - Flags: feedback?(mnoorenberghe+bmo)
Attachment #600040 - Attachment is obsolete: true
Attachment #606230 - Flags: review?(mnoorenberghe+bmo)
Attachment #606230 - Flags: review?(gavin.sharp)
Attachment #606231 - Flags: review?(mnoorenberghe+bmo)
Attachment #606231 - Flags: review?(gavin.sharp)
Attachment #606236 - Flags: review?(mnoorenberghe+bmo)
Attachment #606236 - Flags: review?(gavin.sharp)
Attachment #600038 - Attachment is obsolete: true
Comment on attachment 606230 [details] [diff] [review]
Asynchronous API

Review of attachment 606230 [details] [diff] [review]:
-----------------------------------------------------------------

::: netwerk/base/public/nsIBrowserSearchService.idl
@@ +170,1 @@
>  [scriptable, uuid(8307b8f2-08ea-45b8-96bf-b1dc7688fe3b)]

This UUID needs to be changed.

@@ +398,5 @@
> +/**
> + * Callback for asynchronous initialization of nsIBrowserSearchService
> + */
> +[scriptable, function, uuid(02256156-16e4-47f1-9979-76ff98ceb590)]
> +interface nsIBrowserSearchInitObserver: nsISupports

Nit: space before the colon.

I'd prefer to use a more general name for the interface (like Gavin's suggestion in comment 4), rather than making this init-specific.  That way future async search service changes can use the same interface. I prefer nsIBrowserSearchObserver.

(Quoting Gavin Sharp from comment #4)
> No, I think you should just use a one-off specific interface.
> nsIBrowserSearchCallback or whatever.

::: toolkit/components/search/nsSearchService.js
@@ +2575,5 @@
> +          // In that case, |gEnginesLoaded| and |_initComplete| remain |false|.
> +          this._loadEngines();
> +          gEnginesLoaded = true;
> +          this._addObservers();
> +          this.isInitialized = true;

Do we really need gEnginesLoaded and isInitialized?

@@ +2587,5 @@
> +        aObserver.onInitComplete.bind(aObserver, Components.results.NS_OK)
> +      );
> +      initialization.trap(
> +        aObserver.onInitComplete.bind(aObserver,
> +                                      Components.results.NS_ERROR_FAILURE)

Can we continue to log a message when loadEngines fails?

@@ +3834,5 @@
>      }
>      LOG("epsCommit: (re)setting timer");
>      this._lazyWriter.go();
>    },
> +  _lazyWriter: null

Nit: The trailing comma was fine here. Having trailing commas helps for VCS blame when someone adds another member afterwards.
Attachment #606230 - Flags: review?(mnoorenberghe+bmo)
Comment on attachment 606230 [details] [diff] [review]
Asynchronous API

Review of attachment 606230 [details] [diff] [review]:
-----------------------------------------------------------------

::: toolkit/components/search/nsSearchService.js
@@ +2571,5 @@
> +
> +      this._initializer = initialization = Schedule.queue(
> +        (function() {
> +          // Note: loadEngines can fail.
> +          // In that case, |gEnginesLoaded| and |_initComplete| remain |false|.

This comment needs to be updated to reflect the renaming of |_initComplete|.
Comment on attachment 606231 [details] [diff] [review]
Clients of nsSearchService

Review of attachment 606231 [details] [diff] [review]:
-----------------------------------------------------------------

::: browser/base/content/nsContextMenu.js
@@ +1273,5 @@
>               getService(Ci.nsIBrowserSearchService);
> +
> +    if (!ss.isInitialized) {
> +      // If the search service is not initialized yet, there is not
> +      // much we can do.

Is this block necessary? The silent return here could hide a problem with the first time the menu item is shown.

::: browser/base/content/safeMode.js
@@ +106,5 @@
>    var searchService = Components.classes["@mozilla.org/browser/search-service;1"]
>                                  .getService(Components.interfaces.nsIBrowserSearchService);
> +  searchService.
> +      init(
> +           function(status) {

Give a name to this function. Nit: keep |function| and |init| on the same line as |searchService.|:

ie. |searchService.init(function ssInitCallback(status) {|

::: browser/components/nsBrowserContentHandler.js
@@ +317,5 @@
>  function doSearch(searchTerm, cmdLine) {
>    var ss = Components.classes["@mozilla.org/browser/search-service;1"]
>                       .getService(nsIBrowserSearchService);
> +  ss.init(
> +    function(aResult) {

Nit: same as safeMode.js

@@ +349,5 @@
> +                        "chrome,dialog=no,all" +
> +                        gBrowserContentHandler.getFeatures(cmdLine),
> +                        sa);
> +    }
> +  );

Nit: });

::: browser/components/search/content/search.xml
@@ +119,5 @@
>            os.addObserver(this, "browser-search-engine-modified", false);
>  
>            this._addedObserver = true;
> +
> +          this.searchService.init((function(status) {

Name this function too

::: docshell/base/nsDefaultURIFixup.cpp
@@ +396,5 @@
>      // Try falling back to the search service's default search engine
>      nsCOMPtr<nsIBrowserSearchService> searchSvc = do_GetService("@mozilla.org/browser/search-service;1");
>      if (searchSvc) {
> +        bool isSearchInitialized = false;
> +        nsresult rv = searchSvc -> GetIsInitialized(&isSearchInitialized);

Nit: remove spaces around |->| throughout your changes to this file.

@@ +417,5 @@
> +                // for a magic "application/x-moz-keywordsearch"
> +                // submission type. In the future, we should instead
> +                // use a solution that relies on bug 587780.
> +                defaultEngine->
> +                    GetSubmission(NS_ConvertUTF8toUTF16(keyword),

Nit: the wrapping here was fine.

::: toolkit/components/search/nsSearchSuggestions.js
@@ +458,5 @@
>      this.stopSearch();
>  
> +    let self = this;
> +    searchService.init(
> +      function(aResult) {

Same as others: same line and name the function.

@@ +459,5 @@
>  
> +    let self = this;
> +    searchService.init(
> +      function(aResult) {
> +        if (aResult != Components.results.NS_OK) {

s/Components.results/Cr/

@@ +460,5 @@
> +    let self = this;
> +    searchService.init(
> +      function(aResult) {
> +        if (aResult != Components.results.NS_OK) {
> +          return;

This silent return may hide future bugs.  Perhaps Cu.reportError("…")?

@@ +461,5 @@
> +    searchService.init(
> +      function(aResult) {
> +        if (aResult != Components.results.NS_OK) {
> +          return;
> +        }

Nit: You don't need to brace this single-line block.

@@ +484,4 @@
>  
> +        // Actually do the search
> +        self._request = Cc["@mozilla.org/xmlextras/xmlhttprequest;1"].
> +          createInstance(Ci.nsIXMLHttpRequest);

Nit: fix indentation of createInstance (align with Cc).

@@ +494,2 @@
>  
> +        self._request.onreadystatechange = function() {

name this function

@@ +495,5 @@
> +        self._request.onreadystatechange = function() {
> +          self.onReadyStateChange();
> +          // Note: we cannot simply |bind| here, as |onReadyStateChange| seems
> +          // to be defined later.
> +        };

Maybe I'm missing something, but what was wrong with how onReadyStateChange was being attached?
Attachment #606231 - Flags: review?(mnoorenberghe+bmo)
Comment on attachment 606236 [details] [diff] [review]
Companion testsuite

Review of attachment 606236 [details] [diff] [review]:
-----------------------------------------------------------------

::: toolkit/components/search/tests/xpcshell/head_search.js
@@ +64,5 @@
>  var gProfD = do_get_profile();
>  
>  function dumpn(text)
>  {
> +  dump("nsIBrowserSearchService Test: "+text+"\n");

Nit: these tests are not necessarily just for nsIBrowserSearchService. Maybe "search test:" instead?  Please put spaces around the + operator despite them being missing before.

@@ +107,5 @@
>    return data;
>  }
>  
> +// Expand the amount of information available in error logs
> +

Nit: remove the extra newline.

@@ +112,2 @@
>  Services.prefs.setBoolPref("browser.search.log", true);
> +Schedule.DEBUG = true;

Seems somewhat unfortunate that we'd have/want to set this in each component using Schedule.jsm (along with the import).  If this is something we want for all tests then it should probably be at the test suite level rather than per-component.

Off topic: Seems like a debug pref may be better to help developers and testers diagnose bugs without having to set this property in code. Then it's just a matter of adding to to the test suite's default prefs.

::: toolkit/components/search/tests/xpcshell/test_645970.js
@@ +48,5 @@
>  
>    // The search service needs to be started after the jarURIs pref has been
>    // set in order to initiate it correctly
> +  Services.search.init(function(service, status) {
> +      do_check_eq(status, Components.utils.NS_OK);

Please name functions to aid in debugging. 
There should only be one argument passed to the callback.

::: toolkit/components/search/tests/xpcshell/test_migratedb.js
@@ +35,5 @@
>      }
>    );
>  
> +  search.init(function() {
> +              search.getEngines();

name this function and fix the indentation.

::: toolkit/components/search/tests/xpcshell/test_nodb.js
@@ +23,5 @@
> +  search.init(
> +    function() {
> +      do_timeout(500,
> +                 function()
> +                 {

Nit: for consistency, please fix this to have the opening function brace on the same line as |function|.
Attachment #606236 - Flags: review?(mnoorenberghe+bmo)
(In reply to Matthew N. [:MattN] from comment #17)
> Is this block necessary? The silent return here could hide a problem with
> the first time the menu item is shown.

I am not sure. This is part of the reason for which I requested a review on the first version of this code back in January, because I wanted the opinion of someone who knows that codebase better than I do.

Now, my understanding is that it cannot hurt, and it will have the same effect as triggering the context menu without having selected any text.


> ::: docshell/base/nsDefaultURIFixup.cpp
> @@ +396,5 @@
> >      // Try falling back to the search service's default search engine
> >      nsCOMPtr<nsIBrowserSearchService> searchSvc = do_GetService("@mozilla.org/browser/search-service;1");
> >      if (searchSvc) {
> > +        bool isSearchInitialized = false;
> > +        nsresult rv = searchSvc -> GetIsInitialized(&isSearchInitialized);
> 
> Nit: remove spaces around |->| throughout your changes to this file.
> 
> @@ +417,5 @@
> > +                // for a magic "application/x-moz-keywordsearch"
> > +                // submission type. In the future, we should instead
> > +                // use a solution that relies on bug 587780.
> > +                defaultEngine->
> > +                    GetSubmission(NS_ConvertUTF8toUTF16(keyword),
> 
> Nit: the wrapping here was fine.

Well, it gets the next line way past 80 columns (98, by my account).

> @@ +494,2 @@
> >  
> > +        self._request.onreadystatechange = function() {
> 
> name this function
> 
> @@ +495,5 @@
> > +        self._request.onreadystatechange = function() {
> > +          self.onReadyStateChange();
> > +          // Note: we cannot simply |bind| here, as |onReadyStateChange| seems
> > +          // to be defined later.
> > +        };
> 
> Maybe I'm missing something, but what was wrong with how onReadyStateChange
> was being attached?

I can't find out right now. Getting rid of this.
Attachment #606230 - Attachment is obsolete: true
Attachment #606230 - Flags: review?(gavin.sharp)
Attachment #606231 - Attachment is obsolete: true
Attachment #606231 - Flags: review?(gavin.sharp)
(In reply to Matthew N. [:MattN] from comment #18)
> @@ +112,2 @@
> >  Services.prefs.setBoolPref("browser.search.log", true);
> > +Schedule.DEBUG = true;
> 
> Seems somewhat unfortunate that we'd have/want to set this in each component
> using Schedule.jsm (along with the import).  If this is something we want
> for all tests then it should probably be at the test suite level rather than
> per-component.

That would work, too. For the moment, I wanted to keep my changes as small as possible (read "I want Schedule to land, eventually"), but I can open a followup bug on the topic.

> Off topic: Seems like a debug pref may be better to help developers and
> testers diagnose bugs without having to set this property in code. Then it's
> just a matter of adding to to the test suite's default prefs.

I guess I could modify Schedule so that, if it is executed in the main thread, it checks whether that preference is set. I would tend to do this in a second bug, though, once Schedule has landed.

> ::: toolkit/components/search/tests/xpcshell/test_645970.js
> @@ +48,5 @@
> >  
> >    // The search service needs to be started after the jarURIs pref has been
> >    // set in order to initiate it correctly
> > +  Services.search.init(function(service, status) {
> > +      do_check_eq(status, Components.utils.NS_OK);
> 
> Please name functions to aid in debugging. 
> There should only be one argument passed to the callback.

Good spot. Even taking weak typing into account, I can't quite figure out why the test passed.
Attachment #606236 - Attachment is obsolete: true
Attachment #606236 - Flags: review?(gavin.sharp)
Attachment #608741 - Flags: review?(mnoorenberghe+bmo)
Attachment #608742 - Flags: review?(mnoorenberghe+bmo)
Comment on attachment 608744 [details] [diff] [review]
Companion testsuite

Thanks for the reviews. I attach updated versions.
Attachment #608744 - Flags: review?(mnoorenberghe+bmo)
(In reply to David Rajchenbach Teller [:Yoric] from comment #19)
> (In reply to Matthew N. [:MattN] from comment #17)
> > Is this block necessary? The silent return here could hide a problem with
> > the first time the menu item is shown.
> 
> I am not sure. This is part of the reason for which I requested a review on
> the first version of this code back in January, because I wanted the opinion
> of someone who knows that codebase better than I do.
> 
> Now, my understanding is that it cannot hurt, and it will have the same
> effect as triggering the context menu without having selected any text.

What I'm saying is that not showing the context menu item in this case is a bug and so I'd like to make it more obvious when such a case occurs.  Reporting an error to the error console (Components.Utils.reportError) would help IMO but we can wait and see what Gavin thinks.

> > ::: docshell/base/nsDefaultURIFixup.cpp
> > @@ +417,5 @@
> > > +                // for a magic "application/x-moz-keywordsearch"
> > > +                // submission type. In the future, we should instead
> > > +                // use a solution that relies on bug 587780.
> > > +                defaultEngine->
> > > +                    GetSubmission(NS_ConvertUTF8toUTF16(keyword),
> > 
> > Nit: the wrapping here was fine.
> 
> Well, it gets the next line way past 80 columns (98, by my account).

Yes, I noticed that but line 397 was already at 104 characters and since you weren't actually changing that line I think it's better to leave it.
(In reply to David Rajchenbach Teller [:Yoric] from comment #22)

A follow-up for schedule.jsm debug output works for me.
 
> > ::: toolkit/components/search/tests/xpcshell/test_645970.js
> > There should only be one argument passed to the callback.
> 
> Good spot. Even taking weak typing into account, I can't quite figure out
> why the test passed.

I didn't understand how it could pass either but I just assumed you didn't run it :P
Comment on attachment 608741 [details] [diff] [review]
Asynchronous API

Review of attachment 608741 [details] [diff] [review]:
-----------------------------------------------------------------

::: netwerk/base/public/nsIBrowserSearchService.idl
@@ +398,5 @@
> +/**
> + * Callback for asynchronous initialization of nsIBrowserSearchService
> + */
> +[scriptable, function, uuid(02256156-16e4-47f1-9979-76ff98ceb590)]
> +interface nsIBrowserSearchObserver: nsISupports

Nit: space before the colon.

I didn't notice the function modifier when I first reviewed the IDL so my opinion has changed somewhat.
It does make the code cleaner with the modifier but either the interface name should go back to what it was (apologies) or the callback function name should be more generic. I'll defer this to Gavin.

::: toolkit/components/search/nsSearchService.js
@@ +2563,5 @@
>  
> +  /**
> +   * Perform asynchronous initialization
> +   */
> +  init: function SRCH_SVC__init(aObserver) {

Nit: one underscore here.  The pattern is "SRCH_SVC_" + member name and init doesn't have a leading underscore.

@@ +2570,5 @@
> +    if (!initialization) {
> +      // Start initialization
> +
> +      this._initializer = initialization = Schedule.queue(
> +        (function() {

Nit: name this function
Attachment #608741 - Flags: review?(mnoorenberghe+bmo)
Attachment #608741 - Flags: review?(gavin.sharp)
Attachment #608741 - Flags: feedback+
Comment on attachment 608742 [details] [diff] [review]
Clients of nsSearchService

Review of attachment 608742 [details] [diff] [review]:
-----------------------------------------------------------------

::: browser/base/content/nsContextMenu.js
@@ +1282,5 @@
> +      // If the search service is not initialized yet, there is not
> +      // much we can do.
> +
> +      ss.init();
> +      return false;

Gavin: I think that this silent return may hide a bug in the future.  I suggested in comment 25 that reportError be used before the return.  What do you think?

::: browser/base/content/safeMode.js
@@ +105,5 @@
>  function restoreDefaultSearchEngines() {
>    var searchService = Components.classes["@mozilla.org/browser/search-service;1"]
>                                  .getService(Components.interfaces.nsIBrowserSearchService);
> +  searchService.init(function restoreDefaultSearchEngines_cb(status) {
> +          if (status == Components.results.NS_OK) {

Nit: use an "a" prefix for the argument name => aStatus

@@ +107,5 @@
>                                  .getService(Components.interfaces.nsIBrowserSearchService);
> +  searchService.init(function restoreDefaultSearchEngines_cb(status) {
> +          if (status == Components.results.NS_OK) {
> +              searchService.restoreDefaultEngines();
> +          }

Indent the |if| two spaces from the left of |searchService.init| like the style in |disableAddons|.

::: browser/components/nsBrowserContentHandler.js
@@ +319,5 @@
>                       .getService(nsIBrowserSearchService);
> +  ss.init(function doSearch_cb(aResult) {
> +            if (aResult != Components.results.NS_OK) {
> +              return;
> +            }

Please try to indent with the prevailing style of the file.  The function body should be indented (like it was) from the left-most non-space character on the line |function| is on.

@@ +342,3 @@
>  
> +            var wwatch = Components.classes["@mozilla.org/embedcomp/window-watcher;1"]
> +              .getService(nsIWindowWatcher);

Nit: Don't forget to re-align the periods on the 3 getService/createInstance calls once you fix the indentation.

@@ +345,4 @@
>  
> +            wwatch.openWindow(null, gBrowserContentHandler.chromeURL,
> +                              "_blank",
> +                              "chrome,dialog=no,all" +

It's possible that extensions were relying on the return value here so maybe we should put the addon-compat keyword on the bug? From looking through some results on AMO MXR (with many false positives) and based on the fact this is a function for the command-line I don't expect many extension callers.

::: browser/components/search/content/search.xml
@@ +120,5 @@
>  
>            this._addedObserver = true;
> +
> +          this.searchService.init((function search_init_cb(status) {
> +             if (status == Components.results.NS_OK) {

Nit: "a" prefix on args

::: toolkit/components/search/nsSearchSuggestions.js
@@ +458,5 @@
>      this.stopSearch();
>  
> +    searchService.init((function startSearch_cb(aResult) {
> +        if (aResult != Cr.NS_OK) {
> +          Cu.reportError("nsSearchSuggestion.js: Could not init search service. Bailing out");

Nit: IIRC, reportError includes the path to the file which reported the error so it's probably redundant in the message.
Attachment #608742 - Flags: review?(mnoorenberghe+bmo)
Attachment #608742 - Flags: review?(gavin.sharp)
Attachment #608742 - Flags: feedback+
Comment on attachment 608741 [details] [diff] [review]
Asynchronous API

>diff --git a/toolkit/components/search/nsSearchService.js b/toolkit/components/search/nsSearchService.js

IIRC xpconnect won't prevent setting of searchService.isInitialize despite it being marked readonly, so you need to do that yourself (by making it a getter).

> SearchService.prototype = {

>+  init: function SRCH_SVC__init(aObserver) {
>+    let initialization = this._initializer;

This local variable makes the code harder to read rather than easier, I think (and there's no real perf difference) - just omit it and use this._initializer directly?

>+      this._initializer = initialization = Schedule.queue(
>+        (function() {

As Matt mentioned, I would prefer if you used

Schedule.queue(function () {
  // ...
}.bind(this));

style.

>+          this._loadEngines();

You should catch and report an exception here explicitly (as with the old code) rather than using trap (we don't want the error reporting to depend on having a non-null aObserver).

Not related to this bug, but in general, I'm not sure I like the idea of "trap" - it makes error handling very non-obvious.

Shouldn't the value passed to aObserver depend on this.isInitialized, rather than always being NS_OK? If someone calls init() again after initialization failed the first time, they shouldn't get NS_OK.

I'll r+ with these addressed.
Attachment #608741 - Flags: review?(gavin.sharp)
re: interface name, let's go with nsIBrowserSearchInitObserver
Comment on attachment 608744 [details] [diff] [review]
Companion testsuite

Review of attachment 608744 [details] [diff] [review]:
-----------------------------------------------------------------

::: toolkit/components/search/tests/xpcshell/test_645970.js
@@ +49,5 @@
>    // The search service needs to be started after the jarURIs pref has been
>    // set in order to initiate it correctly
> +  Services.search.init(function search_initialized(status) {
> +      do_check_eq(status, Components.utils.NS_OK);
> +      let engine = Services.search.getEngineByName("bug645970");

Nit: fix indentation and use "a" prefix on function arguments.

::: toolkit/components/search/tests/xpcshell/test_migratedb.js
@@ +24,3 @@
>  
> +  afterCommit(function commit_complete() {
> +                //Check that search-metadata.json has been created

Nit: Fix indentation of both functions.

::: toolkit/components/search/tests/xpcshell/test_nodb.js
@@ +22,5 @@
>    do_test_pending();
> +  search.init(function ss_initialized() {
> +                do_timeout(500, function() {
> +                             // Check that search-metadata.json has not been
> +                             // created. Note that we cannot du much better

Nit: Re-indent please. Do you mind fixing the existing typo :s/du/do/ since you're changing the line anyways?

::: toolkit/components/search/tests/xpcshell/test_nodb_pluschanges.js
@@ +33,5 @@
>  
>    let search = Services.search;
>  
>    function observer(aSubject, aTopic, aData) {
> +    do_print("Observer called "+aTopic +", "+aData);

Nit: If you're going to leave this in then please put spaces around the operators.

@@ +81,5 @@
>  
>    do_test_pending();
>  
> +  search.init(
> +    function(status) {

Nits: name the function, move to same line as init, use the "a" prefix on the argument, & fix indentation of body.
Attachment #608744 - Flags: review?(mnoorenberghe+bmo)
Attachment #608744 - Flags: review?(gavin.sharp)
Attachment #608744 - Flags: feedback+
Comment on attachment 608742 [details] [diff] [review]
Clients of nsSearchService

>diff --git a/browser/base/content/nsContextMenu.js b/browser/base/content/nsContextMenu.js

>+    if (!ss.isInitialized) {

Yes, I think we should reportError here (this should never happen in practice; I think we'll always try to ensure the search service gets inited shortly after startup, even if we remove its other users like the search bar).

>diff --git a/browser/base/content/safeMode.js b/browser/base/content/safeMode.js

> function restoreDefaultSearchEngines() {

>+          if (status == Components.results.NS_OK) {

Components.isSuccessCode(status) (here and throughout these patches)

It's a bit of a shame to initialize the search service just to re-set its engines, but I guess that's necessary for now. We could investigate making this cheaper, but I guess it's not a common use case.

>diff --git a/browser/components/nsBrowserContentHandler.js b/browser/components/nsBrowserContentHandler.js

>+  ss.init(function doSearch_cb(aResult) {
>+            if (aResult != Components.results.NS_OK) {

Use isSuccessCode here too, and don't indent the whole function several levels:

ss.init(function doSearch_cb(aResult) {
  // ...
});
 
>diff --git a/browser/components/search/content/search.xml b/browser/components/search/content/search.xml

Does this change impact the browser UI visibly during startup? I imagine in some extreme cases it could result in the search bar being unusable shortly after startup - what happens when you try to use it in that situation? Do we need to add code to handle that better?

>diff --git a/docshell/base/nsDefaultURIFixup.cpp b/docshell/base/nsDefaultURIFixup.cpp

>+        nsresult rv = searchSvc->GetIsInitialized(&isSearchInitialized);
>+        if (!NS_SUCCEEDED(rv)) {

NS_FAILED (here and elsewhere in this patch)

>+            rv = searchSvc->Init(NULL);

nsnull... though I'm not sure it makes sense to trigger initialization here (it won't help with this particular invocation anyways).

>diff --git a/toolkit/components/search/nsSearchSuggestions.js b/toolkit/components/search/nsSearchSuggestions.js

>+    searchService.init((function startSearch_cb(aResult) {

I think it would help readability a lot if you moved the code into a separate callback function, so you could do:

searchService.init(function searchcallback(result) {
  if (Components.isSuccessCode(result))
    this._triggerSearch();
}.bind(this));

Though it may not be ideal to wait for another spin through the event loop before starting the search in the common case... perhaps you should have it check for isInitialized and trigger the search immediately in that case.
Attachment #608742 - Flags: review?(gavin.sharp)
(In reply to Matthew N. [:MattN] from comment #28)
> ::: browser/components/nsBrowserContentHandler.js
> @@ +345,4 @@
> >  
> > +            wwatch.openWindow(null, gBrowserContentHandler.chromeURL,
> > +                              "_blank",
> > +                              "chrome,dialog=no,all" +
> 
> It's possible that extensions were relying on the return value here so maybe
> we should put the addon-compat keyword on the bug? From looking through some
> results on AMO MXR (with many false positives) and based on the fact this is
> a function for the command-line I don't expect many extension callers.

Good point.
Keywords: addon-compat
(In reply to Gavin Sharp (use gavin@gavinsharp.com for email) from comment #29)
> >+          this._loadEngines();
> 
> You should catch and report an exception here explicitly (as with the old
> code) rather than using trap (we don't want the error reporting to depend on
> having a non-null aObserver).

I will do that, but, just to be clear, I would like to emphasize that |Schedule| reports untrapped errors.

> Not related to this bug, but in general, I'm not sure I like the idea of
> "trap" - it makes error handling very non-obvious.

In this case, |trap| may be overkill. However, the next step of this refactoring of |nsSearchService| is to make loading actually asynchronous, at which stage Error will simply stop being an option and we have to turn to actual asynchronous error handling. I am not aware of any technique for asynchronous error handling that behaves better than this form of |trap|, but if you know of any, I am all ears.

> Shouldn't the value passed to aObserver depend on this.isInitialized, rather
> than always being NS_OK? If someone calls init() again after initialization
> failed the first time, they shouldn't get NS_OK.

So do you think that we should report (with your previous comments addressed) |loadEngines| or |addObservers| failing to the caller? This was not the case until now, so I attempted to replicate that behavior.

> I'll r+ with these addressed.
(In reply to Gavin Sharp (use gavin@gavinsharp.com for email) from comment #32)
> >diff --git a/browser/base/content/safeMode.js b/browser/base/content/safeMode.js
> 
> It's a bit of a shame to initialize the search service just to re-set its
> engines, but I guess that's necessary for now. We could investigate making
> this cheaper, but I guess it's not a common use case.

> >diff --git a/browser/components/search/content/search.xml b/browser/components/search/content/search.xml
> 
> Does this change impact the browser UI visibly during startup? I imagine in
> some extreme cases it could result in the search bar being unusable shortly
> after startup - what happens when you try to use it in that situation? Do we
> need to add code to handle that better?

I _think_ that there may be a brief flicker while images get loaded, but I have never met a situation in which I could attempt to use the search bar before the service is loaded. However, I have broken the search service a few times during experiments and the search bar seems to degrade nicely: it simply displays no search engines.

> nsnull... though I'm not sure it makes sense to trigger initialization here
> (it won't help with this particular invocation anyways).

The idea is that we cannot always rely on the search box to be loaded (this was your suggestion, iirc). So, if the search box is not loaded, someone has to take the initiative to initialize the service.


> Though it may not be ideal to wait for another spin through the event loop
> before starting the search in the common case... perhaps you should have it
> check for isInitialized and trigger the search immediately in that case.

ok.
Attachment #608744 - Attachment is obsolete: true
Attachment #608744 - Flags: review?(gavin.sharp)
Attachment #608742 - Attachment is obsolete: true
Attachment #608741 - Attachment is obsolete: true
Attachment #609264 - Flags: review?(mnoorenberghe+bmo)
Attachment #609265 - Flags: review?(mnoorenberghe+bmo)
Attachment #609266 - Flags: review?(mnoorenberghe+bmo)
Comment on attachment 609266 [details] [diff] [review]
Asynchronous API

Review of attachment 609266 [details] [diff] [review]:
-----------------------------------------------------------------

+ from me with the NS_OK change.

::: netwerk/base/public/nsIBrowserSearchService.idl
@@ +398,5 @@
> +/**
> + * Callback for asynchronous initialization of nsIBrowserSearchService
> + */
> +[scriptable, function, uuid(02256156-16e4-47f1-9979-76ff98ceb590)]
> +interface nsIBrowserSearchInitObserver: nsISupports

Nit (for the 3rd time): Please put a space before the colon.

::: toolkit/components/search/nsSearchService.js
@@ +2583,5 @@
> +    if (aObserver) {
> +      // Trigger callback.
> +      // Note that we effectively ignore any error in the callback itself.
> +      this._initializer.queue(
> +        aObserver.onInitComplete.bind(aObserver, Components.results.NS_OK)

I agree with Gavin that this shouldn't blindly send NS_OK if !this._isInitialized.
Attachment #609266 - Flags: review?(mnoorenberghe+bmo) → feedback+
Attachment #609266 - Attachment is obsolete: true
(In reply to Matthew N. [:MattN] from comment #39)
> Comment on attachment 609266 [details] [diff] [review]
> Asynchronous API
> 
> Review of attachment 609266 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> + from me with the NS_OK change.
> 
> ::: netwerk/base/public/nsIBrowserSearchService.idl
> @@ +398,5 @@
> > +/**
> > + * Callback for asynchronous initialization of nsIBrowserSearchService
> > + */
> > +[scriptable, function, uuid(02256156-16e4-47f1-9979-76ff98ceb590)]
> > +interface nsIBrowserSearchInitObserver: nsISupports
> 
> Nit (for the 3rd time): Please put a space before the colon.
Sorry about that. Now fixed.

> 
> ::: toolkit/components/search/nsSearchService.js
> @@ +2583,5 @@
> > +    if (aObserver) {
> > +      // Trigger callback.
> > +      // Note that we effectively ignore any error in the callback itself.
> > +      this._initializer.queue(
> > +        aObserver.onInitComplete.bind(aObserver, Components.results.NS_OK)
> 
> I agree with Gavin that this shouldn't blindly send NS_OK if
> !this._isInitialized.

Done. Note, however, that at the moment, this is dead code. Afaict, there is nothing that can trigger this behavior.
Comment on attachment 609265 [details] [diff] [review]
Clients of nsSearchService

Review of attachment 609265 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good to me.

::: browser/base/content/safeMode.js
@@ +108,5 @@
> +  searchService.init(function restoreDefaultSearchEngines_cb(aStatus) {
> +     if (Components.isSuccessCode(aStatus)) {
> +         searchService.restoreDefaultEngines();
> +     }
> +   });

Nit: There is still an extra space on these 4 lines.

::: browser/components/nsBrowserContentHandler.js
@@ +319,5 @@
>                       .getService(nsIBrowserSearchService);
> +  ss.init(function doSearch_cb(aResult) {
> +     if (!Components.isSuccessCode(aResult)) {
> +       return;
> +     }

Nit: Use 2 spaces to indent, not 3.

@@ +330,3 @@
>  
> +     var wuri = Components.classes["@mozilla.org/supports-string;1"]
> +       .createInstance(Components.interfaces.nsISupportsString);

Nit (again): Don't forget to re-align the periods on the 3 getService/createInstance calls once you fix the indentation.

::: browser/components/search/content/search.xml
@@ +120,5 @@
>  
>            this._addedObserver = true;
> +
> +          this.searchService.init((function search_init_cb(aStatus) {
> +             if (Components.isSuccessCode(aStatus)) {

Nit: 2 spaces here too.

::: toolkit/components/search/nsSearchSuggestions.js
@@ +497,5 @@
>      }
>  
>      // Actually do the search
>      this._request = Cc["@mozilla.org/xmlextras/xmlhttprequest;1"].
> +      createInstance(Ci.nsIXMLHttpRequest);

Nit: Revert the indentation of createInstance here too.
Attachment #609265 - Flags: review?(mnoorenberghe+bmo) → feedback+
Attachment #609498 - Flags: feedback+
Attachment #609264 - Flags: review?(mnoorenberghe+bmo) → feedback+
Yoric, by the way, to cut down on bug spam you can mark a patch as obsolete when you are attaching the new one.
OS: Mac OS X → All
Hardware: x86 → All
Attachment #609265 - Attachment is obsolete: true
(In reply to Matthew N. [:MattN] from comment #43)
> Yoric, by the way, to cut down on bug spam you can mark a patch as obsolete
> when you are attaching the new one.

Unfortunately, bzexport doesn't always succeed at that.
Attachment #609264 - Flags: review?(gavin.sharp)
Attachment #609498 - Flags: review?(gavin.sharp)
Attachment #609676 - Flags: review?(gavin.sharp)
I just attached a first draft of the next step: make actual loading of metadata asynchronous.
Attachment #609498 - Flags: review?(gavin.sharp) → review+
Attachment #609498 - Attachment is obsolete: true
Comment on attachment 609676 [details] [diff] [review]
Clients of nsSearchService

Sorry that I'm wavering back and forth about whether we should treat the search service being uninitialized as exceptional. It's hard to decide whether we should bother trying to make the search service be "optional" (i.e. not initialized itself unless needed). This patch seems to be missing many users of the search service in browser.js, which access it as Services.search. Looking at all of the callers that expect it to be initialized (context menu, location bar command handling, etc.), I'm starting to think that it makes more sense to just always eagerly initialize the search service at some point relatively soon after startup, and then treat any cases where it isn't initialized as errors that will hopefully never occur in practice.

>diff --git a/browser/base/content/nsContextMenu.js b/browser/base/content/nsContextMenu.js

>+    if (!ss.isInitialized) {
>+      // If the search service is not initialized yet, there is not
>+      // much we can do.
>+
>+      Components.utils.reportError("Search service not initialized yet");

This needs to provide a little bit more context (the filename will be included by reportError, but that's not very obvious - at least mention the function where this error is occurring).

>diff --git a/browser/base/content/safeMode.js b/browser/base/content/safeMode.js

> function restoreDefaultSearchEngines() {

>+  searchService.init(function restoreDefaultSearchEngines_cb(aStatus) {
>+    if (Components.isSuccessCode(aStatus)) {
>+      searchService.restoreDefaultEngines();
>+    }
>+  });

Hmm, this function is called shortly before a restart. I don't see anything that ensures that the callback is called before we restart.

>diff --git a/docshell/base/nsDefaultURIFixup.cpp b/docshell/base/nsDefaultURIFixup.cpp

This is an example of a caller where it's not really acceptable for the search service to be unavailable (we can't really just have the search fail). It's also not really feasible to refactor this API to deal with being async :(

>diff --git a/toolkit/components/search/nsSearchSuggestions.js b/toolkit/components/search/nsSearchSuggestions.js

>+  _triggerSearch: function(searchService) {

>     if (!searchString ||

This variable seems to be undefined in this function (along with "searchParam")

>-    var self = this;
>-    function onReadyStateChange() {
>-      self.onReadyStateChange();
>-    }
>-    this._request.onreadystatechange = onReadyStateChange;
>+    this._request.onreadystatechange = this.onReadyStateChange;

Don't you need a .bind(this)? Though this change doesn't seem necessary for this patch.
Attachment #609676 - Flags: review?(gavin.sharp) → review-
Attachment #609264 - Flags: review?(gavin.sharp) → review+
Whiteboard: [Snappy]
(In reply to Gavin Sharp (use gavin@gavinsharp.com for email) from comment #49)
> >diff --git a/docshell/base/nsDefaultURIFixup.cpp b/docshell/base/nsDefaultURIFixup.cpp
> 
> This is an example of a caller where it's not really acceptable for the
> search service to be unavailable (we can't really just have the search
> fail). It's also not really feasible to refactor this API to deal with being
> async :(

I suspect that it could not fail in usual runs (because we do not run a search before having shown the search bar) but yes, if would fail if some add-on and/or a configuration in which we perform keyword searches without having shown the search bar could get it to fail or somehow initialized the search service.

Which brings us back to the matter of deciding _when_ to initialize. Do you have a suggestion for a component that would be loaded early enough that we could be sure that initialization has taken place?

Right now, the best possibility I see is in the constructor for nsDefaultURIFixup: http://mxr.mozilla.org/mozilla-central/source/docshell/base/nsDefaultURIFixup.cpp#63

What do you think?
Gavin, ping.
Whiteboard: [Snappy] → [Snappy:P1]
Ok, we just synchronized on IRC: I will try within the constructor.
Attached patch Variant (obsolete) — Splinter Review
I have just altered the search service methods with a few changes:
- isInitialized can now return |true| (if initialization is complete), |false| (if initialization is not started or in progress) or throw an error (if initialization has failed) - consequently, this is now a method rather than an attribute;
- all methods and attributes of nsIBrowserSearch either assert that initialization has been completed or fallback to asynchronous if initialization is incomplete.

Gavin, can you tell me what you think of these changes?
Attachment #616553 - Flags: review?(gavin.sharp)
Attached patch Clients of nsSearchService (obsolete) — Splinter Review
I have applied the strategy discussed on IRC (launch initialization of the search service early, in particular in the constructor of nsURLFixup), and extended it to all clients (I think) of nsISearchService.
Attachment #609676 - Attachment is obsolete: true
Attachment #616557 - Flags: review?(gavin.sharp)
Gavin, I am currently running my changes through TryServer and attempting to get it to pass. I should be able to get there eventually, but it occurs to me that it might be time to reevaluate my original strategy, as there is a chance that this strategy might save us lots of time. 

For reference, in that strategy (suggested by mak), I had two initializers, one asynchronous and one synchronous. Any call to a method of the service would first check whether initialization was complete. If not, it would stop the asynchronous initializer, print a warning, and launch the synchronous one instead.

What do you think of this?
Attaching a synchronous fallback for the asynchronous API. If this passes tests, this should be something that we can push soon, without having to fear for API clients, tests or add-ons, and that we can progressively tune, one client at a time.

This is a trivial (for the moment) implementation of the following design:
- |init| performs asynchronous initialization;
- whenever |init| is about to start initialization of a subsystem, it first checks whether that subsystem has somehow already been initialized – if so, asynchronous initialization stops;
- if a public API is called before |init| is complete, it calls |syncInit|;
- |syncInit| performs synchronous initialization, taking over from the last subsystem that |init| has successfully initialized.

For the moment, the implementation is trivial, as there are only two steps in the initialization, but I believe that this should scale up to more steps.
Attachment #610046 - Attachment is obsolete: true
Attachment #620226 - Flags: review?(gavin.sharp)
With a few fixes, this patch seems to work. In DEBUG mode, it logs any circumstance in which we trigger the synchronous fallback, which should let us proceed progressively for the following steps.
Attachment #620226 - Attachment is obsolete: true
Attachment #620226 - Flags: review?(gavin.sharp)
Attachment #620732 - Flags: review?(gavin.sharp)
Gavin, I haven't heard from you, so I have proceeded a little in that direction. Here's the result.
Attachment #621619 - Flags: feedback?(gavin.sharp)
Comment on attachment 620732 [details] [diff] [review]
Asynchronous API, with synchronous fallback

Switching reviews from Gavin to Matt.
Attachment #620732 - Flags: review?(gavin.sharp) → review?(mnoorenberghe+bmo)
Attachment #616553 - Flags: review?(gavin.sharp)
Attachment #621619 - Flags: feedback?(gavin.sharp) → feedback?(mnoorenberghe+bmo)
Attachment #616557 - Flags: review?(gavin.sharp)
(re-summarizing this bug to more accurately reflect its main goal)
Summary: nsSearchService metadata should not be synchronous → make nsIBrowserSearchService API async
@Gavin
Not completely accurate: this bug was only about the metadata part. See also bug 699856 for loading engines.
This is a tweaked version of your latest approach, which doesn't depend on the Schedule API. What do you think?
Attachment #609264 - Attachment is obsolete: true
Attachment #609704 - Attachment is obsolete: true
Attachment #616553 - Attachment is obsolete: true
Attachment #616557 - Attachment is obsolete: true
Attachment #620732 - Attachment is obsolete: true
Attachment #621619 - Attachment is obsolete: true
Attachment #620732 - Flags: review?(mnoorenberghe+bmo)
Attachment #621619 - Flags: feedback?(mnoorenberghe+bmo)
Attachment #626151 - Flags: review?(dteller)
Attachment #626151 - Flags: feedback?(mnoorenberghe+bmo)
(In reply to David Rajchenbach Teller [:Yoric] from comment #61)
> Not completely accurate: this bug was only about the metadata part. See also
> bug 699856 for loading engines.

I was just going based on what the patches here did. Is that the wrong bug #?
Comment on attachment 626151 [details] [diff] [review]
add async init() method, keep sync initialization fallback

Review of attachment 626151 [details] [diff] [review]:
-----------------------------------------------------------------

I believe that this works, if the objective of the bug is now solely to alter the API to make it asynchronous. The code will not scale to actual asynchronous implementation, but we can certainly move asynchronous implementation to another bug.

See also my remarks on error-handling and logging.

::: toolkit/components/search/nsSearchService.js
@@ +2521,5 @@
> +      this._loadEngines();
> +    } catch (ex) {
> +      this._initRV = Cr.NS_ERROR_FAILURE;
> +      LOG("_init: failure loading engines: " + ex);
> +    }

The original algorithm (before going asynchronous) completely ignores failures of |this._loadEngines()|, besides logging. If you think that we should now make it an error reported to listeners, I have no issue, but be aware that this changes the semantics.

@@ +2529,5 @@
> +
> +    // Notify all of the init observers
> +    this._initObservers.forEach(function (observer) {
> +      observer.onInitComplete(this._initRV);
> +    }, this);

This will break if |onInitComplete| raises an error. I believe that we should either |executeSoon| or box this in a |try| block.

@@ +2530,5 @@
> +    // Notify all of the init observers
> +    this._initObservers.forEach(function (observer) {
> +      observer.onInitComplete(this._initRV);
> +    }, this);
> +    this._initObservers = [];

We could even do |this._initObservers = null|.

@@ +2535,5 @@
> +  },
> +
> +  get isInitialized() {
> +    return gInitialized;
> +  },

With this mechanism, any initialization error will go completely unreported when the service is used synchronously and/or if there is no callback. Right now, this is not an issue, as we have no errors. However, your previous instructions (which I believe were right) were to prepare for a setting in which we could effectively have errors.

In such cases, I believe that we want methods that force initialization through a call to |_syncInit| to throw of initialization fails or has failed due to an error in a previous run.

@@ +3208,5 @@
>    },
>  
>    // nsIBrowserSearchService
>    getEngines: function SRCH_SVC_getEngines(aCount) {
> +    this._syncInit();

My version introduced an intermediate method |ensureInitialized|, to log cases in which we have to use the synchronous fallback. The objective is to let us progressively locate and fix API clients. I suggest keeping this feature.
Attachment #626151 - Flags: review?(dteller)
(In reply to :Gavin Sharp (use gavin@gavinsharp.com for email) from comment #63)
> (In reply to David Rajchenbach Teller [:Yoric] from comment #61)
> > Not completely accurate: this bug was only about the metadata part. See also
> > bug 699856 for loading engines.
> 
> I was just going based on what the patches here did. Is that the wrong bug #?

Sorry about that, I misunderstood that you wanted an async implementation of the API, rather than just an async API.
(In reply to David Rajchenbach Teller [:Yoric] from comment #65)
> Sorry about that, I misunderstood that you wanted an async implementation of
> the API, rather than just an async API.

I do want the async implementation, but I figured it would be best to do this in steps. I'd like to understand what you mean by "The code will not scale to actual asynchronous implementation"... Once something like this patch lands, my plan was for the next steps to be:
1) Converting callers to calling init() appropriately (as you mention adding a warning would be useful for this)
2) Actually implementing async initialization (leaving in _syncInit, but making the async initialization path use async IO, etc.)

We can work on both of those in parallel. The idea is to eventually remove the sync fallback, but I'm not yet sure how feasible that is in the near term. If you think 2) will somehow be problematic with the approach I've taken in this patch, let me know.
(In reply to :Gavin Sharp (use gavin@gavinsharp.com for email) from comment #66)
> (In reply to David Rajchenbach Teller [:Yoric] from comment #65)
> > Sorry about that, I misunderstood that you wanted an async implementation of
> > the API, rather than just an async API.
> 
> I do want the async implementation, but I figured it would be best to do
> this in steps.

Certainly – you may recall this is what I was doing.

Just mentioning that if you change the scope of the bug, we should split this bug into:
- one bug for the asynchronous API with the mostly-synchronous implementation;
- one or more bugs for the clients (I tend to believe that we want at least search.xml to be adapted immediately, while others are less urgent);
- one bug for loading metadata asynchronously;
- one bug for loading engines asynchronously;
- perhaps more.

> I'd like to understand what you mean by "The code will not
> scale to actual asynchronous implementation"... Once something like this
> patch lands, my plan was for the next steps to be:
> 1) Converting callers to calling init() appropriately (as you mention adding
> a warning would be useful for this)
> 2) Actually implementing async initialization (leaving in _syncInit, but
> making the async initialization path use async IO, etc.)
>
> We can work on both of those in parallel. The idea is to eventually remove
> the sync fallback, but I'm not yet sure how feasible that is in the near
> term. If you think 2) will somehow be problematic with the approach I've
> taken in this patch, let me know.

Well, there is nothing that can't be fixed. However, I was reacting to the fact that you had removed the support I had started to introduce for the next stage, in which we actually perform asynchronous loading with synchronous fallback. 

In my view, this feature requires:
- error-handling;
- a loader cleanly cut in steps;
- a marker recording which is the latest step that we have completed asynchronously, so as to avoid replicating the work if we fallback to the synchronous initializer;
- a marker recording whether loading has failed, so as to avoid the fallback repeatedly re-initializing everything (and failing).

I had started to introduce such support in the patches that you have obsoleted.

Also, recall that I have already converted a number of callers. Now, given that this bug advances by short and distant bursts, I am not completely sure I remember in which patch I have done this, but I believe it is also in one of the patches that you have just obsoleted.
I'm sorry if obsoleting the patches sent the wrong message - I certainly wasn't intending to imply that the work done in them wasn't useful. But I think that trying to combine too many tasks into one patch (or one bug) is one of the reasons we've had a hard time making progress here. So let's take smaller steps, and actually get some things landed. It looks to me like we can land my modified patch once your comments have been addressed - let's focus on that.
(In reply to David Rajchenbach Teller [:Yoric] from comment #64)
> The original algorithm (before going asynchronous) completely ignores
> failures of |this._loadEngines()|, besides logging. If you think that we
> should now make it an error reported to listeners, I have no issue, but be
> aware that this changes the semantics.

Yeah, I am aware of this. _loadEngines failures are exceptional, and would previously cause pretty much any use of the search service to throw an exception. Now that we're introducing an async initialization API with a callback, I figured we might as well propagate that status to that callback.

> This will break if |onInitComplete| raises an error. I believe that we
> should either |executeSoon| or box this in a |try| block.

Yeah, need to wrap this in a try{}, good catch.

> With this mechanism, any initialization error will go completely unreported
> when the service is used synchronously and/or if there is no callback. Right
> now, this is not an issue, as we have no errors. However, your previous
> instructions (which I believe were right) were to prepare for a setting in
> which we could effectively have errors.

I don't think errors are going to be any more or less likely to occur in the future, we're just changing how they manifest themselves. I'm assuming isInitialized's purpose is to answer "do I need to call init()?" - it's not really useful to answer "yes, call init" only to have that call certainly end in failure (instead, the exception will be thrown when you then attempt to make use of the service). But it doesn't really matter to me either way, given that errors should be infrequent.

> My version introduced an intermediate method |ensureInitialized|, to log
> cases in which we have to use the synchronous fallback. The objective is to
> let us progressively locate and fix API clients. I suggest keeping this
> feature.

Yes, this would be good.
(In reply to :Gavin Sharp (use gavin@gavinsharp.com for email) from comment #70)
> (In reply to David Rajchenbach Teller [:Yoric] from comment #64)
> > The original algorithm (before going asynchronous) completely ignores
> > failures of |this._loadEngines()|, besides logging. If you think that we
> > should now make it an error reported to listeners, I have no issue, but be
> > aware that this changes the semantics.
> 
> Yeah, I am aware of this. _loadEngines failures are exceptional, and would
> previously cause pretty much any use of the search service to throw an
> exception. Now that we're introducing an async initialization API with a
> callback, I figured we might as well propagate that status to that callback.

Sounds good.

> > With this mechanism, any initialization error will go completely unreported
> > when the service is used synchronously and/or if there is no callback. Right
> > now, this is not an issue, as we have no errors. However, your previous
> > instructions (which I believe were right) were to prepare for a setting in
> > which we could effectively have errors.
> 
> I don't think errors are going to be any more or less likely to occur in the
> future, we're just changing how they manifest themselves. I'm assuming
> isInitialized's purpose is to answer "do I need to call init()?" - it's not
> really useful to answer "yes, call init" only to have that call certainly
> end in failure (instead, the exception will be thrown when you then attempt
> to make use of the service). But it doesn't really matter to me either way,
> given that errors should be infrequent.

Then let's make |ensureInitialized| report the error, if for some reason the initialization could not complete.
By the way, Gavin, it is my understanding that you intend to write the chunk of code. If you want me to pick the ball, please mention it.
Attachment #626151 - Attachment is obsolete: true
Attachment #626151 - Flags: feedback?(mnoorenberghe+bmo)
Attachment #628343 - Flags: review?(gavin.sharp)
Attachment #628344 - Flags: review?(gavin.sharp)
Attached patch Companion testsuite (obsolete) — Splinter Review
I have adapted most tests to the async API, but maybe we want to duplicate each test, i.e. have one async version and one sync fallback version.
Attachment #628347 - Flags: review?(gavin.sharp)
Comment on attachment 628343 [details] [diff] [review]
Making nsIBrowserSearchService API asynchronous, with a synchronous fallback

A couple of tweaks that I've made:
- there were some redundant checks added - I feel like these make the code harder to reason about because it suggests that some states are possible when they aren't. !Components.isSuccessCode(this._initRV) can only be true if gInitialized is true, so there's no need to check one after the other. Similarly, _syncInit should never be called if gInitialized is true, so there's no need to check that (it's easy to audit both callers).
- I moved the two new method implementations from nsIBrowserSearchService lower in the file to group them with the rest of the methods from that interface
- your comment about "kill any asynchronous initialization in progress" made me realized that my patch failed to actually address that case (_syncInit() being called while there is an async initialization task pending). Since there's no easy way to cancel a pending task, I made the async task check the initialization state before it calls _syncInit.
- I removed an unnecessary comment for "init" - it's pretty self explanatory and there is documentation in the IDL

I'll attach a patch with these changes. I still want to make sure that the tests we're adding cover the various initialization paths completely (async init, async init followed by sync init, sync init only, multiple concurrent async initializations, etc.). If you could help with that, that'd be great.
Attachment #628343 - Flags: review?(gavin.sharp)
Comment on attachment 628347 [details] [diff] [review]
Companion testsuite

You should probably add an "rv" parameter to the init callbacks you're adding, since people have a tendency to copy test code. Maybe even check its value in the tests. Otherwise this looks good, but I haven't verified that it's complete (do other tests also need changing?), and as mentioned, I think we need to significantly expand the test coverage to cover the various scenarios handled by the combined sync/async init paths.
Attachment #628347 - Flags: review?(gavin.sharp) → review+
Comment on attachment 628344 [details] [diff] [review]
Adapting main clients to async API

can we move this step to a separate bug?
Attachment #628344 - Attachment is obsolete: true
Attachment #628344 - Flags: review?(gavin.sharp)
Comment on attachment 628519 [details] [diff] [review]
Making nsIBrowserSearchService API asynchronous, with a synchronous fallback

Review of attachment 628519 [details] [diff] [review]:
-----------------------------------------------------------------

Indeed, we are starting to see the mess for which I was introducing these |initSteps|.
I believe that we are good for the moment, except for the issue underlined below.

::: toolkit/components/search/nsSearchService.js
@@ +2498,5 @@
> +  _ensureInitialized: function() {
> +    if (gInitialized) {
> +      return;
> +    }
> +

There is still a problem: if |_ensureInitialized| is called after |_syncInit| has been completed with a failure, it will not throw. We should check |Components.isSuccessCode(this._initRV)| before returning.
Attachment #628519 - Flags: feedback?(dteller) → feedback+
(In reply to :Gavin Sharp (use gavin@gavinsharp.com for email) from comment #76)
> I'll attach a patch with these changes. I still want to make sure that the
> tests we're adding cover the various initialization paths completely (async
> init, async init followed by sync init, sync init only, multiple concurrent
> async initializations, etc.). If you could help with that, that'd be great.

I will work on it.
(In reply to :Gavin Sharp (use gavin@gavinsharp.com for email) from comment #79)
> Comment on attachment 628344 [details] [diff] [review]
> Adapting main clients to async API
> 
> can we move this step to a separate bug?

ok,
No longer depends on: JSScheduleAPI
(In reply to :Gavin Sharp (use gavin@gavinsharp.com for email) from comment #76)
> Comment on attachment 628343 [details] [diff] [review]
> Making nsIBrowserSearchService API asynchronous, with a synchronous fallback
> 
> A couple of tweaks that I've made:
> - there were some redundant checks added - I feel like these make the code
> harder to reason about because it suggests that some states are possible
> when they aren't. !Components.isSuccessCode(this._initRV) can only be true
> if gInitialized is true, so there's no need to check one after the other.

Right, I had misunderstood the current semantics of |gInitialized|.

> Similarly, _syncInit should never be called if gInitialized is true, so
> there's no need to check that (it's easy to audit both callers).

[...]

> - your comment about "kill any asynchronous initialization in progress" made
> me realized that my patch failed to actually address that case (_syncInit()
> being called while there is an async initialization task pending). Since
> there's no easy way to cancel a pending task, I made the async task check
> the initialization state before it calls _syncInit.

Sure, but isn't that the check you just removed from |_syncInit|?

> - I removed an unnecessary comment for "init" - it's pretty self explanatory
> and there is documentation in the IDL

ok
Same patch, with the following changes:
- |_ensureInitialized| should now be able to throw in case of failure, regardless of the path taken;
- once initialization has succeeded (which should be the common case), |_ensureInitialized| is equivalent to a noop – in this version, after its first trivial call, |_ensureInitialized| replaces itself with a noop implementation;
- I have added a few names to otherwise unnamed functions;
- small typo fix in |init|.
Attachment #628519 - Attachment is obsolete: true
Attachment #628663 - Flags: review?(gavin.sharp)
Attached patch Companion testsuite (obsolete) — Splinter Review
Adding a few items to the testsuite:
- initializing asynchronously before proceeding;
- initializing asynchronously but proceeding without waiting for async initialization to complete.

In both cases, I launch several asynchronous initializers and ensure that they all call back with a success.

I could not add any failure test, though: it looks like every single error raised is caught and logged + ignored.
Attachment #628347 - Attachment is obsolete: true
Attachment #628695 - Flags: review?(gavin.sharp)
Comment on attachment 628663 [details] [diff] [review]
Making nsIBrowserSearchService API asynchronous, with a synchronous fallback

(In reply to David Rajchenbach Teller [:Yoric] from comment #83)
> Sure, but isn't that the check you just removed from |_syncInit|?

Yes indeed - I guess I got a bit confused about this. In any case, I'm satisfied with how the patch turned out :)

> - once initialization has succeeded (which should be the common case),
> |_ensureInitialized| is equivalent to a noop

Not sure I like this - it means the failure case varies between the first failure (we throw _initRV) and subsequent ones (some other code throws because e.g. this._engines is null). Can you just remove this bit?

r=me with that changed.
Attachment #628663 - Flags: review?(gavin.sharp) → review+
Comment on attachment 628695 [details] [diff] [review]
Companion testsuite

I think you should remove the loadFromJars/jarURIs stuff from the initialization tests, it's confusing to test two different "features" in the same test.

r=me with that change.
Attachment #628695 - Flags: review?(gavin.sharp) → review+
(In reply to :Gavin Sharp (use gavin@gavinsharp.com for email) from comment #86)
> > - once initialization has succeeded (which should be the common case),
> > |_ensureInitialized| is equivalent to a noop
> 
> Not sure I like this - it means the failure case varies between the first
> failure (we throw _initRV) and subsequent ones (some other code throws
> because e.g. this._engines is null). Can you just remove this bit?

It is a bit too irregular, I admit, and I will remove it for a first version. I believe that something along these lines will be useful for to performance, though, to avoid killing JIT-ability with that call to |Components.isSuccessCode|.
(In reply to :Gavin Sharp (use gavin@gavinsharp.com for email) from comment #87)
> Comment on attachment 628695 [details] [diff] [review]
> Companion testsuite
> 
> I think you should remove the loadFromJars/jarURIs stuff from the
> initialization tests, it's confusing to test two different "features" in the
> same test.
> 
> r=me with that change.

I believe that using a custom search engine is the right thing to do, to avoid a test dependency on the specifics of search engines packaged with a given version of Firefox.

As far as I understand the initialization of this service, to do it sychronously, I have to either:
- put that search engine in a JAR and use loadFromJars/jarURIs stuff;
- somehow customize NS_APP_SEARCH_DIR_LIST from the test;
- or add to nsSearchService.js a new preference to override the use of NS_APP_SEARCH_DIR_LIST and use that preference in the test.

I have the impression that the least confusing combination is the one I have used. What do you think?
(In reply to David Rajchenbach Teller [:Yoric] from comment #88)
> It is a bit too irregular, I admit, and I will remove it for a first
> version. I believe that something along these lines will be useful for to
> performance, though, to avoid killing JIT-ability with that call to
> |Components.isSuccessCode|.

Ah, yeah, I forgot about that call. Not a big deal though, isSuccessCode is hardly a difficult computation :)

(In reply to David Rajchenbach Teller [:Yoric] from comment #89)
> > I think you should remove the loadFromJars/jarURIs stuff from the
> > initialization tests, it's confusing to test two different "features" in the
> > same test.
> > 
> > r=me with that change.
> 
> I believe that using a custom search engine is the right thing to do, to
> avoid a test dependency on the specifics of search engines packaged with a
> given version of Firefox.

But you're not testing the loading of a specific engine, just that we're initializing the service as a whole. Setting the JAR URI prefs doesn't stop us from loading the other engines, it just adds to the list of places we look, so there's no benefit to doing it in these tests.
Attachment #628695 - Attachment is obsolete: true
Attachment #630158 - Flags: review?(gavin.sharp)
(In reply to :Gavin Sharp (use gavin@gavinsharp.com for email) from comment #90)
> (In reply to David Rajchenbach Teller [:Yoric] from comment #88)
> > It is a bit too irregular, I admit, and I will remove it for a first
> > version. I believe that something along these lines will be useful for to
> > performance, though, to avoid killing JIT-ability with that call to
> > |Components.isSuccessCode|.
> 
> Ah, yeah, I forgot about that call. Not a big deal though, isSuccessCode is
> hardly a difficult computation :)

Fair enough :)

> (In reply to David Rajchenbach Teller [:Yoric] from comment #89)
> > > I think you should remove the loadFromJars/jarURIs stuff from the
> > > initialization tests, it's confusing to test two different "features" in the
> > > same test.
> > > 
> > > r=me with that change.
> > 
> > I believe that using a custom search engine is the right thing to do, to
> > avoid a test dependency on the specifics of search engines packaged with a
> > given version of Firefox.
> 
> But you're not testing the loading of a specific engine, just that we're
> initializing the service as a whole. Setting the JAR URI prefs doesn't stop
> us from loading the other engines, it just adds to the list of places we
> look, so there's no benefit to doing it in these tests.

Good point. I now only test that getEngines returns non-null.
Attachment #630158 - Flags: review?(gavin.sharp) → review+
https://hg.mozilla.org/integration/mozilla-inbound/rev/9463584a3d69
https://hg.mozilla.org/integration/mozilla-inbound/rev/9487eb14320b
Flags: in-testsuite+
Keywords: checkin-needed
Summary: make nsIBrowserSearchService API async → add async initialization API to nsIBrowserSearchService
Target Milestone: --- → Firefox 16
I don't think this will cause any actual addon-compat issues, given that we've implemented fallback to sync initialization. C++ addons may be affected by the IID bump, but I imagine those are rare or non-existent.

We certainly should get developer documentation updated to recommend use of the async initialization technique, though (we'll be moving our internal consumers in bug 760035).
Keywords: dev-doc-needed
https://hg.mozilla.org/mozilla-central/rev/9463584a3d69
https://hg.mozilla.org/mozilla-central/rev/9487eb14320b

(Merged by Ed Morley)
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Blocks: 780179
I'm in a lose-lose situation, I'm being called synchronously and I need to use the search service :-(
You have a couple of choices:
1) don't get called synchronously (I know this can get complicated)
2) make sure the search service is initialized before you do

Which specific case are you referring to?
I've actually got two cases (so far... I hope I don't get any more):
1. nsContextMenu calls initMenu calls isTextSelection calls ss.<engine>.name which initialises the search service synchronously. (I guess nsContextMenu could just force init on load...)
2. One of SeaMonkey's pref panes populates a menulist when it loads, allowing preferences.xml to select the correct item depending on the saved preference value.
This code added a debug message into a release build:

+    let warning = "Search service falling back to synchronous initialization at " + new Error().stack;
+    Components.utils.reportError(warning);
+    LOG(warning);
(In reply to Michael Kaply (mkaply) from comment #101)
> This code added a debug message into a release build:
> 
> +    let warning = "Search service falling back to synchronous
> initialization at " + new Error().stack;
> +    Components.utils.reportError(warning);
> +    LOG(warning);

Is this a problem?
I think mkaply is suggesting that we're spamming stdout in release builds (which would be undesirable), but LOG() doesn't do that unless you set the browser.search.log pref. Spamming the error console isn't that big of a deal.
> Is this a problem?
> Spamming the error console isn't that big of a deal.

I'd agree if this was a genuine failure or exception.

But this is just a warning, and processing continues, and it can happen to a user (it happened to me).

Throwing a call stack into the Error console on a warning seems overkill.

Is there a reason this needs to be on the Error console? Do you want to know about it when it happens?
Well, we want bug reports so that we can fix these issues, or report them to add-on authors if they are using deprecated code. Also, we would like add-on authors to receive this error message if their add-on contain deprecated code.

If this is an issue, of course, we can limit this to debug builds.
(In reply to David Rajchenbach Teller [:Yoric] from comment #105)
> Well, we want bug reports so that we can fix these issues, or report them to
> add-on authors if they are using deprecated code. Also, we would like add-on
> authors to receive this error message if their add-on contain deprecated
> code.

Then the error message needs to say something to that effect.

I didn't realize this was a problem that needed to be reported, and I certainly got no indication from the failure or the error message that it was a problem with my add-on. My add-on was not even in the call stack.

If we are deprecating or changing interfaces and we need to make add-on authors aware, then we need better messages.
Blocks: 1482579
You need to log in before you can comment on or make changes to this bug.