981085 - Stop using OS.File in the apps code

Status: RESOLVED FIXED

(Core Graveyard :: DOM: Apps, defect)

Description

[(no longer active)]

It would be nice if we could switch back to using the netutil implementation until OS.File is improved enough for our needs.

Comment 1

[Marco Castelluccio [:marco]]

I'll post my patch shortly.

Comment 2

[Marco Castelluccio [:marco]]

Attachment: Patch

Comment 3

[(no longer active)]

Thanks Marco!  But I also see two other usages here, are you planning to kill those as well?

Comment 4

[[:fabrice] Fabrice Desré]

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

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

We also need to remove OS.File use from AppsUtils.jsm since it's used by OperatorApps.jsm that is pulled in by Webapps.jsm

Comment 5

[Marco Castelluccio [:marco]]

With this patch the tests were green, should I kill the other usages anyway?

Comment 6

[[:fabrice] Fabrice Desré]

Yes please.

Comment 7

[Marco Castelluccio [:marco]]

Attachment: Patch

This is working locally, I've just sent it to try.

(If it looks ok, could you test it on a real phone?)

Comment 8

[Marco Castelluccio [:marco]]

Oh, I just noticed OS.File is used in OperatorApps.jsm too. I'll upload another patch for that.

Comment 9

[(no longer active)]

Andrea, this might help with datastore tests once it lands.

Comment 10

[David Teller [:Yoric] - still alive but not very active]

Ehsan, can you please file a bug with the improvements you need from OS.File?

Comment 11

[(no longer active)]

(In reply to David Rajchenbach Teller [:Yoric] (currently away from bugzilla – ETA March 11th – please use "needinfo?") from comment #10)
> Ehsan, can you please file a bug with the improvements you need from OS.File?

Filed bug 981789 to get the conversation started!

Comment 12

[Marco Castelluccio [:marco]]

Attachment: Patch

I've tried to keep exactly the same behavior as before to avoid regressions, even if there was some room for cleanups.

osfile.jsm is still included in Webapps.jsm and OperatorApps.jsm because we're still using OS.Path (that isn't a problem).

It is still included in AppsUtils.jsm because without it the tests on the emulator (only on the emulator) fail (I haven't managed to figure out why).

Comment 13

[Marco Castelluccio [:marco]]

https://tbpl.mozilla.org/?tree=Try&rev=e2b245986d0d (AppsUtils.jsm includes osfile.jsm)
https://tbpl.mozilla.org/?tree=Try&rev=d0835facc5ee (AppsUtils.jsm doesn't include osfile.jsm)

The errors that you can see in the failing builds are:
23:43:07  WARNING -  03-13 06:37:04.472    44    44 E GeckoConsole: [JavaScript Error: ""toString" is read-only" {file: "resource://gre/modules/osfile/osfile_unix_allthreads.jsm" line: 89}]
23:43:07  WARNING -  03-13 06:37:04.482    44    44 E GeckoConsole: [JavaScript Error: "NS_ERROR_XPC_GS_RETURNED_FAILURE: Component returned failure code: 0x80570016 (NS_ERROR_XPC_GS_RETURNED_FAILURE) [nsIJSCID.getService]" {file: "resource://gre/modules/PermissionPromptHelper.jsm" line: 46}]

23:43:07  WARNING -  03-13 06:37:12.183    44    44 E GeckoConsole: [JavaScript Error: "TypeError: shell is undefined" {file: "chrome://b2g/content/settings.js" line: 103}]
23:43:07  WARNING -  03-13 06:37:14.203    44    44 E GeckoConsole: [JavaScript Error: ""toString" is read-only" {file: "resource://gre/modules/osfile/osfile_unix_allthreads.jsm" line: 89}]
23:43:07  WARNING -  03-13 06:37:14.662    44    44 E GeckoConsole: [JavaScript Error: ""toString" is read-only" {file: "resource://gre/modules/osfile/osfile_unix_allthreads.jsm" line: 89}]

Comment 14

[[:fabrice] Fabrice Desré]

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

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

::: dom/apps/src/OperatorApps.jsm
@@ +132,5 @@
>      debug("copying " + aOrg + " to " + aDst);
>      return aDst && Task.spawn(function() {
>        try {
> +        let orgDir = Cc["@mozilla.org/file/local;1"].
> +                     createInstance(Ci.nsIFile);

nit: align .createInstance with ["@mozilla...]

@@ +186,5 @@
>        // DIRECTORY_NAME + SINGLE_VARIANT_SOURCE_DIR and move all apps (and
>        // configuration file) to PREF_SINGLE_VARIANT_DIR and return
>        // PREF_SINGLE_VARIANT_DIR as sourceDir.
> +      let svFinalDir = Cc["@mozilla.org/file/local;1"].
> +                       createInstance(Ci.nsIFile);

nit: alignement

::: dom/apps/src/Webapps.jsm
@@ +1162,5 @@
> +    let ostream = FileUtils.openSafeFileOutputStream(file);
> +
> +    // Obtain a converter to convert our data to a UTF-8 encoded input stream.
> +    let converter = Cc["@mozilla.org/intl/scriptableunicodeconverter"].
> +                    createInstance(Ci.nsIScriptableUnicodeConverter);

nit: alignement

@@ +1168,5 @@
> +
> +    // Asynchronously copy the data to the file.
> +    let istream = converter.convertToInputStream(aData);
> +    NetUtil.asyncCopy(istream, ostream, function(rc) {
> +      deferred.resolve();

We should reject() if NS_FAILED(rc)

Comment 15

[Marco Castelluccio [:marco]]

Attachment: Patch

Comment 16

[Marco Castelluccio [:marco]]

Attachment: Patch

Carrying r+.

I think after this we can reenable apps tests on b2g.

Comment 17

[Marco Castelluccio [:marco]]

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

Should we land this now or wait and see if bug 961317 is fixed?

Comment 18

[[:fabrice] Fabrice Desré]

(In reply to Marco Castelluccio [:marco] from comment #17)
> Comment on attachment 8395256 [details] [diff] [review]
> Patch
> 
> Should we land this now or wait and see if bug 961317 is fixed?

I would land.

Comment 19

[Marco Castelluccio [:marco]]

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

Let's land then, so that we can re-enable the tests asap.

Comment 20

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/fx-team/rev/246f001547ac

Comment 21

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

https://hg.mozilla.org/mozilla-central/rev/246f001547ac

Comment 22

[Carsten Book [:Tomcat]]

hi, sorry had to back this out in https://tbpl.mozilla.org/?rev=9afe2a1145bd since one of this 3 pushes seems to have caused Bug 986173

Comment 23

[Marco Castelluccio [:marco]]

The "Dialer > Keypad Entering a 3 digits number with the keypad" test passed in my try build: https://tbpl.mozilla.org/?tree=Try&rev=1c65d75fb4a4
There was a Gi failure, but it's a known failure (bug 982260).

Comment 24

[Ryan VanderMeulen [:RyanVM]]

We disabled the test for now since it seems to just be sensitive to devtools changes in general.
https://hg.mozilla.org/integration/fx-team/rev/12985e11e4e8

Comment 25

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/12985e11e4e8

Comment 26

[(no longer active)]

Marco, have you tried re-enabling the apps tests on the try server with this fixed?

Comment 27

[Marco Castelluccio [:marco]]

(In reply to :Ehsan Akhgari (lagging on bugmail, needinfo? me!) from comment #26)
> Marco, have you tried re-enabling the apps tests on the try server with this
> fixed?

Yes, they were green. I'll open a new bug to re-enable the tests.

Comment 28

[Ryan VanderMeulen [:RyanVM]]

We should probably get this on Aurora so the dom/apps tests can be re-enabled for v1.4 as well.

Comment 29

[Marco Castelluccio [:marco]]

I don't know. I think the patch for Aurora should be pretty similar.

Comment 30

[[:fabrice] Fabrice Desré]

Yes, let's do that on 1.4 too.

Comment 31

[Marco Castelluccio [:marco]]

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

[Approval Request Comment]
Bug caused by (feature/regressing bug #): 
User impact if declined: B2G tests on beta may fail because of problems with OS.File (indeed they're disabled on Beta)
Testing completed (on m-c, etc.): This has been on central for an entire cycle. Two small regressions have been fixed (bug 991246 and bug 992589).
Risk to taking this patch (and alternatives if risky): There may be some risk associated with the switch from OS.File to nsIFile. But the switch would enable us to do a lot more testing on Beta (allowing to uplift bug 915879 too).
String or IDL/UUID changes made by this patch: None.

Note that we need to uplift the two patches in bug 991246 and bug 992589 too.

Comment 32

[(no longer active)]

Why was this reopened?

Comment 33

[Marco Castelluccio [:marco]]

(In reply to :Ehsan Akhgari (lagging on bugmail, needinfo? me!) from comment #32)
> Why was this reopened?

It wasn't reopened, we're landing it on Beta.

Comment 34

[(no longer active)]

Awesome, but you don't need to reopen the bug for that.  We usually reopen bugs when the corresponding patches have been backed out for some reason.

Comment 35

[Marco Castelluccio [:marco]]

(In reply to :Ehsan Akhgari (lagging on bugmail, needinfo? me!) from comment #34)
> Awesome, but you don't need to reopen the bug for that.  We usually reopen
> bugs when the corresponding patches have been backed out for some reason.

Ah, I've just noticed that I accidentally removed all the flags when I added the two bugs in the "Depends on:" field...

Comment 36

[(no longer active)]

(In reply to comment #35)
> (In reply to :Ehsan Akhgari (lagging on bugmail, needinfo? me!) from comment
> #34)
> > Awesome, but you don't need to reopen the bug for that.  We usually reopen
> > bugs when the corresponding patches have been backed out for some reason.
> 
> Ah, I've just noticed that I accidentally removed all the flags when I added
> the two bugs in the "Depends on:" field...

Yeah... Bugzilla sometimes does that to you :(

Comment 37

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/ed09e2b74d14

Comment 38

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g30_v1_4/rev/ed09e2b74d14