1238576 - Stop exposing navigator.mozApps on desktop and Android

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Comment 1

[Alex Vincent [:WeirdAl]]

Do we want to preserve navigator.mozApps for any existing platforms?  If not, I can do this pretty easily. (I just had to do something similar for the company I work for.)

Comment 2

[[:fabrice] Fabrice Desré]

Yes, it needs to be available on all products where MOZ_B2G is set.

Comment 3

[Myk Melez [:myk] [@mykmelez]]

Attachment: disable mozApps API and restrict tests to Mulet

Here's a work-in-progress patch that disables navigator.mozApps unless MOZ_B2G is defined and restricts dom/apps/tests/ to Mulet, where those tests currently work.

The patch also restricts dom/security/test/csp/test_bug949549.html to Mulet, since it uses navigator.mozApps.  And it disables two tests in extensions/cookie/test/, since they use navigator.mozApps but fail on Mulet.

This is a work-in-progress because there are a variety of other tests around the tree that will need to be disabled or restricted to a build target that can run them, per this DXR search:

https://dxr.mozilla.org/mozilla-central/search?q=navigator.mozApps+path%3Atest&redirect=true&case=false

Here's a try run that should show the failures:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=4421dde32da4

Fabrice: I'm asking for feedback on the general approach of disabling mozApps unless MOZ_B2G is defined and then either running tests in Mulet or disabling them.  Is that a reasonable way to resolve this bug?

(I don't want to cavalierly disable mozApps tests, but at the same time I don't want to chase down every Mulet issue that causes a test to fail, nor figure out how to run these tests on other MOZ_B2G targets.)

Comment 4

[Myk Melez [:myk] [@mykmelez]]

Attachment: disable mozApps API and restrict more tests to Mulet

Here's an updated patch with more test fixes.

Comment 5

[Myk Melez [:myk] [@mykmelez]]

Attachment: updated patch to disable mozApps API, with many more test fixes

Here's an updated patch with many more test fixes, plus another try run:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=65a29526f546

Comment 6

[Myk Melez [:myk] [@mykmelez]]

Attachment: disable mozApps API and update tests

Here's a working patch to disable the mozApps API on desktop and Android.  It updates tests to either stop accessing that API (where the API is incidental to the test) or to run on compatible targets (B2G and/or Mulet).

Here's the try run for this patch:

  https://treeherder.mozilla.org/#/jobs?repo=try&revision=e05b5ff996c0

It runs all mochitests, plus crashtests (because one of them accesses mozApps).

This patch touches mostly files in dom/, and a few in extensions/cookie/, so I'm requesting primary review from Ehsan, who is a peer for both the Document Object Model and the Cookies and Permissions modules, for those files:

  dom/activities/tests/mochi/mochitest.ini
  dom/apps/tests/chrome.ini
  dom/apps/tests/mochitest.ini
  dom/base/test/test_navigator_resolve_identity.html
  dom/base/test/test_navigator_resolve_identity_xrays.xul
  dom/bindings/test/test_bug707564-chrome.html
  dom/bindings/test/test_bug707564.html
  dom/broadcastchannel/tests/mochitest.ini
  dom/browser-element/mochitest/chrome.ini
  dom/browser-element/mochitest/mochitest-oop.ini
  dom/browser-element/mochitest/mochitest.ini
  dom/cache/test/mochitest/mochitest.ini
  dom/datastore/tests/mochitest.ini
  dom/indexedDB/test/mochitest.ini
  dom/ipc/tests/mochitest.ini
  dom/media/test/crashtests/crashtests.list
  dom/messages/test/chrome.ini
  dom/permission/tests/mochitest.ini
  dom/requestsync/tests/mochitest.ini
  dom/security/test/csp/mochitest.ini
  dom/tests/mochitest/fetch/mochitest.ini
  dom/tests/mochitest/localstorage/chrome.ini
  dom/tests/mochitest/localstorage/test_app_uninstall.html
  dom/tests/mochitest/notification/mochitest.ini
  dom/webidl/moz.build
  extensions/cookie/test/chrome.ini
  extensions/cookie/test/test_app_uninstall_cookies.html
  extensions/cookie/test/test_app_uninstall_permissions.html

I'm also requesting review from ochameau for the Devtools change:

  devtools/shared/apps/tests/mochitest.ini

From bz for XPConnect:

  js/xpconnect/tests/chrome/test_bug853283.xul

From Michal for Necko:

  netwerk/test/mochitests/mochitest.ini

From jmaher for Testing Infrastructure:

  testing/mochitest/tests/Harness_sanity/mochitest.ini

And from Marco for Desktop Runtime:

  toolkit/webapps/tests/chrome.ini

I'd also still appreciate feedback from Fabrice (and gerard-majax, per a recommendation from jryans) on the general strategy of enabling tests on Mulet where it's possible to run them there!

Comment 7

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8716118 [details] [diff] [review]
disable mozApps API and update tests

r=me on the test_bug853283.xul changes, though this test is really not testing the codepath it was trying to test, not least because I don't think that codepath is reachable anymore.  Thank goodness.

Comment 8

[Marco Castelluccio [:marco]]

Comment on attachment 8716118 [details] [diff] [review]
disable mozApps API and update tests

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

r+ for toolkit/webapps/tests/chrome.ini

::: dom/apps/tests/chrome.ini
@@ +1,2 @@
>  [DEFAULT]
> +skip-if = buildapp == 'b2g' || os == 'android' || buildapp != 'mulet'

Nit: buildapp != 'mulet' should suffice

Comment 9

[Joel Maher ( :jmaher ) (UTC -8)]

Comment on attachment 8716118 [details] [diff] [review]
disable mozApps API and update tests

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

great cleanup.  A few nits about condition logic- all of the same theme.

::: devtools/shared/apps/tests/mochitest.ini
@@ +3,5 @@
>    debugger-protocol-helper.js
>    redirect.sjs
>  
>  [test_webapps_actor.html]
> +skip-if = toolkit == 'android' || (buildapp != 'b2g' && buildapp != 'mulet')

couldn't this just be written as:
skip-if = (buildapp != 'b2g' && buildapp != 'mulet')

and ideally in the [DEFAULT] section

::: dom/activities/tests/mochi/mochitest.ini
@@ +1,2 @@
>  [DEFAULT]
> +skip-if = e10s || os == 'android' || (buildapp != 'b2g' && buildapp != 'mulet')

couldn't this just be written as:
skip-if = (buildapp != 'b2g' && buildapp != 'mulet')

::: dom/cache/test/mochitest/mochitest.ini
@@ +42,5 @@
>    skip-if = buildapp == 'b2g' # bug 1162353
>  [test_cache_restart.html]
>  [test_cache_shrink.html]
>  [test_cache_clear_on_app_uninstall.html]
> +  skip-if = e10s || buildapp != 'mulet' # bug 1178685

the mentioned bug doesn't do much to explain why we skip on buildapp == b2g||mulet

::: dom/datastore/tests/mochitest.ini
@@ +1,2 @@
>  [DEFAULT]
> +skip-if = e10s || (buildapp != 'b2g' && buildapp != 'mulet')

I don't think you need the e10s in the condition.  Do we have e10s and non-e10s b2g|mulet builds we run tests on?

::: dom/indexedDB/test/mochitest.ini
@@ +360,5 @@
>  skip-if = (buildapp == 'b2g' && toolkit != 'gonk') # Bug 931116
>  [test_webapp_clearBrowserData_inproc_inproc.html]
>  # The clearBrowserData tests are only supposed to run in the main process.
>  # They currently time out on android.
> +skip-if = buildapp != 'mulet' || e10s || toolkit == 'android'

these won't be running on desktop builds anymore.  Couldn't we write this as:
skip-if = buildapp != 'mulet'

::: dom/tests/mochitest/fetch/mochitest.ini
@@ +24,5 @@
>  skip-if = buildapp == 'b2g' # Bug 1137683
>  [test_headers_mainthread.html]
>  skip-if = (e10s && debug && os == 'win')
>  [test_fetch_app_protocol.html]
> +skip-if = (e10s && debug && os == 'win') || (buildapp != 'b2g' && buildapp != 'mulet')

shouldn't this be:
skip-if = (buildapp != 'b2g' && buildapp != 'mulet')

Comment 10

[Myk Melez [:myk] [@mykmelez]]

(In reply to Marco Castelluccio [:marco] from comment #8)
> ::: dom/apps/tests/chrome.ini
> @@ +1,2 @@
> >  [DEFAULT]
> > +skip-if = buildapp == 'b2g' || os == 'android' || buildapp != 'mulet'
> 
> Nit: buildapp != 'mulet' should suffice

Indeed, fixed on my branch in https://github.com/mykmelez/gecko-dev/commit/926c25a.


(In reply to Joel Maher (:jmaher) from comment #9)
> ::: devtools/shared/apps/tests/mochitest.ini
> @@ +3,5 @@
> >    debugger-protocol-helper.js
> >    redirect.sjs
> >  
> >  [test_webapps_actor.html]
> > +skip-if = toolkit == 'android' || (buildapp != 'b2g' && buildapp != 'mulet')
> 
> couldn't this just be written as:
> skip-if = (buildapp != 'b2g' && buildapp != 'mulet')

Ah, indeed.  I was thinking that toolkit == 'android' and buildapp == 'b2g' for b2gdroid, but buildapp should actually be 'mobile/android/b2gdroid' for b2gdroid.

> and ideally in the [DEFAULT] section

Both fixed in https://github.com/mykmelez/gecko-dev/commit/6d00a07.


> ::: dom/activities/tests/mochi/mochitest.ini
> @@ +1,2 @@
> >  [DEFAULT]
> > +skip-if = e10s || os == 'android' || (buildapp != 'b2g' && buildapp != 'mulet')
> 
> couldn't this just be written as:
> skip-if = (buildapp != 'b2g' && buildapp != 'mulet')

Yes, fixed in https://github.com/mykmelez/gecko-dev/commit/77b905d.


> ::: dom/cache/test/mochitest/mochitest.ini
> @@ +42,5 @@
> >    skip-if = buildapp == 'b2g' # bug 1162353
> >  [test_cache_restart.html]
> >  [test_cache_shrink.html]
> >  [test_cache_clear_on_app_uninstall.html]
> > +  skip-if = e10s || buildapp != 'mulet' # bug 1178685
> 
> the mentioned bug doesn't do much to explain why we skip on buildapp ==
> b2g||mulet

Indeed (although we've only been skipping b2g, not mulet).  I suspect the bug references the "e10s" conditional, which we could remove, since we aren't building with e10s on Mulet.  But I've left it in, since that bug is still open, and reversed the order of the conditionals to make it clearer that the bug refers to the "e10s" one.

https://github.com/mykmelez/gecko-dev/commit/41c324a


> ::: dom/datastore/tests/mochitest.ini
> @@ +1,2 @@
> >  [DEFAULT]
> > +skip-if = e10s || (buildapp != 'b2g' && buildapp != 'mulet')
> 
> I don't think you need the e10s in the condition.  Do we have e10s and
> non-e10s b2g|mulet builds we run tests on?

I don't think so, so I've removed it.

https://github.com/mykmelez/gecko-dev/commit/d161a80


> ::: dom/indexedDB/test/mochitest.ini
> @@ +360,5 @@
> >  skip-if = (buildapp == 'b2g' && toolkit != 'gonk') # Bug 931116
> >  [test_webapp_clearBrowserData_inproc_inproc.html]
> >  # The clearBrowserData tests are only supposed to run in the main process.
> >  # They currently time out on android.
> > +skip-if = buildapp != 'mulet' || e10s || toolkit == 'android'
> 
> these won't be running on desktop builds anymore.  Couldn't we write this as:
> skip-if = buildapp != 'mulet'

Yep.

https://github.com/mykmelez/gecko-dev/commit/997cd70


> ::: dom/tests/mochitest/fetch/mochitest.ini
> @@ +24,5 @@
> >  skip-if = buildapp == 'b2g' # Bug 1137683
> >  [test_headers_mainthread.html]
> >  skip-if = (e10s && debug && os == 'win')
> >  [test_fetch_app_protocol.html]
> > +skip-if = (e10s && debug && os == 'win') || (buildapp != 'b2g' && buildapp != 'mulet')
> 
> shouldn't this be:
> skip-if = (buildapp != 'b2g' && buildapp != 'mulet')

Indubitably.

https://github.com/mykmelez/gecko-dev/commit/21a2176

Comment 11

[Joel Maher ( :jmaher ) (UTC -8)]

awesome, thanks for addressing these!

Comment 12

[[:fabrice] Fabrice Desré]

Comment on attachment 8716118 [details] [diff] [review]
disable mozApps API and update tests

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

Myk, can you try shipping http://mxr.mozilla.org/mozilla-central/source/dom/moz.build#44 when MOZ_B2G is defined? I'm fine with keeping it, but also curious to know how much breakage we'll get.

Comment 13

[(no longer active)]

Comment on attachment 8716118 [details] [diff] [review]
disable mozApps API and update tests

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

Nice!

Comment 14

[Myk Melez [:myk] [@mykmelez]]

(In reply to [:fabrice] Fabrice Desré from comment #12)
> Myk, can you try shipping
> http://mxr.mozilla.org/mozilla-central/source/dom/moz.build#44 when MOZ_B2G
> is defined? I'm fine with keeping it, but also curious to know how much
> breakage we'll get.

There was build bustage when I stopped building dom/apps/ unless MOZ_B2G.  I also had to exclude some WebIDL files.  Now I'm building successfully, but I'm still unsure if there are other consequences.  So I filed bug 1246721 to explore this as a followup.

Comment 15

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/7f5e4c6bfb07

Comment 16

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/7f5e4c6bfb07

Comment 17

[Chris Mills [:cmills]]

I've marked all the MDN pages related to this functionality in the same way as the marketplace docs that are being deprecated:

https://developer.mozilla.org/en-US/docs/Mozilla/Firefox_OS/API/App_installation_and_management_APIs
https://developer.mozilla.org/en-US/docs/Mozilla/Firefox_OS/API/DOMApplicationsRegistry
https://developer.mozilla.org/en-US/docs/Mozilla/Firefox_OS/API/DOMApplicationsRegistry/mgmt
https://developer.mozilla.org/en-US/docs/Mozilla/Firefox_OS/API/DOMApplicationsRegistry/checkInstalled
https://developer.mozilla.org/en-US/docs/Mozilla/Firefox_OS/API/DOMApplicationsRegistry/install
https://developer.mozilla.org/en-US/docs/Mozilla/Firefox_OS/API/DOMApplicationsRegistry/installPackage
https://developer.mozilla.org/en-US/docs/Mozilla/Firefox_OS/API/DOMApplicationsRegistry/getSelf
https://developer.mozilla.org/en-US/docs/Mozilla/Firefox_OS/API/DOMApplicationsRegistry/getInstalled

https://developer.mozilla.org/en-US/docs/Mozilla/Firefox_OS/API/DOMApplicationsManager
https://developer.mozilla.org/en-US/docs/Mozilla/Firefox_OS/API/DOMApplicationsManager/onenabledstatechange
https://developer.mozilla.org/en-US/docs/Mozilla/Firefox_OS/API/DOMApplicationsManager/getAll
https://developer.mozilla.org/en-US/docs/Mozilla/Firefox_OS/API/DOMApplicationsManager/setEnabled

When March 29th comes around, I'll update the compat tables on all of these to remove support information for non-Firefox OS platforms.

I've also added a note to the Gecko 47 release notes:

https://developer.mozilla.org/en-US/Firefox/Releases/47#Others

Comment 18

[Jan Varga [:janv]]

(In reply to Myk Melez [:myk] [@mykmelez] from comment #10)
> (In reply to Marco Castelluccio [:marco] from comment #8)
> > ::: dom/indexedDB/test/mochitest.ini
> > @@ +360,5 @@
> > >  skip-if = (buildapp == 'b2g' && toolkit != 'gonk') # Bug 931116
> > >  [test_webapp_clearBrowserData_inproc_inproc.html]
> > >  # The clearBrowserData tests are only supposed to run in the main process.
> > >  # They currently time out on android.
> > > +skip-if = buildapp != 'mulet' || e10s || toolkit == 'android'
> > 
> > these won't be running on desktop builds anymore.  Couldn't we write this as:
> > skip-if = buildapp != 'mulet'
> 
> Yep.
> 
> https://github.com/mykmelez/gecko-dev/commit/997cd70

I was about to run indexedDB tests for clearing origins and I found out they are completely disabled (except mulet). The same applies to localStorage and other APIs that needed to separate data for apps.
I believe these tests are still useful for testing separation for different origin attributes like contextId, etc.
These tests were disabled just because they used navigator.mozApps for clearing data.
I'm going to try to re-enable these tests for indexedDB ...

Comment 19

[[:fabrice] Fabrice Desré]

dom/apps will go away soon, so if you re-enable these tests, please find a different way to clear data.

Comment 20

[Jan Varga [:janv]]

(In reply to [:fabrice] Fabrice Desré from comment #19)
> dom/apps will go away soon, so if you re-enable these tests, please find a
> different way to clear data.

Sure, I'm not going to use mozApps for that, I just wanted to warn others that these tests are disabled.