826058 - Write tests for app install/update

Status: RESOLVED FIXED

(Core Graveyard :: DOM: Apps, defect)

Description

[Bobby Holley (:bholley)]

sicking wants this to catch bugs like bug 823189 and bug 824697.

We want to test this for appcach-ed hosted apps as well as packaged apps, probably separately. sicking wants the following steps:

1. Install an app. Check that the correct events fire and that all the state on the app object is correct
2. Ask the app to check for update. Check that the correct events fire and that all the state on the app object is correct
3. Update the app serverside (by modifying state in the server using sjs)
4. repeat 2
5. Tell the API to download the update. Check events/state during download and at end of download
6. Apply (i.e. install) the download
7. repeat 2

We'll also want to run the app after installation to make sure it works properly.

Comment 1

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Oh, another thing we need tests for is error handling. I.e. test that the right thing happens if the update ping returns an invalid response, or if the app download is aborted half-way through. We can definitely do this in followups though if that's easier.

Comment 3

[Bobby Holley (:bholley)]

Attachment: Hosted app install/update tests. WIP

These are the tests as they exist thus far. I don't have any time left to work on
these unfortunately, so someone else will have to pick them up. There's a lot of
useful machinery in here, so they're worth picking up IMO.

Currently, there are a number of issues:
* I tried getting these running on Desktop b2g (via bug 826111), but encountered
  breakage in BrowserElementPromptService that prevented the app from
  communicating with the parent.
* Uninstalling is broken. See bug 829726.
* When uninstall workings, the comments around a large section of the test can be
  removed. When they are, you'll hit a failure that is bug 827908.
* The tests stall when checking for updates. I've filed bug 829763.

Comment 4

[Jason Smith [:jsmith]]

Thanks Dale for helping out here. Much appreciated.

Comment 5

[Dale Harvey (:daleharvey)]

Attachment: Tests Passing in b2g - WIP

Comment 6

[Dale Harvey (:daleharvey)]

These are now passing in b2g desktop, the "dom.mozBrowserFramesEnabled" pref needed enabled there were a few small changes, there are failures in firefox builds

> 12 ERROR TEST-UNEXPECTED-FAIL | /tests/dom/apps/tests/test_app_update.html | App is not installed
> 17 ERROR TEST-UNEXPECTED-FAIL | /tests/dom/apps/tests/test_app_update.html | App is not installed
> 23 ERROR TEST-UNEXPECTED-FAIL | /tests/dom/apps/tests/test_app_update.html | App is not installed
> 42 ERROR TEST-UNEXPECTED-FAIL | /tests/dom/apps/tests/test_app_update.html | App is not installed
> 48 ERROR TEST-UNEXPECTED-FAIL | /tests/dom/apps/tests/test_app_update.html | lastUpdateCheck updated appropriately

Checking that now.

https://bugzilla.mozilla.org/show_bug.cgi?id=826146 also needs uplifted with a rebase, will send along a patch

Comment 7

[Dale Harvey (:daleharvey)]

Attachment: Hosted app install/update tests.

There is 1 TODO which is a genuine bug I am filing, the rest pass

This will only work tested against b2g desktop

Comment 8

[Dale Harvey (:daleharvey)]

Also should note the bug I linked previously has been uplifted, this is the only patch needed

Comment 9

[Dale Harvey (:daleharvey)]

It was mentioned in https://bugzilla.mozilla.org/show_bug.cgi?id=827908 that we could have these working on desktop firefox builds using the same workaround as 

http://mxr.mozilla.org/mozilla-central/source/dom/tests/mochitest/webapps/head.js#4

I am confused by the original intent of the isLaunchable code, but it seems worth doing for these tests

Comment 10

[Jason Smith [:jsmith]]

(In reply to Dale Harvey (:daleharvey) from comment #9)
> It was mentioned in https://bugzilla.mozilla.org/show_bug.cgi?id=827908 that
> we could have these working on desktop firefox builds using the same
> workaround as 
> 
> http://mxr.mozilla.org/mozilla-central/source/dom/tests/mochitest/webapps/
> head.js#4
> 
> I am confused by the original intent of the isLaunchable code, but it seems
> worth doing for these tests

isLaunchable I believe was originally there because webapps on desktop do a different set of checks to determine if an app is launchable - it checks that the app is "natively" installed on the OS, where as other platforms (Android, B2G), we follow the typical rule of checking the registry.

And yes, it would be worth doing the same for these tests. These tests aren't intending to test the native install logic on desktop, so feel free to do the same here with these tests.

Comment 11

[[:fabrice] Fabrice Desré]

Comment on attachment 705203 [details] [diff] [review]
Hosted app install/update tests.

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

great!

Comment 12

[Bobby Holley (:bholley)]

FWIW, I didn't really consider these tests finished. I think a it's probably worth spending a little bit of time expanding them to check more cases, now that the infrastructure's all there.

Comment 13

[Dale Harvey (:daleharvey)]

Attachment: Hosted app install/update tests

Added SpecialPowers API to make all apps launchable, now passes in Firefox

Comment 14

[Dale Harvey (:daleharvey)]

Yeh there is definitely more testing needed, I just wasnt sure of the best workflow, get a set of tests working, commit and improve or do them all in one go.

I mentioned to fabrice earlier, happy to write more tests but I would prefer to be pointed to which cases / bugs to test for.

Comment 15

[Jason Smith [:jsmith]]

I'd actually try to get something landed such that the bare bones of framework is in the codebase, then do more tests as followups.

Btw - if you looking for something else to go after, I'd recommend tackling bug 821589. We currently don't have any gecko automation for packaged apps. So if you can get the bare bones of that framework up, that would be a huge win.

Comment 16

[Fernando Jiménez Moreno [:ferjm]]

Comment on attachment 707511 [details] [diff] [review]
Hosted app install/update tests

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

::: testing/specialpowers/content/specialpowersAPI.js
@@ +673,5 @@
>      this.pushPrefEnv({set: [['dom.mozApps.auto_confirm_install', true]]}, cb);
>    },
>  
> +
> +  originalAppsLaunableValue: null,

typo: "originalAppsLaunchableValue"

@@ +684,5 @@
> +
> +  restoreAllAppsLaunchable: function() {
> +    var Webapps = {};
> +    Components.utils.import("resource://gre/modules/Webapps.jsm", Webapps);
> +    Webapps.DOMApplicationRegistry.allAppsLaunchable = this.originalAppsLaunchableVal;

Should this be: "this.originalAppsLaunchableValue"?

Comment 17

[Jason Smith [:jsmith]]

Can we address the small nits in comment 16 and get this landed?

Comment 18

[Dale Harvey (:daleharvey)]

Yup sorry, was changes I forgot to commit, will update the patch, test again and get it landed tomorrow

Comment 19

[Dale Harvey (:daleharvey)]

Attachment: Hosted app install/update tests

Updated with SpecialPowers API to skip per platform webapp checks

Comment 20

[Dale Harvey (:daleharvey)]

Attachment: Hosted app install/update tests.

Bobby

Planning to land this and follow up with more tests, this wont work until https://bugzilla.mozilla.org/show_bug.cgi?id=837572 lands. but its passing on try aside from that. 

I wanted to check in particular that the way I added the specialpowers api here is the right way, not that familiar with that code.

Comment 21

[Bobby Holley (:bholley)]

Comment on attachment 709666 [details] [diff] [review]
Hosted app install/update tests.

You should probably flag ted or someone for the SpecialPowers stuff, I'm not a peer there.

I can't really review the tests per se given that I wrote them. If you want me to look at any changes, can you attach an interdiff?

Comment 22

[Dale Harvey (:daleharvey)]

Its just the special powers stuff that needs looked at, fabrice already r+'d the tests themselves, the actual tests themselves didnt change, just some preferences and specialpowers things. Cheers

Comment 23

[Dale Harvey (:daleharvey)]

Comment on attachment 709666 [details] [diff] [review]
Hosted app install/update tests.

The tests themselves are r+'d, wanted to ensure the new special powers api was done correctly.

Comment 24

[[:fabrice] Fabrice Desré]

Ted, can you take a look at the special powers part? We really badly need to land this. Thanks!

Comment 25

[Dale Harvey (:daleharvey)]

Preemtively pushed to try and it looks like linux isnt downloading the appcache, investigating

https://tbpl.mozilla.org/?tree=Try&rev=fd19f73af1e5

Comment 26

[Jason Smith [:jsmith]]

Clint - Can we get help here? We've got no automation in the tree for the apps install/update API and really need a review here to get this landed, so that we have some basic tests in the tree.

Comment 27

[(not currently active) Ted Mielczarek]

Comment on attachment 709666 [details] [diff] [review]
Hosted app install/update tests.

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

r- for the 127.0.0.1 bit.

::: dom/apps/tests/file_app.template.html
@@ +49,5 @@
> +  installed(!!app);
> +  if (app) {
> +    var appName = "Really Rapid Release (APPTYPETOKEN)";
> +    is(app.manifest.name, appName, "Manifest name should be correct");
> +    is(app.origin, "http://127.0.0.1:8888", "App origin should be correct");

If you're going to run these on non-desktop machines, we don't actually serve web content on 127.0.0.1.

::: dom/apps/tests/test_app_update.html
@@ +13,5 @@
> +  /** Test for Bug 826058 **/
> +
> +  SimpleTest.waitForExplicitFinish();
> +
> +  var gBaseURL = 'http://127.0.0.1:8888/tests/dom/apps/tests/';

You can't use 127.0.0.1 in Mochitests, that's not where we serve them from on Android/B2G.

::: testing/specialpowers/content/specialpowersAPI.js
@@ +734,5 @@
> +  restoreAllAppsLaunchable: function() {
> +    var Webapps = {};
> +    Components.utils.import("resource://gre/modules/Webapps.jsm", Webapps);
> +    Webapps.DOMApplicationRegistry.allAppsLaunchable = this.originalAppsLaunchableVal;
> +  },

I don't really know how these APIs work, so I can't comment very deeply on them. Just a few things to think about:
a) Will these work if called from a content process?
b) If so, will the value be set synchronously, or asynchronously in the content process (prefs are set asynchronously).

Comment 28

[Dale Harvey (:daleharvey)]

Will fix the host (curious that these tests pass on b2g with the previous try run though)

As far as I can tell preferences are set synchronously via |return(this._sendSyncMessage('SPPrefService', msg)[0])| but this api is unlikely to work when run oop, I will fix that to mirror the preferences api

Comment 29

[Josh Matthews [:jdm]]

(In reply to Dale Harvey (:daleharvey) from comment #28)
> As far as I can tell preferences are set synchronously via
> |return(this._sendSyncMessage('SPPrefService', msg)[0])| but this api is
> unlikely to work when run oop, I will fix that to mirror the preferences api

This works OOP if the code that runs immediately after does not depend on the value of the pref being correct.

Comment 30

[Dale Harvey (:daleharvey)]

What do you mean by 'immediately'? I havent seen any examples of tests that wait for preferences to be set and most of them will fail if there preferences arent

This shouldnt effect this test anyway, if we set allAppsLAunchable inside a syncMessage then it should be safe right?

Comment 31

[Josh Matthews [:jdm]]

The situation is that the message to set the preference is synchronous, so the code in the child won't continue running until the pref has been set in the parent process. However, the child doesn't see the updated value until the parent sends an update message, so code that immediately runs in the child before returning to the event loop will only see the pre-update value. Code that runs in the parent will see the updated value.

Comment 32

[Josh Matthews [:jdm]]

The correct thing to do in any case is use SpecialPowers.pushPrefEnv, which delays running the callback until the child sees the pref update: http://mxr.mozilla.org/mozilla-central/source/testing/specialpowers/content/specialpowersAPI.js?force=1#560

Comment 33

[Dale Harvey (:daleharvey)]

Attachment: Hosted app install/update tests.

I am slightly confused by the reasoning to switch from 127.0.0.1, the app is specifically testing being installed from a host that differs from where it is served from and 127.0.0.1 is in 

http://mxr.mozilla.org/mozilla-central/source/build/pgo/server-locations.txt

But switched it to another anyway.

The SpecialPrivileges API call will now work when called from oop content processes, oop cant yet be enabled for these tests. Will file a follow up bug for that.

Comment 34

[Dale Harvey (:daleharvey)]

Pushed to try: https://tbpl.mozilla.org/?tree=Try&rev=a1936bd3213b

Comment 35

[Dale Harvey (:daleharvey)]

Try push passing on all platforms, there are a bunch of failures there but all previously reported intermittents

Comment 36

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Ted: Any chance of getting a review on this fairly quickly? It'd be very nice to not have to worry about regressions in the app update code since that is very critical that it's working. Likewise, this is holding up other testing work happening in other bugs.

Comment 37

[Jason Smith [:jsmith]]

I just pinged Ted over email to see if we can get a review as soon as possible.

Comment 38

[(not currently active) Ted Mielczarek]

Comment on attachment 714161 [details] [diff] [review]
Hosted app install/update tests.

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

Thanks for those fixes. r=me

Comment 39

[cmtalbert]

(In reply to Dale Harvey (:daleharvey) from comment #35)
> Try push passing on all platforms, there are a bunch of failures there but
> all previously reported intermittents

That try push actually failed on b2g, did you realize that? It failed saying that "app is not installed"  But if this worked for you locally then it may have been an orange.

Comment 40

[Dale Harvey (:daleharvey)]

Attachment: Hosted app install/update tests

rebased

Comment 41

[Dale Harvey (:daleharvey)]

I hadnt, cheers for catching that Clint, will to another push to try to make sure, I cant reproduce locally

Comment 42

[Dale Harvey (:daleharvey)]

Update on this

The b2g failure clint noticed is a genuine bug, it seems like the special powers API to enabe the webapps-manage permission is not working correctly as the emulator hangs on assertPermission when trying to uninstall an application.

It doesnt seem to be a race condition (I just added a large timeout after adding the permissions), but its strange that this works on b2g / desktop builds and not emulator ones, it may be an oop problem, now that I have reproduced it I should get to the cause in the morning

Comment 43

[Josh Matthews [:jdm]]

Erg. Bug 685652 is required for the same reasons that pushPrefEnv was introduced - the synchronous message the sets the permission is not quite synchronous enough, since the actual permission update passes over IPDL from parent to child separately, so the child doesn't see the updated permission value until some time later in the future.

Comment 44

[Josh Matthews [:jdm]]

And yeah, this works on desktop because we disable OOP.

Comment 45

[Jason Smith [:jsmith]]

So is this bug basically blocked on bug 685652's completion then? If so, I can raise this to Clint about needing to get that bug resolved.

Comment 46

[Josh Matthews [:jdm]]

Sounds like it.

Comment 47

[Dale Harvey (:daleharvey)]

I am not so sure, as said I put a 5 second timeout after calling special powers before starting the tests and it didnt help, so I think the permission is permanently not being set, looking into it now

Comment 48

[Dale Harvey (:daleharvey)]

So I got lost looking into this, I am still sure its something fairly basic, the permission looks to be set correctly but not read (possibly off the wrong process)

I spent a long time and got lost in the permissions code, right now I dont even have a machine I can reproduce this on so I will have to unassign myself

Comment 49

[Jason Smith [:jsmith]]

Do we still need bug 685652 to fix this bug?

Comment 50

[Dale Harvey (:daleharvey)]

I am not convinced we do, at the least it needs verified by fabrice beforehand

Comment 51

[[:fabrice] Fabrice Desré]

I still can't reproduce the failure locally (I should get access to a build slave soon), but here's where we fail in try builds:

- when calling mozApps.mgmt.uninstall(), the child process recognize that the process has the webapps-manage permission, so sends an uninstall message to the parent.
- the parent, at https://mxr.mozilla.org/mozilla-central/source/dom/apps/src/Webapps.jsm#791 checks again if the child has the webapps-manage permission.
- we end up at https://mxr.mozilla.org/mozilla-central/source/dom/ipc/AppProcessChecker.cpp#35 where we look for ASSERT_APP_PROCESS_PERMISSION. The tab->IsBrowserElement() returns true, so we don't go into the switch block and return false and get killed.

Jonas && Jusin, is there any reason for not adding aType == ASSERT_APP_PROCESS_PERMISSION at https://mxr.mozilla.org/mozilla-central/source/dom/ipc/AppProcessChecker.cpp#38 ?

Comment 52

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

The way we do security checks are pretty broken :(. Definitely something that we need to apply a bigger hammer to and fix up. But let's not do that here.

I don't quite understand what you are proposing, but using anything related to ASSERT_APP_HAS_PERMISSION would be the wrong fix.

The problem is that by the time we get into this function, we have lost all sense of what the calling "origin" was. And without that we will have a very hard time using the nsIPermissionManager :(

Comment 53

[[:fabrice] Fabrice Desré]

And now that the emulator has been updated on tbpl, we are green on try:
https://tbpl.mozilla.org/?tree=Try&rev=1a1a59966b17

Comment 54

[[:fabrice] Fabrice Desré]

The test failing was M2

Comment 55

[Jason Smith [:jsmith]]

So does this mean we actually don't need bug 685652 to fix this bug then?

Comment 56

[[:fabrice] Fabrice Desré]

(In reply to Jason Smith [:jsmith] from comment #55)
> So does this mean we actually don't need bug 685652 to fix this bug then?

That's likely, yes. I'm gonna do a full try run with the "ready to land" patch instead of my debug version to make sure that we are good to land.

Comment 57

[[:fabrice] Fabrice Desré]

re-pushed to try: https://tbpl.mozilla.org/?tree=Try&rev=6c8e1030595e

Comment 58

[[:fabrice] Fabrice Desré]

So... The failures in this patch were due to the outdated emulator used by the test slaves (hence I could not repro locally). With that fixed, we still had a couple of other test failures because they were interfering with the new SpecialPowers.setAllAppsLaunchable() api from this patch.

I fixed the 2 faulty tests and got a green try build:
https://tbpl.mozilla.org/?tree=Try&rev=e095766410ceui

If no one objects, I will push this patch tomorrow.

Comment 59

[[:fabrice] Fabrice Desré]

https://hg.mozilla.org/integration/mozilla-inbound/rev/e63cb4c3e063

Comment 60

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/e63cb4c3e063