Stop using OS.File in the apps code

RESOLVED FIXED in Firefox 30

Status

defect
RESOLVED FIXED
5 years ago
2 years ago

People

(Reporter: Ehsan, Assigned: marco)

Tracking

Trunk
mozilla31
x86
macOS
Dependency tree / graph
Bug Flags:
in-testsuite ?

Firefox Tracking Flags

(firefox29 unaffected, firefox30 fixed, firefox31 fixed, firefox-esr24 unaffected, b2g-v1.4 fixed, b2g-v2.0 fixed)

Details

Attachments

(1 attachment, 4 obsolete attachments)

Reporter

Description

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

Comment 1

5 years ago
I'll post my patch shortly.
Assignee: nobody → mar.castelluccio
Status: NEW → ASSIGNED
Assignee

Comment 2

5 years ago
Posted patch Patch (obsolete) — Splinter Review
Attachment #8387863 - Flags: review?(fabrice)
Reporter

Comment 3

5 years ago
Thanks Marco!  But I also see two other usages here, are you planning to kill those as well?
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
Attachment #8387863 - Flags: review?(fabrice) → feedback+
Assignee

Comment 5

5 years ago
With this patch the tests were green, should I kill the other usages anyway?
Yes please.
Assignee

Comment 7

5 years ago
Posted patch Patch (obsolete) — Splinter Review
This is working locally, I've just sent it to try.

(If it looks ok, could you test it on a real phone?)
Attachment #8387863 - Attachment is obsolete: true
Attachment #8387960 - Flags: review?(fabrice)
Assignee

Comment 8

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

Comment 9

5 years ago
Andrea, this might help with datastore tests once it lands.
Ehsan, can you please file a bug with the improvements you need from OS.File?
Flags: needinfo?(ehsan)
Assignee

Updated

5 years ago
Attachment #8387960 - Attachment is obsolete: true
Attachment #8387960 - Flags: review?(fabrice)
Reporter

Comment 11

5 years ago
(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!
Flags: needinfo?(ehsan)
Assignee

Comment 12

5 years ago
Posted patch Patch (obsolete) — Splinter Review
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).
Attachment #8390611 - Flags: review?(fabrice)
Assignee

Comment 13

5 years ago
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 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)
Attachment #8390611 - Flags: review?(fabrice) → feedback+
Assignee

Comment 15

5 years ago
Posted patch Patch (obsolete) — Splinter Review
Attachment #8390611 - Attachment is obsolete: true
Attachment #8391699 - Flags: review?(fabrice)
Attachment #8391699 - Flags: review?(fabrice) → review+
Assignee

Comment 16

5 years ago
Posted patch PatchSplinter Review
Carrying r+.

I think after this we can reenable apps tests on b2g.
Attachment #8391699 - Attachment is obsolete: true
Attachment #8395256 - Flags: review+
Assignee

Updated

5 years ago
Keywords: checkin-needed
Assignee

Comment 17

5 years ago
Comment on attachment 8395256 [details] [diff] [review]
Patch

Should we land this now or wait and see if bug 961317 is fixed?
Attachment #8395256 - Flags: checkin?(fabrice)
Assignee

Updated

5 years ago
Keywords: checkin-needed
(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.
Assignee

Updated

5 years ago
Keywords: checkin-needed
Assignee

Comment 19

5 years ago
Comment on attachment 8395256 [details] [diff] [review]
Patch

Let's land then, so that we can re-enable the tests asap.
Attachment #8395256 - Flags: checkin?(fabrice)
https://hg.mozilla.org/mozilla-central/rev/246f001547ac
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → mozilla31
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
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Assignee

Comment 23

5 years ago
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).
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
Whiteboard: [fixed-in-fx-team]
https://hg.mozilla.org/mozilla-central/rev/12985e11e4e8
Status: REOPENED → RESOLVED
Closed: 5 years ago5 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Reporter

Comment 26

5 years ago
Marco, have you tried re-enabling the apps tests on the try server with this fixed?
Flags: needinfo?(mar.castelluccio)
Assignee

Comment 27

5 years ago
(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.
Flags: needinfo?(mar.castelluccio)
We should probably get this on Aurora so the dom/apps tests can be re-enabled for v1.4 as well.
Flags: needinfo?(mar.castelluccio)
Assignee

Comment 29

5 years ago
I don't know. I think the patch for Aurora should be pretty similar.
Flags: needinfo?(mar.castelluccio) → needinfo?(fabrice)
Yes, let's do that on 1.4 too.
Flags: needinfo?(fabrice)
Assignee

Updated

5 years ago
Status: RESOLVED → REOPENED
Depends on: 991246, 992589
Resolution: FIXED → ---
Assignee

Comment 31

5 years ago
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.
Attachment #8395256 - Flags: approval-mozilla-beta?
Attachment #8395256 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Reporter

Comment 32

5 years ago
Why was this reopened?
Assignee

Comment 33

5 years ago
(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.
Reporter

Comment 34

5 years ago
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.
Status: REOPENED → RESOLVED
Closed: 5 years ago5 years ago
Resolution: --- → FIXED
Assignee

Comment 35

5 years ago
(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...
Reporter

Comment 36

5 years ago
(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 :(
Flags: in-testsuite?

Updated

2 years ago
Product: Core → Core Graveyard
You need to log in before you can comment on or make changes to this bug.