Bug 699856 introduces |Lazy| to buffer writes to the metadata file. We should use |Lazy| to similarly buffer writes to search.json.

If anyone wants to pick this up, I'm willing to mentor.

I would like/have started to work on this bug. Please assign this to me.

Comment on attachment 626000 [details] [diff] [review] Patch v1 Review of attachment 626000 [details] [diff] [review]: ----------------------------------------------------------------- Good start. Could you please make the following changes and [re?]run it through the TryServer? ::: toolkit/components/search/nsSearchService.js @@ +1060,1 @@ > |this._self| won't work. You probably mean something along the lines of this._lazy = new Lazy((function() { try { this._serializeToFile(); } catch (ex) { LOG(...); } }).bind(this)); However, see further comments: you probably don't want this._lazy at all. @@ +1168,5 @@ > _iconUpdateURL: null, > // A reference to the timer used for lazily serializing the engine to file > + get _serializeTimer() { > + return this._lazy; > + }, You should probably remove _serializeTimer completely. @@ +2218,2 @@ > } > + ); You are defining this._lazy and this._lazyWriter. Just decide on one - I would use this._lazyWriter, as it ensures faster startup. Or you can go one step further and place this in a lazy getter. Something along the lines of: get _lazyWriter() { delete this._lazyWriter; let self = this; let result = new Lazy(...); this._lazyWriter = result; return result; } This has the benefit of: - not taking any significant time during startup (which is a main performance concern); - being as fast as using |this._lazy| after the second usage. @@ +2738,2 @@ > }, > Good habit to take: don't reuse the name of a variable or property for something different. This makes it harder to notice errors. Here, you may call this lazyCacheInvalidator, for instance. As above, you may also want to put this in a lazy getter. @@ +3504,1 @@ > this._buildCache(); I think that |flush| already calls |buildCache|. You don't need to do it twice.

Comment on attachment 626418 [details] [diff] [review] Patch v2 Review of attachment 626418 [details] [diff] [review]: ----------------------------------------------------------------- Still good, still a few things that we can improve. ::: toolkit/components/search/nsSearchService.js @@ +2193,5 @@ > > _lazySerializeToFile: function SRCH_ENG_serializeToFile() { > + this._lazyWriter.go(); > + }, > + Could you add one line of comment to explain what _lazyWriter is? @@ +2198,5 @@ > + get _lazyWriter() { > + delete this._lazyWriter; > + let self = this; > + let result = new Lazy( > + function() { For debugging purposes, it is generally a good idea to add a name to such functions. For instance, |new Lazy(function serializeToFile() { ... })|. @@ +2714,5 @@ > + delete this._lazyCacheInvalidator; > + let self = this; > + let result = new Lazy( > + function() { > + LOG("_lazyCacheInvalidation: Invalidating engine cache"); Here, too, give a name to that function. @@ +2719,5 @@ > + self._buildCache(); > + }, > + CACHE_INVALIDATION_DELAY > + ); > + this._lazyCacheInvalidator = result; You can factor these two lines into one: return this._lazyCacheInvalidator = result; @@ +2722,5 @@ > + ); > + this._lazyCacheInvalidator = result; > + return result; > + }, > + Here, you have not changed the semantics of the method, so I would keep name |_lazyCacheInvalidation|. On the other hand, name |_lazyCacheInvalidator| looks good for the |Lazy| object. @@ +3337,5 @@ > engineToRemove.alias = null; > } else { > // Cancel the lazy serialization timer if it's running > + if (engineToRemove._lazyWriter) { > + engineToRemove._lazyWriter.cancel(); Mmmhh... We will need something a little more subtle here. With the lazy getter, |engineToRemove._lazyWriter| is always going to return a writer. So, you have three possibilities: - you can simply remove that |if()| and always cancel; - you can detect if |engineToRemove._lazyWriter| is still a getter, using https://developer.mozilla.org/en/JavaScript/Reference/Global_Objects/Object/getOwnPropertyDescriptor, and only call |cancel| in that case. - you can introduce a boolean that tells you whether the getter has been called already, and only call |cancel| if this boolean is |true|. I would go for getOwnPropertyDescriptor.

Comment on attachment 626470 [details] [diff] [review] Patch v3 Review of attachment 626470 [details] [diff] [review]: ----------------------------------------------------------------- Looks good to me, with one tiny change. Please fix this, and r? gavin. ::: toolkit/components/search/nsSearchService.js @@ +2192,4 @@ > }, > > _lazySerializeToFile: function SRCH_ENG_serializeToFile() { > + //A Lazy object buffering calls to _serializeToFile I would put that comment just before |_lazySerializeToFile|, rather than inside.

Comment on attachment 626482 [details] [diff] [review] Patch v4 >diff --git a/toolkit/components/search/nsSearchService.js b/toolkit/components/search/nsSearchService.js > // Cancel the lazy serialization timer if it's running >- if (engineToRemove._serializeTimer) { >- engineToRemove._serializeTimer.cancel(); >- engineToRemove._serializeTimer = null; >+ if (!(Object.getOwnProperty(engineToRemove, "_lazyWriter").get === undefined)) { >+ engineToRemove._lazyWriter.cancel(); This is missing a "Descriptor": getOwnPropertyDescriptor. This error indicates that this patch wasn't tested - you should generally always test your patches before requesting review. >+ if (this._lazyCacheInvalidator) { This check will need the same getOwnPropertyDescriptor treatment.

I will make those changes. However, before submitting the patch, I ran |make -C toolkit/components/search/tests/ && make -C toolkit/components/search/tests xpcshell-tests| in my objdir. The result was this : INFO | Result summary: INFO | Passed: 4 INFO | Failed: 0 INFO | Todo: 0 So I thought maybe the tests were passing. I am sorry, from future I'll try and not repeat such silly things. Are there some other errors in my patch, that you want me to fix ? Should I run some other set of tests(supposing I ran some last time :P) before again asking for review ?

Ah, yeah, the current search service test suite coverage is not very comprehensive unfortunately. The best way to test these changes is to actually run the build and manually test that the search engine cache and engine files are properly written to disk. It would be great to add tests that caught the issues with the previous versions of this patch.

@David I'm sorry I haven't been able to get back to this bug for some reason or the other. I don't want to block anyone else, hence am unassigning myself. have a patch with that typo corrected but I haven't yet written the xpcshell test. Again, sorry David for having delayed this bug.

Note bug 775328, which will bit-rot this patch just a bit.

I'll continue with this.

Updated patch using DeferredTask.

Any news about that review?

Comment on attachment 665685 [details] [diff] [review] Patch v5 Review of attachment 665685 [details] [diff] [review]: ----------------------------------------------------------------- I have a test for search.json pending review from Gavin in attachment 618093 [details] [diff] [review] but I don't think there is one for the individual engine files. Could you please add a basic test? r+ with the test and changes below. ::: toolkit/components/search/nsSearchService.js @@ +2093,5 @@ > + if (!this._lazySerializeTask) { > + let task = function taskCallback() { > + this._serializeToFile(); > + }.bind(this); > + this._lazySerializeTask = new DeferredTask(task, LAZY_SERIALIZE_DELAY); The above can move to a memoizing getter for _lazySerializeTask. It doesn't seem like we gain much by delaying this but let me know if you disagree. @@ +2098,2 @@ > } > + this._lazySerializeTask.start(); Then the single caller of _lazySerializeToFile can just call this._lazySerializeTask.start(); directly. @@ +2673,2 @@ > } > + this._batchTask.start(); Same two comments for this task too.

Any news, andreshm?

Updated patch. Still pending the test, can you explain further what we need to test?

Comment on attachment 724785 [details] [diff] [review] Patch v6 Review of attachment 724785 [details] [diff] [review]: ----------------------------------------------------------------- Matt, I'm not clear about what tests you want.

Can I mark this as checkin-needed and add the test in a follow up bug if still required?

Comment on attachment 724785 [details] [diff] [review] Patch v6 I wanted tests to make sure that the tasks actually complete without exceptions as I don't think we have all these cases covered by existing tests. I just want to make sure the patch doesn't break things and that we won't break this in the future.

Matt, we are still not clear about _what_ you want tested.

e.g. the following test actually loads engines: http://dxr.mozilla.org/mozilla-central/toolkit/components/search/tests/xpcshell/test_nodb_pluschanges.js

1) I want a test which will fail when aEngine._serializeToFile(); is commented out in SRCH_ENG_onLoad and in lazySerializeTask. The toolkit xpcshell tests currently pass without this. 2) Tests that fail when calls to lazySerializeTask.start() are commented out is another example. Right now those codepaths are untested in toolkit/components/search/.

Andres, your turn.

(In reply to Matthew N. [:MattN] from comment #27) > 1) I want a test which will fail when aEngine._serializeToFile(); is > commented out in SRCH_ENG_onLoad and in lazySerializeTask. The toolkit > xpcshell tests currently pass without this. > 2) Tests that fail when calls to lazySerializeTask.start() are commented out > is another example. > > Right now those codepaths are untested in toolkit/components/search/. Added test cases.

Matthew: do you have a chance to review this bug?

Sorry, I forgot this was awaiting my review. I'll try get to this within the next few days.

Comment on attachment 742928 [details] [diff] [review] Patch v7 Review of attachment 742928 [details] [diff] [review]: ----------------------------------------------------------------- r=me with changes ::: toolkit/components/search/tests/xpcshell/head_search.js @@ +112,5 @@ > try { > stream.init(aFile, MODE_RDONLY, FileUtils.PERMS_FILE, 0); > return parseJsonFromStream(stream, stream.available()); > } catch(ex) { > + dump("readJSONFile: Error reading JSON file: " + ex); Please revert changes to this file. ::: toolkit/components/search/tests/xpcshell/test_serialize_file.js @@ +30,5 @@ > + NetUtil.asyncFetch(aFile, function(inputStream, status) { > + do_check_true(Components.isSuccessCode(status)); > + > + let data = > + NetUtil.readInputStreamToString(inputStream, inputStream.available()); Nit: no need to wrap here as we're fine with 100 characters in this module (especially in a test) @@ +43,5 @@ > + let engine2 = Services.search.getEngineByName("Sherlock test search engine"); > + if (engine1 && engine2) { > + // Test that files are written correctly. > + let engineFile1 = engine1.wrappedJSObject._file; > + let engineFile2 = engine2.wrappedJSObject._file Nit: missing semi-colon @@ +57,5 @@ > + // Test that lazySerializeTask updates the file. > + engine1.addParam("param-name", "param-value", null); > + // Timeout require to wait for lazySerializeTask to finish > + do_timeout(1000, function() { > + readAsyncFile(engineFile1, function(engineData) { I worry that this timeout will lead to intermittent failures. Perhaps we can poll the file modification date and and continue the test once it has increased? I don't know how accurate that is on all platforms though. I don't feel too strongly about this but it's hard to find the right number of balance intermittent failures and test execution time. @@ +64,5 @@ > + }); > + }); > + } > + } > + }; Nit: add a newline after the function and remove the semicolon. @@ +68,5 @@ > + }; > + Services.obs.addObserver(observer, "browser-search-engine-modified", false); > + > + Services.search.addEngine( > + "http://localhost:4444/data/engine.xml", Ci.nsISearchEngine.DATA_XML, Nit: In this module we generally prefer to start the arguments on the same line as the function: Services.search.addEngine("http://localhost:4444/data/engine.xml", Ci.nsISearchEngine.DATA_XML, null, false); @@ +71,5 @@ > + Services.search.addEngine( > + "http://localhost:4444/data/engine.xml", Ci.nsISearchEngine.DATA_XML, > + null, false); > + Services.search.addEngine( > + "http://localhost:4444/data/engine.src", Ci.nsISearchEngine.DATA_TEXT, ditto @@ +75,5 @@ > + "http://localhost:4444/data/engine.src", Ci.nsISearchEngine.DATA_TEXT, > + "http://localhost:4444/data/ico-size-16x16-png.ico", false); > + > + do_timeout(120000, function() { > + do_throw("Timeout"); This is no longer needed due to bug 597064 getting fixed. ::: toolkit/components/search/tests/xpcshell/xpcshell.ini @@ +16,5 @@ > [test_nodb_pluschanges.js] > [test_save_sorted_engines.js] > [test_purpose.js] > [test_engineselect.js] > +[test_serialize_file.js] Nit: can you please put these last three tests in alphabetical order and test that the whole directory still passes.

Updated patch.

Pushed to try: https://tbpl.mozilla.org/?tree=Try&rev=92fc21125d06

(In reply to Andres Hernandez [:andreshm] from comment #34) > Pushed to try: https://tbpl.mozilla.org/?tree=Try&rev=92fc21125d06 Could you check this it fails on Windows platforms please?

I have been running tests on try, in order to find what is causing the fail on windows. Test on try: https://tbpl.mozilla.org/?tree=Try&rev=4766500d1ff4 I have an idea what is failing but still need to confirm with the dumps.

Rebased patch. New test on try: https://tbpl.mozilla.org/?tree=Try&rev=5ef29b438942

So, what's the current status on that bug?

I still need to rewrite a bit this test. Since the test is not currently working on windows. This was the last try test: https://tbpl.mozilla.org/?tree=Try&rev=afd07baa8e87 and the full log: https://tbpl.mozilla.org/php/getParsedLog.php?id=25208722&tree=Try&full=1 I'll be working on this in the next days.

Refactor the test in order to fix the Windows issue. Pushed to try again for fresh run: https://tbpl.mozilla.org/?tree=Try&rev=218673b51505

Comment on attachment 807412 [details] [diff] [review] bug-725943.patch r- due to xpcshell test failures in the try push

I tried also adding a loop to wait for the afterCommit function, but didn't work. See: https://tbpl.mozilla.org/?tree=Try&rev=c6a51bf928f5 I'm stuck and need some guidance on this bug, since after adding a param to the engine, the engine file should be updated and I can't use a timeout. But, neither waiting for file size to change or using the afterCommit method is working. Any ideas on how I can solve this?

(In reply to Andres Hernandez [:andreshm] from comment #42) > I tried also adding a loop to wait for the afterCommit function, but didn't > work. See: > https://tbpl.mozilla.org/?tree=Try&rev=c6a51bf928f5 "Adding a loop"? That sounds risky. What are you trying to do, exactly?

The test consist on adding a custom param to an existing search engine. And then check the engine file to verify that the param was added successfully. So, after I add the param I have to wait for the file to be written with the serializeToFile function. But, I haven't found a way to get the engine file after is updated by the DeferredTask. > let engine = Services.search.getEngineByName("Test search engine"); > engine.addParam("param-name", "param-value", null);

To test such features in Session Restore, we send observer notifications whenever we write to our file. I don't remember whether that's the case with nsSearchService, but if it isn't, this would be the most robust way to solve the problem.

(In reply to David Rajchenbach Teller [:Yoric] <on summit, unavailable until October 8th> from comment #45) > To test such features in Session Restore, we send observer notifications > whenever we write to our file. I don't remember whether that's the case with > nsSearchService, but if it isn't, this would be the most robust way to solve > the problem. David is right that I think you can use the observer notifications (e.g. SEARCH_SERVICE_CACHE_WRITTEN) for this.

I have added a new notification and used that in the test. https://tbpl.mozilla.org/?tree=Try&rev=f162c07b1049

Comment on attachment 821563 [details] [diff] [review] v10 Review of attachment 821563 [details] [diff] [review]: ----------------------------------------------------------------- ::: toolkit/components/search/nsSearchService.js @@ +2246,5 @@ > > closeSafeOutputStream(fos); > + > + Services.obs.notifyObservers(null, SEARCH_SERVICE_TOPIC, > + "serialize-engine-to-file-complete"); Nit: The other two related notifications use "*-to-disk-complete" ("disk" instead of "file") so it would be nice to be consistent with their suffix. On the other hand, this name is closer to the function name. I think I'd still prefer consistency with the other notifications but I don't feel strongly about it.

(In reply to Matthew N. [:MattN] (catching up on reviews) from comment #48) > Comment on attachment 821563 [details] [diff] [review] > v10 > > Review of attachment 821563 [details] [diff] [review]: > ----------------------------------------------------------------- > > ::: toolkit/components/search/nsSearchService.js > @@ +2246,5 @@ > > > > closeSafeOutputStream(fos); > > + > > + Services.obs.notifyObservers(null, SEARCH_SERVICE_TOPIC, > > + "serialize-engine-to-file-complete"); > > Nit: The other two related notifications use "*-to-disk-complete" ("disk" > instead of "file") so it would be nice to be consistent with their suffix. > On the other hand, this name is closer to the function name. I think I'd > still prefer consistency with the other notifications but I don't feel > strongly about it. I have changed the "serialize-engine-to-file-complete" to ""write-engine-to-disk-complete" Pushed to try and waiting for results https://tbpl.mozilla.org/?tree=Try&rev=b70bd71c82b0

Passed Try https://tbpl.mozilla.org/?tree=Try&rev=b70bd71c82b0

Backed out for failing intermittently (but frequently) in test_serialize_file.js, eg: https://tbpl.mozilla.org/php/getParsedLog.php?id=29953547&tree=Fx-Team https://tbpl.mozilla.org/php/getParsedLog.php?id=29955030&tree=Fx-Team remote: https://hg.mozilla.org/integration/fx-team/rev/ffef202a0bcc

The problem was probably caused by the topic "browser-search-service" with "write-engine-to-disk-complete" is fired for the code in the first test "test_batchTask" when running the second test "test_addParam". Extra check is added to the second test to address that.

(In reply to Raymond Lee [:raymondlee] from comment #53) > Created attachment 826808 [details] [diff] [review] > v12 > > The problem was probably caused by the topic "browser-search-service" with > "write-engine-to-disk-complete" is fired for the code in the first test > "test_batchTask" when running the second test "test_addParam". Extra check > is added to the second test to address that. Pushed to try twice https://tbpl.mozilla.org/?tree=Try&rev=2400074a6faf https://tbpl.mozilla.org/?tree=Try&rev=0aa817d34024

I have updated the patch, since the nsSeachService.js has been changed. Pushed to try and passed. https://tbpl.mozilla.org/?tree=Try&rev=a493fe26382e