916874 - Reenable toolkit/devtools/apps/test/unit/test_webappsActor.js on !b2g

Status: RESOLVED FIXED

(Firefox Graveyard :: Web Apps, defect, P1)

Description

[Alexandre Poirot [:ochameau]]

Bug 905881 disabled completely this xpcshell test. It was expected to still run on B2G, but it doesn't.
The following xpcshell rule doesn't work as expected:
  skip-if = (os == "win" || "linux" || "mac")

It may work if we use this syntax:
  skip-if = (os == "win" || os == "linux" || os == "mac")

But various other tests are using the first syntax... so they may need to be fixed as well:
http://mxr.mozilla.org/mozilla-central/search?string=skip-if&find=xpcshell.ini&findi=&filter=^[^\0]*%24&hitlimit=&tree=mozilla-central

Also, ideally, we wouldn't just re-enable it on b2g, but also keep the webapps actor test to work on desktop as explained in bug 905881 comment 20.

Comment 1

[Marco Castelluccio [:marco]]

(In reply to Alexandre Poirot (:ochameau) from comment #0)
> Also, ideally, we wouldn't just re-enable it on b2g, but also keep the
> webapps actor test to work on desktop as explained in bug 905881 comment 20.

I think we should also disable the webapps actor on desktop, bug 914275.

Comment 2

[Alexandre Poirot [:ochameau]]

The test appears to triggers assertion in startupcache code because of this line that shouldn't have been commited:
http://mxr.mozilla.org/mozilla-central/source/toolkit/devtools/apps/tests/unit/head_apps.js#120

Thanks again to arai who figured this out immediatly!!

arai: startupCache uses ProfLDS directory for finding older cache. xpcshell tests is designed to pass temporary directory for that to make sure there is no older cache. provider in head_apps.js returns TempD for ProfLDS

Comment 4

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

Attachment: Hacks and Logs

I've spent some time trying to get this working on the b2g emulator, but I haven't had much success at all.  I'm attaching my current state in case someone has clues on how to move forward.

Here are some issues I encountered (after fixing the skip-if statement so the test actually runs):

* Webapps.jsm calls buildIDToTime, which tries to get the service nsIXULAppInfo, but this fails with the error NS_ERROR_XPC_GS_RETURNED_FAILURE
  * It seems like this *should* work, since we call updateAppInfo() just before loading Webapps.jsm, which should be able to supply this information
* There's an error while loading osfile_unix_allthreads.jsm because it defines a custom toString method
  * I don't see why this is an error, but I commented it out for now to get things moving
* Webapps.jsm calls DOMApplicationRegistry.init(), which then looks up the path to webapps.json, which causes the test to hang indefinitely
  * Once you kill the "hanging" test with Ctrl-C though, the test actually runs, in many cases passing!
  * The call to FileUtils.getFile is safe, but calling the ".path" accessor is what makes the test hang

In general, most of these issues seem like some kind of timing problem, but I don't really follow what the root cause is yet.  If anyone has any ideas on how make more sense out of this, it would be greatly appreciated!

Comment 5

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

Some of these issues might be related to refactoring done in bug 959420.  Will have to investigate further.

Comment 6

[Marco Castelluccio [:marco]]

Attachment: Patch

Mocking WebappOSUtils we could re-enable the test everywhere. The problem is that it is failing on b2g because of a problem with OS.File (the same error I was experiencing while trying to remove the osfile inclusion from AppsUtils.jsm in bug 981085).

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

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

Comment 7

[Marco Castelluccio [:marco]]

Fabrice, looks like it is the same problem you are experiencing for bug 993282.

Comment 8

[Marco Castelluccio [:marco]]

Comment on attachment 8411416 [details] [diff] [review]
Patch

The test works correctly on desktop platforms, fails on b2g emulator.

I think we can start by enabling the test on desktop and wait for bug 1005120 to enable it on b2g emulator.

Comment 9

[Alexandre Poirot [:ochameau]]

Comment on attachment 8411416 [details] [diff] [review]
Patch

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

::: toolkit/devtools/apps/tests/unit/xpcshell.ini
@@ -3,5 @@
>  tail = tail_apps.js
>  support-files = data/app.zip
>  
>  [test_webappsActor.js]
> -skip-if = (os == "win" || "linux" || "mac")

So if this test still fails on b2g you meant to keep a skip-if rule, right?

Comment 10

[Marco Castelluccio [:marco]]

(In reply to Alexandre Poirot (:ochameau) from comment #9)
> Comment on attachment 8411416 [details] [diff] [review]
> Patch
> 
> Review of attachment 8411416 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: toolkit/devtools/apps/tests/unit/xpcshell.ini
> @@ -3,5 @@
> >  tail = tail_apps.js
> >  support-files = data/app.zip
> >  
> >  [test_webappsActor.js]
> > -skip-if = (os == "win" || "linux" || "mac")
> 
> So if this test still fails on b2g you meant to keep a skip-if rule, right?

Yes, this is the patch I sent to try, but if it is r+ed, I'll check it in with skip-if = buildapp == "b2g".

Comment 11

[Alexandre Poirot [:ochameau]]

Comment on attachment 8411416 [details] [diff] [review]
Patch

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

Looks good then.

Comment 12

[Marco Castelluccio [:marco]]

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

Comment 13

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

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