1224703 - Enable WebExtensions tests on emulator

Status: RESOLVED FIXED

(Firefox OS Graveyard :: General, defect)

Comment 1

[[:fabrice] Fabrice Desré]

Attachment: ext-tests-b2g.patch

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).

Comment 2

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

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.

Comment 3

[[:fabrice] Fabrice Desré]

(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.

Comment 4

[[:fabrice] Fabrice Desré]

Attachment: ext-tests-b2g.patch

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|

Comment 5

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

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?

Comment 6

[[:fabrice] Fabrice Desré]

(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!

Comment 7

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

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.

Comment 8

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Attachment: message manager patch

I needed this additional patch for b2g.

Comment 9

[[:fabrice] Fabrice Desré]

(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.

Comment 10

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

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>.

Comment 11

[[:fabrice] Fabrice Desré]

Attachment: ext-tests-b2g.patch v2

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

Comment 12

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

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.

Comment 13

[Pulsebot]

https://hg.mozilla.org/integration/b2g-inbound/rev/f4f332578a96
https://hg.mozilla.org/integration/b2g-inbound/rev/ee9fc98d08f6

Comment 14

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/f4f332578a96
https://hg.mozilla.org/mozilla-central/rev/ee9fc98d08f6