1097468 - expose |homescreen-webapps-manage| via webidl

Status: RESOLVED FIXED

(Core Graveyard :: DOM: Apps, defect)

Description

[Junior [inactive]]

Since the mozApps API was converted to use webidl in bug 899322, permission checking in apps.webidl should be discussed to let privileged app use mozApps.mgmt

Comment 1

[Kevin Grandon :kgrandon]

Hey Junior - Is this bug to follow-up with the patch here: https://bug1000313.bugzilla.mozilla.org/attachment.cgi?id=8521141 ?

Comment 2

[Junior [inactive]]

(In reply to Kevin Grandon :kgrandon (In Europe/Conf until 11/24) from comment #1)
> Hey Junior - Is this bug to follow-up with the patch here:
> https://bug1000313.bugzilla.mozilla.org/attachment.cgi?id=8521141 ?
Yes, but it's a WIP patch.

I'd like to claim explictly in webidl what's done in the patch of bug 1000313:

Expose to |homescreen-webapps-manage|
  DOMRequest getAll();
  DOMRequest uninstall(DOMApplication app);
  attribute EventHandler oninstall;
  attribute EventHandler onuninstall;
  attribute EventHandler onenabledstatechange;

NOT Expose to |homescreen-webapps-manage|
  DOMRequest getNotInstalled();
  void applyDownload(DOMApplication app);
  Promise<DOMApplication> import(Blob blob);
  Promise<any> extractManifest(Blob blob);
  void setEnabled(DOMApplication app, boolean state);

Fabrice, I'd like to double-check if non-exposure of import/enable API is what we want. Could you please confirm that?

Comment 3

[[:fabrice] Fabrice Desré]

Yes, that's what we want for now.

Comment 4

[Junior [inactive]]

A short question:
In Apps.webidl, |mozApps.mgmt| is of DOMApplicationsManager, which is [ChromeOnly].
However, functions such as |getAll| should be exposed to privileged apps with |homescreen-webapps-manage| permission.
Should I remove [ChromeOnly] from |DOMApplicationsManager| and add [ChromeOnly] to methods like |getNotInstalled|? 

Code Snippet as
[JSImplementation="@mozilla.org/webapps/manager;1"]
interface DOMApplicationsManager : EventTarget {

  [CheckPermissions="webapps-manage homescreen-webapps-manage"]   
  DOMRequest getAll();

  [ChromeOnly,
  CheckPermissions="webapps-manage]
  DOMRequest getNotInstalled();

Comment 5

[[:fabrice] Fabrice Desré]

No, don't change the [ChromeOnly] on DOMApplicationsManager. It's there to prevent the interface to be exposed on the global.  That would also make no sense to have getNotInstalled be chrome-only.

I would focus only on the permissions.

Comment 6

[Junior [inactive]]

Attachment: Part1: expose |homescreen-webapps-manage| in Apps.webidl

The patch is for the webidl change.
Will add tests in the next part after the change is agreed.

Comment 7

[Kevin Grandon :kgrandon]

Any update here? Can anyone else review this code if Jonas is busy?

Comment 8

[Junior [inactive]]

Since the change is granted, I'll add some tests.

Comment 9

[Kevin Grandon :kgrandon]

Junior - I tried with the patch, but I'm seeing an error, though the error details have changed. Any idea if I'm doing something wrong? Here is the error I receive:

[JavaScript Error: "TypeError: Return value of DOMApplicationsRegistry.mgmt is not an object."]

Comment 10

[[:fabrice] Fabrice Desré]

Is the pref/setting mapping at http://mxr.mozilla.org/mozilla-central/source/b2g/chrome/content/settings.js#82 working properly? It is used to decide whether we expose mgmt at http://dxr.mozilla.org/mozilla-central/source/dom/apps/Webapps.js#269

Comment 11

[Kevin Grandon :kgrandon]

Ah, ok that's probably it. I don't think this is going to work for third party developers then as you can't use WebIDE to develop home screens. Let's figure that out in another bug. Thanks for chiming in.

Comment 12

[Junior [inactive]]

Fabrice and comment of bug 1000313[1] has explained.

[1] https://bugzilla.mozilla.org/show_bug.cgi?id=1000313#c50
    https://bugzilla.mozilla.org/show_bug.cgi?id=1000313#c52

Comment 13

[Junior [inactive]]

Attachment: Part1: expose |homescreen-webapps-manage| in Apps.webidl

Rebase, carry r+

Comment 14

[Junior [inactive]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=4f25a7cb4fad
Try seems fine. I'm going to write test

Comment 15

[Junior [inactive]]

Two questions I have encountered:
1. inproc-app does not have its own permission. permissions are kinda per process.
2. fail to launch a signed app in mochitest-plain

Therefore I'll try to launch a oop app and push permission by special power

Comment 16

[Junior [inactive]]

Attachment: Part 2: test

test for permission |homescreen-webapps-manage|

I encountered lots of wired things while cooking this test.
Will describe them later.

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

Comment 17

[[:fabrice] Fabrice Desré]

Comment on attachment 8584419 [details] [diff] [review]
Part 2: test

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

r=me with nit fixed. thanks!

::: dom/apps/tests/test_third_party_homescreen.html
@@ +21,5 @@
> +function runApp(aApp, aCallback) {
> +  var ifr = document.createElement('iframe');
> +  ifr.setAttribute('mozbrowser', 'true');
> +  ifr.setAttribute('mozapp', aApp.manifestURL);
> +  ifr.src = aApp.origin + aApp.manifest.launch_path;

you need to resolve the launch_path against the app manifest url, not append to the origin.

@@ +171,5 @@
> +  info("Running " + app.manifestURL);
> +  runApp(app, continueTest);
> +  yield undefined;
> +
> +  SpecialPowers.popPermissions(continueTest);

I don't think you need to call that explicitly. The harness should do it for you when the test finishes.

Comment 18

[Junior [inactive]]

> @@ +171,5 @@
> > +  info("Running " + app.manifestURL);
> > +  runApp(app, continueTest);
> > +  yield undefined;
> > +
> > +  SpecialPowers.popPermissions(continueTest);
> 
> I don't think you need to call that explicitly. The harness should do it for
> you when the test finishes.
It's a weird fact:
If we don't call the |popPermissions| explicitly, the test will get stucked in |flushPermissions| in
https://dxr.mozilla.org/mozilla-central/source/testing/mochitest/tests/SimpleTest/SimpleTest.js#1018

I haven't found the root cause, but I believe something went wrong if I push permissions in an inproc iframe.

Comment 19

[Junior [inactive]]

I got the root cause of comment 18.
Uninstall an app will remove all the permissions for the app.
But |flushPermissions| needs a |perm-changes| notification to invoke the callback
https://dxr.mozilla.org/mozilla-central/source/testing/specialpowers/content/specialpowersAPI.js#876

Thus I'd like to keep the |popPermissions| before unstalling the app.
Is that OK, Fabrice?

Comment 20

[Junior [inactive]]

Attachment: Part2: test

Modify by comment 17 but keep the |popPermissions|
Carry r+

Btw, I list some facts I found:
1. The permissions of in-proc package app inherits from its embedder. Per Bug 1059662 comment 0, it's like a known issue.

2. I can not load a uri from a package app in mochitest-plain, even with webapps-manage permission. That's weird since I can load by handy push the uri to the awesome bar.

3. Inner page can not get |mozApps.mgmt|, thus causing me use |loadFrameScript|.
I guess maybe it's blocked since the mozapp iframe is a hosted app. Not sure.

4. comment 18 and comment 19 describe a error-prone way if the permission pushed by |SpecialPowers.pushPermissions| is undone before the test finishes

Comment 21

[[:fabrice] Fabrice Desré]

(In reply to Junior Hsu [:juniorhsu] from comment #19)
> I got the root cause of comment 18.
> Uninstall an app will remove all the permissions for the app.
> But |flushPermissions| needs a |perm-changes| notification to invoke the
> callback
> https://dxr.mozilla.org/mozilla-central/source/testing/specialpowers/content/
> specialpowersAPI.js#876
> 
> Thus I'd like to keep the |popPermissions| before unstalling the app.
> Is that OK, Fabrice?

Yep, it's fine for me.

Comment 22

[Carsten Book [:Tomcat]]

Hi, Part 1 failed to apply:

renamed 1097468 -> homescreen-webapps-manage
applying homescreen-webapps-manage
patching file dom/webidl/Apps.webidl
Hunk #2 FAILED at 104
1 out of 2 hunks FAILED -- saving rejects to file dom/webidl/Apps.webidl.rej
patch failed, unable to continue (try -v)
patch failed, rejects left in working dir
errors during apply, please fix and refresh homescreen-webapps-manage

could you take a look, thanks!

Comment 23

[Junior [inactive]]

Attachment: Part1: expose |homescreen-webapps-manage| in Apps.webidl

Rebase, carry r+.

Comment 24

[Junior [inactive]]

Attachment: Part2: test

rebase, carry r+

Comment 25

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/3a626a11ac38
https://hg.mozilla.org/integration/mozilla-inbound/rev/57d347aa0701

Comment 26

[Carsten Book [:Tomcat]]

sorry had to back this out for test failures like https://treeherder.mozilla.org/logviewer.html#?job_id=8338752&repo=mozilla-inbound

Comment 27

[Junior [inactive]]

Attachment: Part2: test

test failures since bug 1059662 blocks embedding inproc app in content process.
Skip the test case.

Wait for try happy
https://treeherder.mozilla.org/#/jobs?repo=try&revision=dcb07319e500

Comment 28

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/b2g-inbound/rev/961fe9c61047
https://hg.mozilla.org/integration/b2g-inbound/rev/61480347343d

Comment 29

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/961fe9c61047
https://hg.mozilla.org/mozilla-central/rev/61480347343d