1274545 - User-set keyword and ordering of search engine lost on engine update

Status: VERIFIED FIXED

(Firefox :: Search, defect)

Description

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

This is similar to bug 1238335. It might have started in Firefox 45, but I still experience it in Firefox 46.0.2 (64 bit) on Windows 8.1.

Soon after the update to Firefox 45.0, I reinstalled some search engines (had lost them with the initial update, fixed it with files from the backup and 'upgrading' again, no idea anymore why I did this).

I visited https://support.mozilla.org/de/ and installed the search engine from the search bar, then opened Firefox's search options and added a keyword ('sumo'). This works, but after some time the keyword is gone. This isn't after every restart and the keywords for other built-in and installed search engines remain.

Comment 1

[Florian Quèze [:florian]]

Would be very useful if you could identify steps to reproduce.

Comment 2

[Ian Moody [:Kwan] (UTC+1)]

This has been bugging me periodically the last few months, wiping out MDN and DXR keyword searches.  Haven't figured out any STR.  Neither adding new searches, restarting the browser, nor crashing it seem to do it. Going to keep an eye on it.  Wondering if upgrades have something to do with it.

Comment 3

[monk-time]

I have started experiencing this issue regularly after the update to 45: all newly added engines (MDN and DevDocs) and one pre-45 engine reset their position and alias roughly once a week. I have 7 default engines hidden, 44 engines enabled, and only 3/44 behave in that way.

Can't reproduce it either. Last time I noticed it today. Using mozlz4a.py [1] on search.json.mozlz4 I can see that there are 21 engines with _metaData.updateexpir set to one week from now, but only the three faulty engines among them have _metaData.updatelastmodified set to yesterday (the rest to March), so it seems to be related to weekly engine updates. Unfortunately, I can't figure out how to trigger such an update, changing updateexpir for DevDocs to 2000-01-01 (946684800000) and packing the file back with the tool has no effect on anything.

Is there any other way to trigger that weekly engine update check?

[1] https://gist.github.com/Tblue/62ff47bef7f894e92ed5

Comment 4

[Florian Quèze [:florian]]

(In reply to monk-time from comment #3)
> I can see that there are 21 engines with
> _metaData.updateexpir set to one week from now, but only the three faulty
> engines among them have _metaData.updatelastmodified set to yesterday (the
> rest to March), so it seems to be related to weekly engine updates.

Very interesting, thanks!

Could you give me URLs from where I can install (at least one of) the 3 affected plugins?

> Is there any other way to trigger that weekly engine update check?

Changing your system's clock may do it?

Comment 5

[Florian Quèze [:florian]]

(In reply to Florian Quèze [:florian] [:flo] from comment #4)

> Could you give me URLs from where I can install (at least one of) the 3
> affected plugins?

Ah, I just saw you said MDN and DevDocs, so I assume:
https://developer.mozilla.org/en-US/search/xml
and
http://devdocs.io/opensearch.xml

Do the "21 engines with _metaData.updateexpir set to one week from now" have an update URL? Could you give an example of such plugin?

Comment 6

[monk-time]

Update: 4 hours after I changed devdocs' metadata to 2000-01-01, I minimized the browser and went to sleep, and 10 minutes after that Firefox modified search.json.mozlz4. Now this engine is missing its alias, moved to the end of the file, has updatelastmodified set to the file's mtime, and updateexpir set to a week from that:

  "updatelastmodified": "Thu, 07 Jul 2016 02:56:38 GMT",
  "updateexpir": 1468464998655
  
Florian: I'm not sure what counts as an update URL, but almost all of 21 (including three affected) have an "application/opensearchdescription+xml" url with "rels": ["self"]. I have sent you an e-mail with my search.json.mozlz4.

Comment 7

[Ian Moody [:Kwan] (UTC+1)]

Attachment: manualEngineUpdate.patch

Update definitely seems to be what blows the alias away, initiating a manual one using a function added by the attached patch is enough to kill it.  Also got a simple fix incoming.

Comment 8

[Ian Moody [:Kwan] (UTC+1)]

Attachment: Bug 1274545 - Rework the search engine _onLoad code to group update-only code together, and preserve user's metaData on update.

Review commit: https://reviewboard.mozilla.org/r/62834/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/62834/

Comment 9

[Ian Moody [:Kwan] (UTC+1)]

Comment on attachment 8768782 [details]
Bug 1274545 - Rework the search engine _onLoad code to group update-only code together, and preserve user's metaData on update.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/62834/diff/1-2/

Comment 10

[monk-time]

Ian, thanks for the fix!
Does it also cover engines losing their position in the list (that a user can set by dragging them in about:preferences#search) seemingly also after updates? For me usually both an alias and order (_metaData.order in search.json.mozlz4) disappear at the same time. Or should it be a separate bug?

Comment 11

[Ian Moody [:Kwan] (UTC+1)]

(In reply to monk-time from comment #10)
> Ian, thanks for the fix!
> Does it also cover engines losing their position in the list (that a user
> can set by dragging them in about:preferences#search) seemingly also after
> updates? For me usually both an alias and order (_metaData.order in
> search.json.mozlz4) disappear at the same time. Or should it be a separate
> bug?
No it doesn't but I think you've helped explain something that was puzzling me about this bug: why it's only the last engine that loses its keyword.  The answer is it isn't, but it loses its order at the same time, which dumps it at the end of the list.  Order fix should be simple though, and I think could ride along.

Comment 12

[Ian Moody [:Kwan] (UTC+1)]

Comment on attachment 8768782 [details]
Bug 1274545 - Rework the search engine _onLoad code to group update-only code together, and preserve user's metaData on update.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/62834/diff/2-3/

Comment 13

[Florian Quèze [:florian]]

Comment on attachment 8768782 [details]
Bug 1274545 - Rework the search engine _onLoad code to group update-only code together, and preserve user's metaData on update.

https://reviewboard.mozilla.org/r/62834/#review60024

Thank you very much for debugging this and providing a patch!

The following review comments will make the patch less simple if you address them, so I would understand if you don't want to go this way and would prefer me to take over the bug. I also think we really need to get this code covered by a test. Currently there's no test at all for the update feature, as we were wondering if we should just remove it completely (see bug 1259510).

Given that this bug has potential to cause issues with the loadPathHash values, I would like to uplift the fix at least to Firefox 49, where we have bug 1203168 that relies on it.

::: toolkit/components/search/nsSearchService.js:1626
(Diff revision 3)
>      if (!aBytes) {
>        promptError();
>        return;
>      }
>  
>      var engineToUpdate = null;

All the code in this method related to updates in split in several chunks which make it a fragile mess. I think this bug is a good opportunity to consolidate it in a single place.

::: toolkit/components/search/nsSearchService.js:1634
(Diff revision 3)
>  
>        // Make this new engine use the old engine's shortName,
>        // to preserve user-set metadata.
>        aEngine._shortName = engineToUpdate._shortName;
> +      // preserve alias and order
> +      aEngine.alias = engineToUpdate.alias;

Are you intentionally calling the alias setter (it sends a notification) rather than directly setting the attribute?

::: toolkit/components/search/nsSearchService.js:1655
(Diff revision 3)
>        return;
>      }
>  
>      // Check that when adding a new engine (e.g., not updating an
>      // existing one), a duplicate engine does not already exist.
>      if (!engineToUpdate) {

I think the 3 line related to defining the 'engineToUpdate' should move down to here; the first place where it is used.

::: toolkit/components/search/nsSearchService.js:1685
(Diff revision 3)
>          return;
>        }
>        aEngine._useNow = confirmation.useNow;
>      }
>  
>      // If we don't yet have a shortName, get one now. We would already have one

This is where I think we should regroup all the update/non-update specific code.

::: toolkit/components/search/nsSearchService.js:1688
(Diff revision 3)
>      }
>  
>      // If we don't yet have a shortName, get one now. We would already have one
>      // if this is an update and _file was set above.
>      if (!aEngine._shortName)
>        aEngine._shortName = sanitizeName(aEngine.name);

Looks like this only make sense for new engines.

::: toolkit/components/search/nsSearchService.js:1690
(Diff revision 3)
>      // If we don't yet have a shortName, get one now. We would already have one
>      // if this is an update and _file was set above.
>      if (!aEngine._shortName)
>        aEngine._shortName = sanitizeName(aEngine.name);
>  
>      aEngine._loadPath = aEngine.getAnonymizedLoadPath(null, aEngine._uri);

I think we only want this for new engines, and should preserve engineToUpdate._loadPath for the update case.

::: toolkit/components/search/nsSearchService.js:1691
(Diff revision 3)
>      // if this is an update and _file was set above.
>      if (!aEngine._shortName)
>        aEngine._shortName = sanitizeName(aEngine.name);
>  
>      aEngine._loadPath = aEngine.getAnonymizedLoadPath(null, aEngine._uri);
>      aEngine.setAttr("loadPathHash", getVerificationHash(aEngine._loadPath));

I also think this should only be done for new engines. For updated engines, I think we should transfer all the existing metadata (not specifically "alias" or "order", but all of it).

Comment 14

[Ian Moody [:Kwan] (UTC+1)]

Wanted to find out exactly when this started to know which releases are affected.  Some manual bisecting of the the revs listed on https://hg.mozilla.org/mozilla-central/log/tip/toolkit/components/search/nsSearchService.js gives:

The first bad revision is:
changeset:   270315:bba9634ab696
user:        Florian Quèze <[e-mail]>
date:        Fri Oct 30 09:54:52 2015 +0100
summary:     Bug 1203167 - store metadata in the main JSON file and remove the engine metadata service, r=adw.

Comment 15

[Ian Moody [:Kwan] (UTC+1)]

https://reviewboard.mozilla.org/r/62834/#review60024

I'm certainly willing to give it a go. Regarding the test, that was definitely something I was thinking of, but I don't know how one would write one without a method to force an update, ala my manualEngineUpdate patch.  Is there some extant way that I'm missing?

> Are you intentionally calling the alias setter (it sends a notification) rather than directly setting the attribute?

That was an artifact of finding that method first (in the browser console) when I was only updating the alias, before monk-time mentioned the order loss.  Hadn't clocked the notification difference, will switch to setAttr for both cases.

> I also think this should only be done for new engines. For updated engines, I think we should transfer all the existing metadata (not specifically "alias" or "order", but all of it).

Is there a way to get all metadata easily that I'm not seeing, or will it have to be a manual setAttr(getAttr()) for each one?

Comment 16

[Ian Moody [:Kwan] (UTC+1)]

https://reviewboard.mozilla.org/r/62834/#review60024

> Is there a way to get all metadata easily that I'm not seeing, or will it have to be a manual setAttr(getAttr()) for each one?

Okay, nevermind, ._metaData = ._metaData works fine.  I swear I'd tried that before and it hadn't worked with unable to access. Maybe a capitalisation typo. Ah, or probably in the original version where I'd put the code elsewhere and the original engine wasn't .wrappedJSObjected.

Comment 17

[Florian Quèze [:florian]]

(In reply to Ian Moody [:Kwan] from comment #15)
> https://reviewboard.mozilla.org/r/62834/#review60024
> 
> I'm certainly willing to give it a go. Regarding the test, that was
> definitely something I was thinking of, but I don't know how one would write
> one without a method to force an update, ala my manualEngineUpdate patch. 
> Is there some extant way that I'm missing?

You can tweak the "updateexpir" values (using setAttr) so that the next update should happen now, and then call the timer callback with something like:
Service.search.QueryInterface(Ci.nsITimerCallback).notify()

> > I also think this should only be done for new engines. For updated engines, I think we should transfer all the existing metadata (not specifically "alias" or "order", but all of it).
> 
> Is there a way to get all metadata easily that I'm not seeing, or will it
> have to be a manual setAttr(getAttr()) for each one?

I was thinking of iterating over the content of engineToUpdate._metaData

Comment 18

[Florian Quèze [:florian]]

(In reply to Ian Moody [:Kwan] from comment #16)
> https://reviewboard.mozilla.org/r/62834/#review60024
> 
> > Is there a way to get all metadata easily that I'm not seeing, or will it have to be a manual setAttr(getAttr()) for each one?
> 
> Okay, nevermind, ._metaData = ._metaData works fine.

Well, that makes both aEngine and engineToUpdate refer to the same metaData object. That's probably undesirable, unless we are completely sure one of the two engine updates will be GCed soon.

Comment 19

[Florian Quèze [:florian]]

(In reply to Florian Quèze [:florian] [:flo] from comment #18)

> object. That's probably undesirable, unless we are completely sure one of
> the two engine updates will be GCed soon.

read "engine objects" not "engine updates" here.

Comment 20

[Ian Moody [:Kwan] (UTC+1)]

https://reviewboard.mozilla.org/r/62834/#review60024

> All the code in this method related to updates in split in several chunks which make it a fragile mess. I think this bug is a good opportunity to consolidate it in a single place.

Done.

> That was an artifact of finding that method first (in the browser console) when I was only updating the alias, before monk-time mentioned the order loss.  Hadn't clocked the notification difference, will switch to setAttr for both cases.

or just copy the metaData wholesale, as mentioned below.

> I think the 3 line related to defining the 'engineToUpdate' should move down to here; the first place where it is used.

Done.

> This is where I think we should regroup all the update/non-update specific code.

Done.

> Looks like this only make sense for new engines.

Done.

> I think we only want this for new engines, and should preserve engineToUpdate._loadPath for the update case.

Done.

Comment 21

[Ian Moody [:Kwan] (UTC+1)]

Comment on attachment 8768782 [details]
Bug 1274545 - Rework the search engine _onLoad code to group update-only code together, and preserve user's metaData on update.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/62834/diff/3-4/

Comment 22

[Florian Quèze [:florian]]

Comment on attachment 8768782 [details]
Bug 1274545 - Rework the search engine _onLoad code to group update-only code together, and preserve user's metaData on update.

https://reviewboard.mozilla.org/r/62834/#review60320

Thanks for the updated patch! Only 1 comment to address and I'll r+ the code change; are you planning to work on a test?

::: toolkit/components/search/nsSearchService.js:1645
(Diff revision 4)
> -
>      // If requested, confirm the addition now that we have the title.
>      // This property is only ever true for engines added via
>      // nsIBrowserSearchService::addEngine.
>      if (aEngine._confirm) {
>        var confirmation = aEngine._confirmAddEngine();

It seems to me that this can only happen for new engines, and that it's better to move this code after the prompt/early return for the case of attempting to add a duplicate, so that the user doesn't see 2 modal dialogs in a row. ie. preserve the order of this code

Comment 23

[Ian Moody [:Kwan] (UTC+1)]

https://reviewboard.mozilla.org/r/62834/#review60320

Thanks for the review!  I have indeed been working on a test, just been taking me a while to find my way around xpcshell tests and search events/observers, and find the free time.  Have one ready now though, just with one slightly awkward bit I'm not sure of.

> It seems to me that this can only happen for new engines, and that it's better to move this code after the prompt/early return for the case of attempting to add a duplicate, so that the user doesn't see 2 modal dialogs in a row. ie. preserve the order of this code

Done.

Comment 24

[Ian Moody [:Kwan] (UTC+1)]

Attachment: Bug 1274545 - Add a test for user-set keyword & order preservation on search engine update.

Review commit: https://reviewboard.mozilla.org/r/63678/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/63678/

Comment 25

[Ian Moody [:Kwan] (UTC+1)]

Comment on attachment 8768782 [details]
Bug 1274545 - Rework the search engine _onLoad code to group update-only code together, and preserve user's metaData on update.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/62834/diff/4-5/

Comment 26

[Ian Moody [:Kwan] (UTC+1)]

https://reviewboard.mozilla.org/r/63678/#review60608

::: toolkit/components/search/tests/xpcshell/test_engineUpdate.js:40
(Diff revision 1)
> +
> +  yield new Promise(resolve => {
> +    let observed = 0;
> +    Services.obs.addObserver(function obs(subject, topic, data) {
> +      if (data == "engine-changed") {
> +        // we need to wait for the second changed event

This is the bit I'm unsure of, it seems fragile.

Comment 27

[Florian Quèze [:florian]]

Comment on attachment 8768782 [details]
Bug 1274545 - Rework the search engine _onLoad code to group update-only code together, and preserve user's metaData on update.

https://reviewboard.mozilla.org/r/62834/#review60620

Thanks!

Comment 28

[Florian Quèze [:florian]]

Comment on attachment 8770101 [details]
Bug 1274545 - Add a test for user-set keyword & order preservation on search engine update.

https://reviewboard.mozilla.org/r/63678/#review60646

Thank you so much for writing a test! :-)

::: toolkit/components/search/tests/xpcshell/test_engineUpdate.js:12
(Diff revision 1)
> +
> +function run_test() {
> +  updateAppInfo();
> +  useHttpServer();
> +
> +  do_register_cleanup(() => {

A new profile/process is created to run each xpcshell test, so I don't think we need this.

::: toolkit/components/search/tests/xpcshell/test_engineUpdate.js:40
(Diff revision 1)
> +
> +  yield new Promise(resolve => {
> +    let observed = 0;
> +    Services.obs.addObserver(function obs(subject, topic, data) {
> +      if (data == "engine-changed") {
> +        // we need to wait for the second changed event

Indeed this seems fragile. I think I would prefer waiting first for engine-loaded fired by the _onLoad method and then for engine-changed fired by _addEngineToStore, with comments indicating this.

Note: waiting for just engine-loaded seems enough for the test to pass, so maybe we don't need to bother waiting for _addEngineToStore to finish...

If you do want to wait for both notifications, I would suggest using a helper like http://searchfox.org/mozilla-central/source/toolkit/components/search/tests/xpcshell/head_search.js#501
(either by copying it to your test, or by adding an optional parameter so that it can observer engine notifications)

::: toolkit/components/search/tests/xpcshell/test_engineUpdate.js:43
(Diff revision 1)
> +    Services.obs.addObserver(function obs(subject, topic, data) {
> +      if (data == "engine-changed") {
> +        // we need to wait for the second changed event
> +        if (++observed == 2) {
> +          let engine = subject.QueryInterface(Ci.nsISearchEngine);
> +          equal(engine.alias, KEYWORD, "Keyword not cleared by update");

Should the test also verify that _shortName and _loadPath are preserved after the update?

It's a bit unfortunate that the update URL is the same URL we initially installed the plugin from, as the loadPath and loadPathHash would be identical even if the update code didn't preserve them.

::: toolkit/components/search/tests/xpcshell/test_engineUpdate.js:52
(Diff revision 1)
> +      }
> +    }, TOPIC, false);
> +
> +    // set last update to 8 days ago, since the default interval is 7, then
> +    // trigger an update
> +    engine.wrappedJSObject.setAttr("updateexpir", Date.now() - (1000 * 60 * 60 * 24 * 8));

This would be more reable with:
const ONE_DAY_IN_MS = 24 * 60 * 60 * 1000;

Comment 29

[Florian Quèze [:florian]]

[Tracking Requested - why for this release]: regression in 45; I would be more comfortable shipping bug 1203168 (which landed for 49) if this was fixed at the same time or before. I didn't request tracking until we had steps to reproduce, but this is something I would have wanted to fix in 46 if it had been possible at the time.

Comment 30

[Ian Moody [:Kwan] (UTC+1)]

https://reviewboard.mozilla.org/r/63678/#review60646

> A new profile/process is created to run each xpcshell test, so I don't think we need this.

Removed.

> Indeed this seems fragile. I think I would prefer waiting first for engine-loaded fired by the _onLoad method and then for engine-changed fired by _addEngineToStore, with comments indicating this.
> 
> Note: waiting for just engine-loaded seems enough for the test to pass, so maybe we don't need to bother waiting for _addEngineToStore to finish...
> 
> If you do want to wait for both notifications, I would suggest using a helper like http://searchfox.org/mozilla-central/source/toolkit/components/search/tests/xpcshell/head_search.js#501
> (either by copying it to your test, or by adding an optional parameter so that it can observer engine notifications)

>Note: waiting for just engine-loaded seems enough for the test to pass, so maybe we don't need to bother waiting for \_addEngineToStore to finish...

Huh, thought I'd looked at the value of alias at engine-loaded and found it lacking, but you're right, it passes, which of course makes sense given it's fired just after where I put the fix.  Switching to that.

> Should the test also verify that _shortName and _loadPath are preserved after the update?
> 
> It's a bit unfortunate that the update URL is the same URL we initially installed the plugin from, as the loadPath and loadPathHash would be identical even if the update code didn't preserve them.

\_shortName isn't user-set right?  I'm not sure if it's worth testing or not.
I was thinking of also testing if order is preserved, shall I do so?

Also I've realised with the \_loadPath preservation we now don't support a search engine changing it's updateURL, whereas before we did (I think).  Is that desired?

(Also are xpcshell tests supposed to immediately bail on first test failure, rather than continuing and checking for more failures/passes?)

> This would be more reable with:
> const ONE_DAY_IN_MS = 24 * 60 * 60 * 1000;

Done.

Comment 31

[Florian Quèze [:florian]]

(In reply to Ian Moody [:Kwan] from comment #30)

> > Should the test also verify that _shortName and _loadPath are preserved after the update?
> > 
> > It's a bit unfortunate that the update URL is the same URL we initially installed the plugin from, as the loadPath and loadPathHash would be identical even if the update code didn't preserve them.
> 
> \_shortName isn't user-set right?  I'm not sure if it's worth testing or not.
> I was thinking of also testing if order is preserved, shall I do so?

Wouldn't hurt to do so :-).

> Also I've realised with the \_loadPath preservation we now don't support a
> search engine changing it's updateURL, whereas before we did (I think).  Is
> that desired?

I don't see how the _loadPath relates to the updateURL. The loadPath is used mostly for Telemetry (see bug 1164159).

> (Also are xpcshell tests supposed to immediately bail on first test failure,
> rather than continuing and checking for more failures/passes?)

That's the behavior I've always seen, even though it's not always what I would prefer.

Comment 32

[Ian Moody [:Kwan] (UTC+1)]

(In reply to Florian Quèze [:florian] [:flo] from comment #31)
> (In reply to Ian Moody [:Kwan] from comment #30)
> 
> > > Should the test also verify that _shortName and _loadPath are preserved after the update?
> > > 
> > > It's a bit unfortunate that the update URL is the same URL we initially installed the plugin from, as the loadPath and loadPathHash would be identical even if the update code didn't preserve them.
> > 
> > \_shortName isn't user-set right?  I'm not sure if it's worth testing or not.
> > I was thinking of also testing if order is preserved, shall I do so?
> 
> Wouldn't hurt to do so :-).
Cool, added that.

> > Also I've realised with the \_loadPath preservation we now don't support a
> > search engine changing it's updateURL, whereas before we did (I think).  Is
> > that desired?
> 
> I don't see how the _loadPath relates to the updateURL. The loadPath is used
> mostly for Telemetry (see bug 1164159).
Oh, sorry, I misread the code, never mind then.

> > (Also are xpcshell tests supposed to immediately bail on first test failure,
> > rather than continuing and checking for more failures/passes?)
> 
> That's the behavior I've always seen, even though it's not always what I
> would prefer.
Ah okay, thanks for the confirmation.  Found it confusing when I was trying out testing order preservation earlier and checking if it failed pre-fix.

Comment 33

[Ian Moody [:Kwan] (UTC+1)]

Comment on attachment 8768782 [details]
Bug 1274545 - Rework the search engine _onLoad code to group update-only code together, and preserve user's metaData on update.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/62834/diff/5-6/

Comment 34

[Ian Moody [:Kwan] (UTC+1)]

Comment on attachment 8770101 [details]
Bug 1274545 - Add a test for user-set keyword & order preservation on search engine update.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/63678/diff/1-2/

Comment 35

[Ian Moody [:Kwan] (UTC+1)]

STR for QA reference:
1. Go to https://developer.mozilla.org
2. Install the search plugin
3. Go to the Search Settings page and give the plugin a keyword of "mdn" (optionally drag it to change the order)
4a. Open the Dev Tools and in the settings check "Ënable browser chrome and add-on debugging toolboxes"
4b. Open the Browser Console and run the following code (to simulate waiting a week for an update):
Services.search.getEngineByAlias("mdn").wrappedJSObject.setAttr("updateexpir", Date.now() - 691200000);
Services.search.QueryInterface(Components.interfaces.nsITimerCallback).notify(null);
5. Observe that the keyword has been lost (restart the browser to observe the loss of ordering)

Comment 36

[Florian Quèze [:florian]]

Comment on attachment 8770101 [details]
Bug 1274545 - Add a test for user-set keyword & order preservation on search engine update.

https://reviewboard.mozilla.org/r/63678/#review60976

Thanks!

The test passes locally for me (on Mac) and I assume it also does for you, but I would like this to go through a try run with xpcshell tests on all the relevant platforms before it lands.

Comment 37

[Ian Moody [:Kwan] (UTC+1)]

Thanks Florian! 

Green try (requested some extra windows runs that haven't finished yet), and did some browser chrome on linux
https://treeherder.mozilla.org/#/jobs?repo=try&revision=6b54ec7fa33d6a06defbd636cad0192f017d71a3

Comment 38

[Gerry Chang [:gchang]]

Track this as this is regression and impact user's behaviors.

Comment 39

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/fx-team/rev/107b045bcdc4
Rework the search engine _onLoad code to group update-only code together, and preserve user's metaData on update. r=florian
https://hg.mozilla.org/integration/fx-team/rev/6b143301ec2f
Add a test for user-set keyword & order preservation on search engine update. r=florian

Comment 40

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/107b045bcdc4
https://hg.mozilla.org/mozilla-central/rev/6b143301ec2f

Comment 41

[Florian Quèze [:florian]]

Comment on attachment 8768782 [details]
Bug 1274545 - Rework the search engine _onLoad code to group update-only code together, and preserve user's metaData on update.

Approval Request Comment
[Feature/regressing bug #]: regression from bug 1203167 in Firefox 45.
[User impact if declined]: user settings periodically lost for search plugins that make use of the engine update feature.
[Describe test coverage new/current, TreeHerder]: The modified code already had test coverage, and we added an xpcshell test covering specifically what this bug fixes.
[Risks and why]: Acceptable I think. Only the code that's specific to the engine-update case has been really modified. The patch looks larger than it actually is, due to code being moved around to reorganize it in a more coherent way.
[String/UUID change made/needed]: none

Comment 42

[Florian Quèze [:florian]]

Comment on attachment 8770101 [details]
Bug 1274545 - Add a test for user-set keyword & order preservation on search engine update.

Approval Request Comment

This test-only changeset should also be uplifted, but doesn't require approval.

Comment 43

[Sylvestre Ledru [:Sylvestre]]

Comment on attachment 8768782 [details]
Bug 1274545 - Rework the search engine _onLoad code to group update-only code together, and preserve user's metaData on update.

We are too close to the release date to take it in beta.
As it is not a recent regression, let it ride the train from aurora.

Comment 44

[Petruta Horea [:phorea], Desktop QA]

Reproduced and verified with str from comment 35 using Nightly 50.0a1 2016-07-19 under Win 10 64-bit, Ubuntu 14.04 64-bit and Mac OS X 10.9.5.

Comment 45

[Carsten Book [:Tomcat]]

the test has uplift problems to aurora

merging toolkit/components/search/tests/xpcshell/xpcshell.ini
warning: conflicts while merging toolkit/components/search/tests/xpcshell/xpcshell.ini! (edit, then use 'hg resolve --mark')
abort: unresolved conflicts, can't continue
(use 'hg resolve' and 'hg graft --continue')

Comment 46

[Ian Moody [:Kwan] (UTC+1)]

Attachment: Test patch for Aurora

(In reply to Carsten Book [:Tomcat] from comment #45)
> the test has uplift problems to aurora
> 
> merging toolkit/components/search/tests/xpcshell/xpcshell.ini
> warning: conflicts while merging
> toolkit/components/search/tests/xpcshell/xpcshell.ini! (edit, then use 'hg
> resolve --mark')
> abort: unresolved conflicts, can't continue
> (use 'hg resolve' and 'hg graft --continue')

Fixed.

Comment 47

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/305b50a208ae

Comment 48

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/e8c0bc753747