address test issues with searchservice and webextensions
Categories
(WebExtensions :: General, enhancement, P1)
Tracking
(Not tracked)
People
(Reporter: daleharvey, Assigned: daleharvey)
References
Details
Attachments
(2 obsolete files)
As part of https://bugzilla.mozilla.org/show_bug.cgi?id=1496075#c60, the SearchService needs to be aware of installed search extensions before it starts init()
Kris suggestion on IRC that these notifications can be sent synchronously by storing the details in addonStartup.json
Updated•6 years ago
|
Assignee | ||
Comment 1•6 years ago
|
||
Trying to clarify the problem, if you checkout https://phabricator.services.mozilla.com/D24142 (+ its deps) and run
$ ./mach marionette-test browser/components/search/test/marionette/test_engines_on_restart.py --gecko-log -
It should fail with:
FAIL browser/components/search/test/marionette/test_engines_on_restart.py TestEnginesOnRestart.test_engines - AssertionError: 'google' != u'bing'
The test opens Firefox, checks google is the default search engine, restarts firefox and checks that google is still the default search engine.
I have annotated the log @ https://gist.github.com/daleharvey/396edddb2d8c8f2146ed0bb26344564c, the reason it fails is if I try to |AddonManager.installBuiltInAddon("resource://search-extensions/google")| when that extension has been previously installed, I get a broken extension object sent to addEnginesFromExtension. rawManifest is null at the point I try to call getLocalisedManifest and it crashes.
To explain the problem at a higher level, when the ServiceService starts I have a list of engines "foo, bar", by the time SearchService.init() is complete I need to know the details for those engines, search_url=http://foo.org etc, currently the only way to get that information is being notified via addEnginesFromExtension. Now if I install a fresh extension it will call addEnginesFromExtension for me immediately, however as you can see if I call that on and engine that has been previously installed things break
I could use AddonManager.getActiveAddons() to not reinstall previously installed addons, but then for SearchService.init to finish, it still needs to wait for addEnginesFromExtension to be called which as far as I understand requires loading the addons db which I believe there is an effort to be trying to delay that until as late as possible (ie the same issue with calling getAddonByID)
kmag proposed in IRC that it would be possible to ensure that addEnginesFromExtension would be guaranteed to be called for each installed search extension before SearchService.init could possibly be called, this means the data would be available for SearchService to init immediately
Updated•6 years ago
|
Comment 2•6 years ago
|
||
I've tracked down the primary blockers as far as I can tell. The patch I'll attach here has a couple XXX comments that need some better understanding to make sure the changes are correct.
- Regarding the failure with getLocalizedManifest as illustrated here: https://gist.github.com/daleharvey/9acf6e21ad8ac4c9a778f7c0843c52f5#file-gistfile1-txt-L344
That is a failure to parse the manifest against the schema. Specifically the error is thrown here:
This tells me the google engines b-d locale is resolving into a manifest that doesn't match required schema. I haven't looked at what or why.
-
The xpcshell test head_search.js is doing some, as others phrased, sketchy startup stuff. My patch removes the particular problem that was causing the AOM state to not be stored. At that point I was able to write new tests, and fix the existing failures in test_webextension_install.js.
-
The failing marionette test seems fixed by some small changes I did in searchsearvice. I commented specifically at the change points regarding this, search for XXX comments in the file.
Comment 3•6 years ago
|
||
Comment 4•6 years ago
|
||
Dale, can you look into what I've done in the patch and test the marionette test on your end?
Comment 5•6 years ago
|
||
Comment 6•6 years ago
|
||
These patches will be integrated into the other bugs, so assigning to dale to close when appropriate.
Updated•6 years ago
|
Updated•6 years ago
|
Comment 7•6 years ago
|
||
Changes from this are imported into other patches now, closing.
Description
•