Closed Bug 1534710 Opened 5 years ago Closed 5 years ago

address test issues with searchservice and webextensions

Categories

(WebExtensions :: General, enhancement, P1)

enhancement

Tracking

(Not tracked)

RESOLVED FIXED

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

Blocks: 1496075
Assignee: nobody → mixedpuppy
Priority: -- → P1

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

Summary: Make search extension APP_STARTUP notification synchronous → address test issues with searchservice and webextensions

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.

That is a failure to parse the manifest against the schema. Specifically the error is thrown here:

https://searchfox.org/mozilla-central/rev/4eaf7c637a303e0f7adcf1ae45064b2d4bef9eb0/toolkit/components/extensions/Schemas.jsm#1247

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.

Dale, can you look into what I've done in the patch and test the marionette test on your end?

Flags: needinfo?(dharvey)
Depends on: 1538975

These patches will be integrated into the other bugs, so assigning to dale to close when appropriate.

Assignee: mixedpuppy → dharvey
Flags: needinfo?(dharvey)
Attachment #9053377 - Attachment is obsolete: true
Attachment #9052987 - Attachment is obsolete: true

Changes from this are imported into other patches now, closing.

Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: