1227045 - The default search engine is not kept after selecting the "restore default search engines"

Status: VERIFIED FIXED

(Firefox :: Search, defect, P1)

Description

[Simona Badau, Desktop QA]

Mozilla/5.0 (Windows NT 6.1; Win64; x64; rv:45.0) Gecko/20100101 Firefox/45.0
Build ID: 20151122030230

Steps to reproduce:
1. Launch Firefox
2. Go to about:preferences#search and set Amazon as the default engine
3. In the One-click search engines list -> select Amazon -> click the "Remove" button
4. Click on the "Restore Default Search Engines" button 

Expected Results:
Amazon is placed back in the "One Click search engines" list. Google remains the default search engine.

Actual Results:
Amazon is placed back in the "One Click search engines" list. Google remains the default search engine until Firefox is restarted. After the restart, Amazon is placed back as being the default search engine.

Comment 1

[Florian Quèze [:florian]]

Changes being lost after a restart is bad, we should fix sooner than later.

Comment 2

[Florian Quèze [:florian]]

Attachment: Use the currentEngine setter

When the current engine is removed, we correctly avoid using the currentEngine setter and set _currentEngine to null. This avoids generating plenty of notifications when several engines are removed at once. However, when the currentEngine getter is called and has to select a new current engine, we should call the setter so that this new current engine is persisted across restarts.

Comment 3

[Marco Bonardo [:mak]]

Comment on attachment 8691418 [details] [diff] [review]
Use the currentEngine setter

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

I agree this should be fixed sooner than later...
Though this is crying for an automated test, even in a follow-up bug, if it's complicate to add one here. We should really add one, supposed we can fake the restart action.

::: toolkit/components/search/nsSearchService.js
@@ +4092,4 @@
>      }
>  
> +    if (currentEngine)
> +      this.currentEngine = currentEngine;

While it's clear from the bug explanation, having the getter invoke the setter will look exotic to a third party.
Would you mind bracing the if and adding a brief comment inside it explaining why we are doing this?

I was about to suggest factoring out the part of set currentEngine() that persist the change, then I thought here we are basically redefining a new currentEngine, thus it should always make sense to just invoke the setter.

Comment 4

[Florian Quèze [:florian]]

Attachment: Patch v2

Addressed comment 3. I took this as an opportunity to also add test coverage for the resetToOriginalDefaultEngine method. It was added in bug 1075157 and is already covered by a test in browser/components/selfsupport, but now that we are also using this method in the search preferences UI (since bug 1119872), I prefer it to also be covered by the search service's set of tests.

Comment 5

[Florian Quèze [:florian]]

Attachment: Patch v3

Fixed the browser/components/search/test/browser_addEngine.js test failure.

Comment 6

[Florian Quèze [:florian]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=7d8a541ec991

Comment 7

[Florian Quèze [:florian]]

Comment on attachment 8692536 [details] [diff] [review]
Patch v3

This patch is unfortunately regressing bug 1220246 (test_geodefaults.js intermittently failing on Windows 7 debug, as shown by https://treeherder.mozilla.org/#/jobs?repo=try&revision=21ea6f247bd4 ; the bc failures there are already fixed).

On the plus side, it's possible to consistently reproduce the failure locally on all platforms by applying attachment 8683652 [details] [diff] [review].

Comment 8

[Florian Quèze [:florian]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=67a96839cd2a

Comment 9

[Florian Quèze [:florian]]

Attachment: fix intermittent failures

(In reply to Florian Quèze [:florian] [:flo] from comment #7)
> Comment on attachment 8692536 [details] [diff] [review]
> Patch v3
>
> This patch is unfortunately regressing bug 1220246 (test_geodefaults.js
> intermittently failing on Windows 7 debug, as shown by
> https://treeherder.mozilla.org/#/jobs?repo=try&revision=21ea6f247bd4 ; the
> bc failures there are already fixed).

Attaching the test fix separately for easier review. Try (comment 8) is green :-).

There where 2 problems with the tests:
- a broken assumption about how re-initialization of the search service works, that caused the cache file to not always be flushed to disk before a restart of the search service.
- there's one case where if the server takes more than 1000ms (the delay before the cache is flushed to disk) to return a result, the test fails if we don't wait for a second flush that actually saves the data returned by the test server.

Due to asyncReInit now always waiting for the cache flush, creating the promiseAfterCache promise before calling asyncReInit no longer makes sense, so I could do some simplifications in the test.
I had to add a new uninit-complete notification to handle the case of a test relying on a synchronous re-initialization of the search service.

Comment 10

[Marco Bonardo [:mak]]

Comment on attachment 8694683 [details] [diff] [review]
fix intermittent failures

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

::: toolkit/components/search/nsSearchService.js
@@ +3085,4 @@
>      }.bind(this));
>    },
>  
>    _asyncReInit: function () {

OT: I'm not thrilled by the fact the search service has wrappedJSObject that allows to call into _asyncReInit. This allows add-ons to put hands into the service... wrappedJSObject is almost always the wrong thing to do.

@@ +3093,1 @@
>      Task.spawn(function* () {

what about making
_asyncReInit: Task.async(function* () {
  ...

@@ +3110,5 @@
> +
> +        // Tests that want to force a synchronous re-initialization need to
> +        // be notified when we are done uninitializing.
> +        Services.obs.notifyObservers(null, SEARCH_SERVICE_TOPIC,
> +                                     "uninit-complete");

I'm not sure why we couldn't use the Task promise to wait for reinit to be complete instead. By using Task.async (or return Task.spawn) you'd have a clean promise to use everywhere you need to wait.

The only "defect" is that consumers of _asyncReInit that don't care about the promise should
..._asyncReInit().catch(Cu.reportError);
but that would be a positive consequence adding cleanliness and proper error handling.
http://mxr.mozilla.org/mozilla-central/search?string=_asyncReInit

That would also allow to remove "reinit-failed" and "reinit-complete" (looks like the only consumer could just wait for the _asyncReInit promise).

Comment 11

[Marco Bonardo [:mak]]

Comment on attachment 8694683 [details] [diff] [review]
fix intermittent failures

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

Ok, I was confused by the bogus usage on Android, sorry.

::: toolkit/components/search/tests/xpcshell/test_geodefaults.js
@@ +149,2 @@
>  
>    // Synchronously check the current default engine, to force a sync init.

could you please expand this comment to explain why we don't want async reinit to complete but we instead force a sync init?

Comment 12

[Florian Quèze [:florian]]

https://hg.mozilla.org/integration/fx-team/rev/3294bc6d07c44017e6b7a307f6f97052dda42d9c
Bug 1227045 - Save the new current engine when it has been set as a fallback after the previous current engine was removed, r=mak.

https://hg.mozilla.org/integration/fx-team/rev/2d33fa3346b2e4bfd7720a9f1f19e4a59b84e143
Bug 1227045 - fix intermittent test failures due to the cache file not being consistently saved before test-triggered restarts, r=mak.

Comment 13

[Florian Quèze [:florian]]

(In reply to Marco Bonardo [::mak] from comment #11)
> Review of attachment 8694683 [details] [diff] [review]:

Thanks for the review!

> ::: toolkit/components/search/tests/xpcshell/test_geodefaults.js
> @@ +149,2 @@
> >  
> >    // Synchronously check the current default engine, to force a sync init.
> 
> could you please expand this comment to explain why we don't want async
> reinit to complete but we instead force a sync init?

There's not enough context in the diff to see it, but there's a comment explaining why we need a sync init 10 lines above in the file.

Comment 14

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/3294bc6d07c4
https://hg.mozilla.org/mozilla-central/rev/2d33fa3346b2

Comment 15

[Simona Badau, Desktop QA]

Mozilla/5.0 (X11; Linux i686; rv:45.0) Gecko/20100101 Firefox/45.0
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:45.0) Gecko/20100101 Firefox/45.0
Mozilla/5.0 (Macintosh; Intel Mac OS X 10.10; rv:45.0) Gecko/20100101 Firefox/45.0 
Build ID: 20151210030212

Verified as fixed on the latest Nightly 45.0a1 version - the default search engine (Google) is kept after selecting the option to "restore default search engines" from about:preferences#search and Firefox is restarted.