Closed Bug 1224703 Opened 6 years ago Closed 6 years ago

Enable WebExtensions tests on emulator

Categories

(Firefox OS Graveyard :: General, defect)

ARM
Gonk (Firefox OS)
defect
Not set
normal

Tracking

(firefox46 fixed)

RESOLVED FIXED
2.6 S5 - 1/15
Tracking Status
firefox46 --- fixed

People

(Reporter: fabrice, Assigned: fabrice)

References

Details

Attachments

(2 files, 2 obsolete files)

No description provided.
Attached patch ext-tests-b2g.patch (obsolete) — Splinter Review
Try run at https://treeherder.mozilla.org/#/jobs?repo=try&revision=ed320a385127

The failure is fixed in the patch (I though that e10s was defined in b2g mochitests).
Attachment #8687398 - Flags: review?(wmccloskey)
Comment on attachment 8687398 [details] [diff] [review]
ext-tests-b2g.patch

Review of attachment 8687398 [details] [diff] [review]:
-----------------------------------------------------------------

Just have a few questions below.

::: dom/apps/UserCustomizations.jsm
@@ +79,5 @@
> +      // aURI is expected to be the appURI from the jarChannel but there is
> +      // no real app associated to mochitest's jar:remoteopenfile:/// uris.
> +      return true;
> +    }
> +    return aURI ? this.appId.has(aURI.host) : true;

Do we need this change? Shouldn't aURI only be null if webextensions.tests is true?

::: toolkit/components/extensions/test/mochitest/mochitest.ini
@@ +26,5 @@
>  [test_ext_generate.html]
>  [test_ext_localStorage.html]
>  [test_ext_notifications.html]
>  [test_ext_runtime_connect.html]
> +skip-if = buildapp == 'b2g' # runat != document_idle is not supported.

I think test_ext_contentscript.html is the only test that cares about the runat value. I think it would be better to change the other tests to use document_idle.

::: toolkit/components/extensions/test/mochitest/test_ext_simple.html
@@ +13,5 @@
>  <script type="application/javascript;version=1.8">
>  
> +add_task(function* setup() {
> +  var p = new Promise(resolve => {
> +    SpecialPowers.pushPrefEnv({"set": [['webextensions.tests', true]]}, resolve);

I don't understand why you only need to do this for test_ext_simple.html.
(In reply to Bill McCloskey (:billm) from comment #2)
> Comment on attachment 8687398 [details] [diff] [review]
> ext-tests-b2g.patch
> 
> Review of attachment 8687398 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Just have a few questions below.
> 
> ::: dom/apps/UserCustomizations.jsm
> @@ +79,5 @@
> > +      // aURI is expected to be the appURI from the jarChannel but there is
> > +      // no real app associated to mochitest's jar:remoteopenfile:/// uris.
> > +      return true;
> > +    }
> > +    return aURI ? this.appId.has(aURI.host) : true;
> 
> Do we need this change? Shouldn't aURI only be null if webextensions.tests
> is true?

You're right, that's a leftover, will revert.

> ::: toolkit/components/extensions/test/mochitest/mochitest.ini
> @@ +26,5 @@
> >  [test_ext_generate.html]
> >  [test_ext_localStorage.html]
> >  [test_ext_notifications.html]
> >  [test_ext_runtime_connect.html]
> > +skip-if = buildapp == 'b2g' # runat != document_idle is not supported.
> 
> I think test_ext_contentscript.html is the only test that cares about the
> runat value. I think it would be better to change the other tests to use
> document_idle.

ok!

> ::: toolkit/components/extensions/test/mochitest/test_ext_simple.html
> @@ +13,5 @@
> >  <script type="application/javascript;version=1.8">
> >  
> > +add_task(function* setup() {
> > +  var p = new Promise(resolve => {
> > +    SpecialPowers.pushPrefEnv({"set": [['webextensions.tests', true]]}, resolve);
> 
> I don't understand why you only need to do this for test_ext_simple.html.

Because that's the only test that currently passes and needs to load a resource from the zip. That will likely change once I turn document_start to document_idle.
Depends on: 1211401
Depends on: 1225463
No longer depends on: 1225463
Attached patch ext-tests-b2g.patch (obsolete) — Splinter Review
Rebased patch.

Bill, to test with the emulator, clone the main B2G repo and use the KitKat x86 configuration:
./config.sh emulator-x86-kk

You can configure the build to use your own gecko directory by creating a .userconfig file and adding GECKO_PATH=... in it.

then ./build.sh

To run tests, stay in $B2G and run |./mach mochitest test_ext_sendmessage_reply.html|
Attachment #8687398 - Attachment is obsolete: true
Attachment #8687398 - Flags: review?(wmccloskey)
I spent a couple hours trying to get both the x86 and ARM emulators working, but I keep running into problems. We'll have to figure out a different approach. Will you be in the office on Monday?
(In reply to Bill McCloskey (:billm) from comment #5)
> I spent a couple hours trying to get both the x86 and ARM emulators working,
> but I keep running into problems. We'll have to figure out a different
> approach. Will you be in the office on Monday?

Yes!
Fabrice, I'm seeing an issue where we seem to be loading test extensions more than once. This doesn't seem to be affecting whether the tests pass, but it's worrisome. We seem to be registering multiple listeners for the SPStartupExtension messages. The special powers code is really hard to follow. Would you mind looking into this? I think it should be reproducible by dump()ing from the SPStartupExtension message handler and noticing that it happens multiple times per test, even when we only load one extension in the test.
I needed this additional patch for b2g.
Attachment #8691121 - Flags: review?(fabrice)
(In reply to Bill McCloskey (:billm) from comment #8)
> Created attachment 8691121 [details] [diff] [review]
> message manager patch
> 
> I needed this additional patch for b2g.

What did that fix for you? I haven't seen any new tests passing with this patch.
I saw some console errors. We need this change because b2g doesn't use the browser.xml XBL binding, so browser.messageManager is undefined if browser is an <iframe mozbrowser>.
Attachment #8691121 - Flags: review?(fabrice) → review+
That enables a good number of tests on emulators. Most failures are expected (unimplemented apis), except the xhr permission one. But I'm getting tired rebasing that stuff, so I'd like to land as is and fix remaining issues in follow ups.


Green try build is at https://treeherder.mozilla.org/#/jobs?repo=try&revision=6a6e0c82916a
Attachment #8689837 - Attachment is obsolete: true
Attachment #8704224 - Flags: review?(wmccloskey)
Comment on attachment 8704224 [details] [diff] [review]
ext-tests-b2g.patch v2

Review of attachment 8704224 [details] [diff] [review]:
-----------------------------------------------------------------

::: toolkit/components/extensions/Extension.jsm
@@ +912,5 @@
>  
>    broadcast(msg, data) {
>      return new Promise(resolve => {
>        let count = Services.ppmm.childCount;
> +      if (AppConstants.MOZ_NUWA_PROCESS) {

I'd really like to see a fix for this at the platform level.
Attachment #8704224 - Flags: review?(wmccloskey) → review+
Depends on: 1237109
https://hg.mozilla.org/mozilla-central/rev/f4f332578a96
https://hg.mozilla.org/mozilla-central/rev/ee9fc98d08f6
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → 2.6 S5 - 1/15
You need to log in before you can comment on or make changes to this bug.