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.

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

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.

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

(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?

(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?

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.

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.

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

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/

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?

(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 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 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).

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 <florian@queze.net> 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.

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?

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.

(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

(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.

(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.

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 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 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

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.

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

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/

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 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 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;

[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.

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.

(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.

(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 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 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/

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 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.

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

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

Pushed by cbook@mozilla.com: 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

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

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 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 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.

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.

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')

(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.