nsSearchService write cache using OS.File

RESOLVED FIXED in Firefox 22

Status

()

Firefox
Search
RESOLVED FIXED
6 years ago
5 years ago

People

(Reporter: Yoric, Assigned: Yoric)

Tracking

unspecified
Firefox 22
Points:
---
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 3 obsolete attachments)

Extracted from bug 706523.
Migrate writing the cache to OS.File.
Created attachment 699732 [details] [diff] [review]
Write nsSearchService cache with OS.File
Assignee: nobody → dteller
Attachment #699732 - Flags: review?(gavin.sharp)
Comment on attachment 699732 [details] [diff] [review]
Write nsSearchService cache with OS.File

Do we have test coverage here?
Attachment #699732 - Flags: review?(gavin.sharp) → review+
I don't think we do. I'll try and find out how to add some.
So, for the sake of testing, I could send a notification "cache-built" or something such. Gavin, do you think it's worth the bother?
Flags: needinfo?(gavin.sharp)
If that's all that's needed to get test coverage for cache population that we otherwise don't have, that certainly sounds worth it!
Flags: needinfo?(gavin.sharp)
Created attachment 701066 [details] [diff] [review]
Write nsSearchService cache with OS.File, v2

Same one, with tests.
Attachment #699732 - Attachment is obsolete: true
Attachment #701066 - Flags: review?(gavin.sharp)
Attachment #701066 - Flags: review?(gavin.sharp) → review+
Hrm, was this forgotten, or is there something else preventing it from landing?
Created attachment 716529 [details] [diff] [review]
Write nsSearchService cache with OS.File, v2.1

Indeed, it was forgotten (and slightly bitrotten).

I will wait for Telemetry to tell us whether encoding on the main thread is a good idea, though. I am starting to believe that we should wait for something like bug 832664 before proceeding with such transformations.
Attachment #701066 - Attachment is obsolete: true
Attachment #716529 - Flags: review+
(In reply to David Rajchenbach Teller [:Yoric] from comment #8)
> I will wait for Telemetry to tell us whether encoding on the main thread is
> a good idea, though. I am starting to believe that we should wait for
> something like bug 832664 before proceeding with such transformations.

This isn't adding any additional main thread work, right? As far as I can tell this is a net reduction in the amount of main-thread work going on, so I don't see why we would block on additional improvements.
Ah, you are right. I was assuming that the pre-existing code managed to make more things asynchronous. So the patch is a net improvement – one which we may want to improve further.

Try: https://tbpl.mozilla.org/?tree=Try&rev=a1ecaeb21261
Created attachment 717509 [details] [diff] [review]
Write nsSearchService cache with OS.File, v2.2

Fixed test.
Try: https://tbpl.mozilla.org/?tree=Try&rev=8cb97f8a9fde
Attachment #716529 - Attachment is obsolete: true
Attachment #717509 - Flags: review+
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/910837eec45a
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 22

Comment 14

5 years ago
Hmm, this test fails intermittently when we run it on our own builds in Ubuntu:

https://jenkins.qa.ubuntu.com/job/raring-ppa-adt-ubuntu_mozilla_daily_ppa-firefox-trunk/92/ARCH=amd64,label=adt/testReport/junit/toolkit.components.search.tests/xpcshell/test_nocache_js/

It looks like adding the search engine completes before the initial cache file is written
I'll take a look asap.
Flags: needinfo?(dteller)
Chris, I can't read that log. Do you still have it accessible somewhere?
Flags: needinfo?(dteller) → needinfo?(chrisccoulson)

Comment 17

5 years ago
Ah, sorry, I didn't realize that Jenkins doesn't keep very much history, otherwise I would have just pasted the failure inline (and we filter this test out now so that the failure doesn't show up in the UI).

I've just had a look through some recent logs though and found this, which I think is the same failure:

TEST-INFO | /tmp/tmp.pFFfKlLDYi/dsc0t-xpcshell-tests-testtmp/tmpdir/tmpB9hGWd/xpcshell/tests/toolkit/components/search/tests/xpcshell/test_nocache.js | running test ...
TEST-UNEXPECTED-FAIL | /tmp/tmp.pFFfKlLDYi/dsc0t-xpcshell-tests-testtmp/tmpdir/tmpB9hGWd/xpcshell/tests/toolkit/components/search/tests/xpcshell/test_nocache.js | test failed (with xpcshell return code: 0), see following log:
>>>>>>>

TEST-INFO | (xpcshell/head.js) | test 1 pending

TEST-PASS | /tmp/tmp.pFFfKlLDYi/dsc0t-xpcshell-tests-testtmp/tmpdir/tmpB9hGWd/xpcshell/head.js | [do_load_manifest : 834] true == true

TEST-INFO | (xpcshell/head.js) | test 2 pending
*** Search: metadata init: starting

TEST-INFO | (xpcshell/head.js) | test 2 finished

TEST-INFO | (xpcshell/head.js) | running event loop
*** Search: SRCH_SVC_EMS_asyncMigrate start
*** Search: SRCH_SVC_EMS_migrate search.sqlite does not exist
*** Search: metadata init: No store to migrate to disk
*** Search: _syncLoadEngines: start
*** Search: _loadEngines: Absent or outdated cache. Loading engines from disk.
*** Search: _buildCache: Writing to cache file.

TEST-PASS | /tmp/tmp.pFFfKlLDYi/dsc0t-xpcshell-tests-testtmp/tmpdir/tmpB9hGWd/xpcshell/tests/toolkit/components/search/tests/xpcshell/test_nocache.js | [ss_initialized : 45] true == true

TEST-INFO | /tmp/tmp.pFFfKlLDYi/dsc0t-xpcshell-tests-testtmp/tmpdir/tmpB9hGWd/xpcshell/tests/toolkit/components/search/tests/xpcshell/test_nocache.js | Setting up observer
*** Search: addEngine: Adding "http://localhost:4444/data/engine.xml".
*** Search: _initFromURI: Downloading engine from: "http://localhost:4444/data/engine.xml".
*** Search: _syncInit: Completed _syncInit
*** Search: loadListener: Starting request: http://localhost:4444/data/engine.xml
*** Search: loadListener: Stopping request: http://localhost:4444/data/engine.xml
*** Search: _init: Initing MozSearch plugin from http://localhost:4444/data/engine.xml
*** Search: _parseImage: Image textContent: "data:image/png;base64,AAABAAEAEBAAAAEAGABoAwAAFgAAACgAAAAQAAAAIAAAAAEAGAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAADs9Pt8xetPtu9FsfFNtu%2BTzvb2%2B%2Fne4dFJeBw0egA%2FfAJAfAA8ewBBegAAAAD%2B%2FPtft98Mp%2BwWsfAVsvEbs%2FQeqvF8xO7%2F%2F%2F63yqkxdgM7gwE%2FggM%2BfQA%2BegBDeQDe7PIbotgQufcMufEPtfIPsvAbs%2FQvq%2Bfz%2Bf%2F%2B%2B%2FZKhR05hgBBhQI8hgBAgAI9ewD0%2B%2Fg3pswAtO8Cxf4Kw%2FsJvvYAqupKsNv%2B%2Fv7%2F%2FP5VkSU0iQA7jQA9hgBDgQU%2BfQH%2F%2Ff%2FQ6fM4sM4KsN8AteMCruIqqdbZ7PH8%2Fv%2Fg6Nc%2Fhg05kAA8jAM9iQI%2BhQA%2BgQDQu6b97uv%2F%2F%2F7V8Pqw3eiWz97q8%2Ff%2F%2F%2F%2F7%2FPptpkkqjQE4kwA7kAA5iwI8iAA8hQCOSSKdXjiyflbAkG7u2s%2F%2B%2F%2F39%2F%2F7r8utrqEYtjQE8lgA7kwA7kwA9jwA9igA9hACiWSekVRyeSgiYSBHx6N%2F%2B%2Fv7k7OFRmiYtlAA5lwI7lwI4lAA7kgI9jwE9iwI4iQCoVhWcTxCmb0K%2BooT8%2Fv%2F7%2F%2F%2FJ2r8fdwI1mwA3mQA3mgA8lAE8lAE4jwA9iwE%2BhwGfXifWvqz%2B%2Ff%2F58u%2Fev6Dt4tr%2B%2F%2F2ZuIUsggA7mgM6mAM3lgA5lgA6kQE%2FkwBChwHt4dv%2F%2F%2F728ei1bCi7VAC5XQ7kz7n%2F%2F%2F6bsZkgcB03lQA9lgM7kwA2iQktZToPK4r9%2F%2F%2F9%2F%2F%2FSqYK5UwDKZAS9WALIkFn%2B%2F%2F3%2F%2BP8oKccGGcIRJrERILYFEMwAAuEAAdX%2F%2Ff7%2F%2FP%2B%2BfDvGXQLIZgLEWgLOjlf7%2F%2F%2F%2F%2F%2F9QU90EAPQAAf8DAP0AAfMAAOUDAtr%2F%2F%2F%2F7%2B%2Fu2bCTIYwDPZgDBWQDSr4P%2F%2Fv%2F%2F%2FP5GRuABAPkAA%2FwBAfkDAPAAAesAAN%2F%2F%2B%2Fz%2F%2F%2F64g1C5VwDMYwK8Yg7y5tz8%2Fv%2FV1PYKDOcAAP0DAf4AAf0AAfYEAOwAAuAAAAD%2F%2FPvi28ymXyChTATRrIb8%2F%2F3v8fk6P8MAAdUCAvoAAP0CAP0AAfYAAO4AAACAAQAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAACAAQAA"
*** Search: _setIcon: Setting icon url "data:image/png;base64,AAABAAEAEBAAAAEAGABoAwAAFgAAACgAAAAQAAAAIAAAAAEAGAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAADs9Pt8xetPtu9FsfFNtu%2BTzvb2%2B%2Fne4dFJeBw0egA%2FfAJAfAA8ewBBegAAAAD%2B%2FPtft98Mp%2BwWsfAVsvEbs%2FQeqvF8xO7%2F%2F%2F63yqkxdgM7gwE%2FggM%2BfQA%2BegBDeQDe7PIbotgQufcMufEPtfIPsvAbs%2FQvq%2Bfz%2Bf%2F%2B%2B%2FZKhR05hgBBhQI8hgBAgAI9ewD0%2B%2Fg3pswAtO8Cxf4Kw%2FsJvvYAqupKsNv%2B%2Fv7%2F%2FP5VkSU0iQA7jQA9hgBDgQU%2BfQH%2F%2Ff%2FQ6fM4sM4KsN8AteMCruIqqdbZ7PH8%2Fv%2Fg6Nc%2Fhg05kAA8jAM9iQI%2BhQA%2BgQDQu6b97uv%2F%2F%2F7V8Pqw3eiWz97q8%2Ff%2F%2F%2F%2F7%2FPptpkkqjQE4kwA7kAA5iwI8iAA8hQCOSSKdXjiyflbAkG7u2s%2F%2B%2F%2F39%2F%2F7r8utrqEYtjQE8lgA7kwA7kwA9jwA9igA9hACiWSekVRyeSgiYSBHx6N%2F%2B%2Fv7k7OFRmiYtlAA5lwI7lwI4lAA7kgI9jwE9iwI4iQCoVhWcTxCmb0K%2BooT8%2Fv%2F7%2F%2F%2FJ2r8fdwI1mwA3mQA3mgA8lAE8lAE4jwA9iwE%2BhwGfXifWvqz%2B%2Ff%2F58u%2Fev6Dt4tr%2B%2F%2F2ZuIUsggA7mgM6mAM3lgA5lgA6kQE%2FkwBChwHt4dv%2F%2F%2F728ei1bCi7VAC5XQ7kz7n%2F%2F%2F6bsZkgcB03lQA9lgM7kwA2iQktZToPK4r9%2F%2F%2F9%2F%2F%2FSqYK5UwDKZAS9WALIkFn%2B%2F%2F3%2F%2BP8oKccGGcIRJrERILYFEMwAAuEAAdX%2F%2Ff7%2F%2FP%2B%2BfDvGXQLIZgLEWgLOjlf7%2F%2F%2F%2F%2F%2F9QU90EAPQAAf8DAP0AAfMAAOUDAtr%2F%2F%2F%2F7%2B%2Fu2bCTIYwDPZgDBWQDSr4P%2F%2Fv%2F%2F%2FP5GRuABAPkAA%2FwBAfkDAPAAAesAAN%2F%2F%2B%2Fz%2F%2F%2F64g1C5VwDMYwK8Yg7y5tz8%2Fv%2FV1PYKDOcAAP0DAf4AAf0AAfYEAOwAAuAAAAD%2F%2FPvi28ymXyChTATRrIb8%2F%2F3v8fk6P8MAAdUCAvoAAP0CAP0AAfYAAO4AAACAAQAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAACAAQAA" for engine "Test search engine".
*** Search: NOTIFY: Engine: "Test search engine"; Verb: "engine-changed"

TEST-INFO | /tmp/tmp.pFFfKlLDYi/dsc0t-xpcshell-tests-testtmp/tmpdir/tmpB9hGWd/xpcshell/tests/toolkit/components/search/tests/xpcshell/test_nocache.js | Observing topic browser-search-engine-modified
*** Search: NOTIFY: Engine: "Test search engine"; Verb: "engine-loaded"

TEST-INFO | /tmp/tmp.pFFfKlLDYi/dsc0t-xpcshell-tests-testtmp/tmpdir/tmpB9hGWd/xpcshell/tests/toolkit/components/search/tests/xpcshell/test_nocache.js | Observing topic browser-search-engine-modified
*** Search: nsSearchService::observe: Done installation of Test search engine.
*** Search: _addEngineToStore: Adding engine: "Test search engine"
*** Search: NOTIFY: Engine: "Test search engine"; Verb: "engine-added"

TEST-INFO | /tmp/tmp.pFFfKlLDYi/dsc0t-xpcshell-tests-testtmp/tmpdir/tmpB9hGWd/xpcshell/tests/toolkit/components/search/tests/xpcshell/test_nocache.js | Observing topic browser-search-engine-modified

TEST-INFO | /tmp/tmp.pFFfKlLDYi/dsc0t-xpcshell-tests-testtmp/tmpdir/tmpB9hGWd/xpcshell/tests/toolkit/components/search/tests/xpcshell/test_nocache.js | Engine has been added, let's wait for the cache to be built
*** Search: nsSearchService::observe: setting current
*** Search: _buildSortedEngineList: building list
*** Search: NOTIFY: Engine: "Test search engine"; Verb: "engine-current"
*** Search: _batchCacheInvalidation: Batch timer reset

TEST-INFO | /tmp/tmp.pFFfKlLDYi/dsc0t-xpcshell-tests-testtmp/tmpdir/tmpB9hGWd/xpcshell/tests/toolkit/components/search/tests/xpcshell/head_search.js | afterCache: write-cache-to-disk-complete

TEST-INFO | /tmp/tmp.pFFfKlLDYi/dsc0t-xpcshell-tests-testtmp/tmpdir/tmpB9hGWd/xpcshell/tests/toolkit/components/search/tests/xpcshell/test_nocache.js | Success

TEST-INFO | /tmp/tmp.pFFfKlLDYi/dsc0t-xpcshell-tests-testtmp/tmpdir/tmpB9hGWd/xpcshell/tests/toolkit/components/search/tests/xpcshell/test_nocache.js | Searching test engine in cache

TEST-INFO | /tmp/tmp.pFFfKlLDYi/dsc0t-xpcshell-tests-testtmp/tmpdir/tmpB9hGWd/xpcshell/tests/toolkit/components/search/tests/xpcshell/head_search.js | afterCache: write-cache-to-disk-complete

TEST-PASS | /tmp/tmp.pFFfKlLDYi/dsc0t-xpcshell-tests-testtmp/tmpdir/tmpB9hGWd/xpcshell/tests/toolkit/components/search/tests/xpcshell/test_nocache.js | [cacheCreated : 40] true == true

TEST-UNEXPECTED-FAIL | /tmp/tmp.pFFfKlLDYi/dsc0t-xpcshell-tests-testtmp/tmpdir/tmpB9hGWd/xpcshell/tests/toolkit/components/search/tests/xpcshell/test_nocache.js | false == true - See following stack:
JS frame :: /tmp/tmp.pFFfKlLDYi/dsc0t-xpcshell-tests-testtmp/tmpdir/tmpB9hGWd/xpcshell/head.js :: do_throw :: line 461
JS frame :: /tmp/tmp.pFFfKlLDYi/dsc0t-xpcshell-tests-testtmp/tmpdir/tmpB9hGWd/xpcshell/head.js :: do_report_result :: line 563
JS frame :: /tmp/tmp.pFFfKlLDYi/dsc0t-xpcshell-tests-testtmp/tmpdir/tmpB9hGWd/xpcshell/head.js :: _do_check_eq :: line 573
JS frame :: /tmp/tmp.pFFfKlLDYi/dsc0t-xpcshell-tests-testtmp/tmpdir/tmpB9hGWd/xpcshell/head.js :: do_check_eq :: line 580
JS frame :: /tmp/tmp.pFFfKlLDYi/dsc0t-xpcshell-tests-testtmp/tmpdir/tmpB9hGWd/xpcshell/head.js :: do_check_true :: line 594
JS frame :: /tmp/tmp.pFFfKlLDYi/dsc0t-xpcshell-tests-testtmp/tmpdir/tmpB9hGWd/xpcshell/tests/toolkit/components/search/tests/xpcshell/test_nocache.js :: task :: line 79
JS frame :: resource://gre/modules/Task.jsm :: TaskImpl_run :: line 192
JS frame :: resource://gre/modules/commonjs/sdk/core/promise.js :: resolve :: line 120
JS frame :: resource://gre/modules/commonjs/sdk/core/promise.js :: then :: line 45
JS frame :: resource://gre/modules/commonjs/sdk/core/promise.js :: resolve :: line 187
JS frame :: resource://gre/modules/commonjs/sdk/core/promise.js :: resolve :: line 120
JS frame :: resource://gre/modules/commonjs/sdk/core/promise.js :: then :: line 45
JS frame :: resource://gre/modules/commonjs/sdk/core/promise.js :: resolve :: line 187
JS frame :: resource://gre/modules/commonjs/sdk/core/promise.js :: resolve :: line 120
JS frame :: resource://gre/modules/commonjs/sdk/core/promise.js :: then :: line 45
JS frame :: resource://gre/modules/commonjs/sdk/core/promise.js :: resolve :: line 187
JS frame :: resource://gre/modules/osfile/_PromiseWorker.jsm :: onmessage :: line 134
native frame :: <unknown filename> :: <TOP_LEVEL> :: line 0

TEST-INFO | (xpcshell/head.js) | exiting test

TEST-UNEXPECTED-FAIL | /tmp/tmp.pFFfKlLDYi/dsc0t-xpcshell-tests-testtmp/tmpdir/tmpB9hGWd/xpcshell/tests/toolkit/components/search/tests/xpcshell/test_nocache.js | 2147500036 - See following stack:
JS frame :: /tmp/tmp.pFFfKlLDYi/dsc0t-xpcshell-tests-testtmp/tmpdir/tmpB9hGWd/xpcshell/head.js :: do_throw :: line 461
JS frame :: /tmp/tmp.pFFfKlLDYi/dsc0t-xpcshell-tests-testtmp/tmpdir/tmpB9hGWd/xpcshell/tests/toolkit/components/search/tests/xpcshell/test_nocache.js :: task :: line 81
JS frame :: resource://gre/modules/Task.jsm :: TaskImpl_run :: line 192
JS frame :: resource://gre/modules/commonjs/sdk/core/promise.js :: resolve :: line 120
JS frame :: resource://gre/modules/commonjs/sdk/core/promise.js :: then :: line 45
JS frame :: resource://gre/modules/commonjs/sdk/core/promise.js :: resolve :: line 187
JS frame :: resource://gre/modules/commonjs/sdk/core/promise.js :: resolve :: line 120
JS frame :: resource://gre/modules/commonjs/sdk/core/promise.js :: then :: line 45
JS frame :: resource://gre/modules/commonjs/sdk/core/promise.js :: resolve :: line 187
JS frame :: resource://gre/modules/commonjs/sdk/core/promise.js :: resolve :: line 120
JS frame :: resource://gre/modules/commonjs/sdk/core/promise.js :: then :: line 45
JS frame :: resource://gre/modules/commonjs/sdk/core/promise.js :: resolve :: line 187
JS frame :: resource://gre/modules/osfile/_PromiseWorker.jsm :: onmessage :: line 134
native frame :: <unknown filename> :: <TOP_LEVEL> :: line 0

TEST-INFO | (xpcshell/head.js) | exiting test

TEST-INFO | (xpcshell/head.js) | test 1 finished

TEST-INFO | (xpcshell/head.js) | exiting test
<<<<<<<
Flags: needinfo?(chrisccoulson)
You need to log in before you can comment on or make changes to this bug.