Closed Bug 1238576 Opened 9 years ago Closed 9 years ago

Stop exposing navigator.mozApps on desktop and Android

Categories

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

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla47
Tracking Status
firefox47 --- fixed

People

(Reporter: ehsan.akhgari, Assigned: myk)

References

Details

(Keywords: dev-doc-complete, site-compat)

Attachments

(1 file, 3 obsolete files)

No description provided.
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.)
Yes, it needs to be available on all products where MOZ_B2G is set.
Blocks: 1235869
Keywords: site-compat
Depends on: 1242899
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.)
Assignee: nobody → myk
Status: NEW → ASSIGNED
Attachment #8714580 - Flags: feedback?(fabrice)
Blocks: 1245189
Blocks: 1245190
Blocks: 1245199
Here's an updated patch with more test fixes.
Attachment #8714580 - Attachment is obsolete: true
Attachment #8714580 - Flags: feedback?(fabrice)
Attachment #8715102 - Flags: feedback?(fabrice)
Here's an updated patch with many more test fixes, plus another try run: https://treeherder.mozilla.org/#/jobs?repo=try&revision=65a29526f546
Attachment #8715102 - Attachment is obsolete: true
Attachment #8715102 - Flags: feedback?(fabrice)
Attachment #8715555 - Flags: feedback?(fabrice)
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!
Attachment #8715555 - Attachment is obsolete: true
Attachment #8715555 - Flags: feedback?(fabrice)
Attachment #8716118 - Flags: review?(poirot.alex)
Attachment #8716118 - Flags: review?(michal.novotny)
Attachment #8716118 - Flags: review?(mcastelluccio)
Attachment #8716118 - Flags: review?(jmaher)
Attachment #8716118 - Flags: review?(ehsan)
Attachment #8716118 - Flags: review?(bzbarsky)
Attachment #8716118 - Flags: feedback?(lissyx+mozillians)
Attachment #8716118 - Flags: feedback?(fabrice)
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.
Attachment #8716118 - Flags: review?(bzbarsky) → review+
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
Attachment #8716118 - Flags: review?(mcastelluccio) → review+
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')
Attachment #8716118 - Flags: review?(jmaher) → review+
(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
awesome, thanks for addressing these!
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.
Attachment #8716118 - Flags: feedback?(fabrice) → feedback+
Comment on attachment 8716118 [details] [diff] [review] disable mozApps API and update tests Review of attachment 8716118 [details] [diff] [review]: ----------------------------------------------------------------- Nice!
Attachment #8716118 - Flags: review?(ehsan) → review+
Attachment #8716118 - Flags: feedback?(lissyx+mozillians) → feedback+
Attachment #8716118 - Flags: review?(poirot.alex) → review+
Blocks: 1246721
(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.
Attachment #8716118 - Flags: review?(michal.novotny) → review+
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla47
Blocks: 1252635
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
(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 ...
dom/apps will go away soon, so if you re-enable these tests, please find a different way to clear data.
(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.
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: