992589 - OperatorApps.jsm errors when running with and without Single Variant apps

Status: RESOLVED FIXED

(Core Graveyard :: DOM: Apps, defect)

Description

[Carmen Jimenez Cabezas]

When we execute without single variant apps (on a build without the /data/local/svoperapps directory) we have the following error:

I/Gecko ( 132): -*-*- OperatorApps.jsm : Error copying /data/local/svoperapps to /persist/svoperapps. [Exception... "Component returned failure code: 0x80520006 (NS_ERROR_FILE_TARGET_DOES_NOT_EXIST) [nsIFile.isDirectory]" nsresult: "0x80520006 (NS_ERROR_FILE_TARGET_DOES_NOT_EXIST)" location: "JS frame :: resource://gre/modules/OperatorApps.jsm :: this.OperatorAppsRegistry._copyDirectory/< :: line 147" data: no]

We need to replace the line:

+ if (!orgDir.isDirectory()) {

with something like:
       if (!orgDir.exists() || !orgDir.isDirectory()) {

On the other hand, when we execute the code with single variant apps (/data/local/svoperapps exists and has valid data) we have the following error: 

OperatorApps.jsm : Error copying /data/local/svoperapps to /persist/svoperapps. TypeError: invalid path component

Only the configuration file is copied to the persist directory but the apps directories will not be copied on it

We have this error because entry.name is undefined. in line:
            yield this._copyDirectory(entry.path,
                                      Path.join(aDst, entry.name));

nsIFile has not name attribute, we need to use leafName instead

Comment 1

[Carmen Jimenez Cabezas]

Attachment: Proposed patch

Comment 2

[Marco Castelluccio [:marco]]

Comment on attachment 8402240 [details] [diff] [review]
Proposed patch

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

The patch looks good to me, but I guess you should ask r? to a module peer.

::: dom/apps/src/OperatorApps.jsm
@@ +141,5 @@
>        try {
>          let orgDir = Cc["@mozilla.org/file/local;1"]
>                         .createInstance(Ci.nsIFile);
>          orgDir.initWithPath(aOrg);
> +        if (!orgDir.exists() || !orgDir.isDirectory()) {

We used to log the error with OS.File too, the code was:
> let orgInfo = yield File.stat(aOrg);
> if (!orgInfo.isDir) {
OS.File.stat throws if the file doesn't exist.

@@ +167,5 @@
>  
>              entry.copyTo(dstDir, entry.leafName);
>            } else {
>              yield this._copyDirectory(entry.path,
> +                                      Path.join(aDst, entry.leafName));

Thank you, I modified the other two |entry.name|, but forgot this one.

Comment 3

[Fernando Jiménez Moreno [:ferjm]]

Comment on attachment 8402240 [details] [diff] [review]
Proposed patch

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

::: dom/apps/src/OperatorApps.jsm
@@ +141,5 @@
>        try {
>          let orgDir = Cc["@mozilla.org/file/local;1"]
>                         .createInstance(Ci.nsIFile);
>          orgDir.initWithPath(aOrg);
> +        if (!orgDir.exists() || !orgDir.isDirectory()) {

Add a debug message here, so we don't fail silently. Or use OS.File as suggested by Marco.

Comment 4

[Marco Castelluccio [:marco]]

(In reply to Fernando Jiménez Moreno [:ferjm] from comment #3)
> Comment on attachment 8402240 [details] [diff] [review]
> Proposed patch
> 
> Review of attachment 8402240 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/apps/src/OperatorApps.jsm
> @@ +141,5 @@
> >        try {
> >          let orgDir = Cc["@mozilla.org/file/local;1"]
> >                         .createInstance(Ci.nsIFile);
> >          orgDir.initWithPath(aOrg);
> > +        if (!orgDir.exists() || !orgDir.isDirectory()) {
> 
> Add a debug message here, so we don't fail silently. Or use OS.File as
> suggested by Marco.

The debug message is already shown if we don't add orgDir.exists (it's the message Carmen copied in the first comment).

(P.S.: We migrated from OS.File back to nsIFile because with OS.File we were having a lot of test failures)

Comment 5

[Carmen Jimenez Cabezas]

Attachment: Proposed patch - v2

Comment 6

[Carmen Jimenez Cabezas]

(In reply to Carmen Jimenez Cabezas from comment #5)
> Created attachment 8403186 [details] [diff] [review]
> Proposed patch - v2

r=ferjm
Proposed patch v2 has the changes that Fernando had requested

Comment 7

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/33f98fa5c415

Comment 8

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/33f98fa5c415

Comment 9

[Marco Castelluccio [:marco]]

Comment on attachment 8403186 [details] [diff] [review]
Proposed patch - v2

[Approval Request Comment]
Bug caused by (feature/regressing bug #): Bug 981085.
User impact if declined: We can't uplift bug 981085 and so we can't enable a lot of tests on Beta.
Testing completed (on m-c, etc.): This has been an entire cycle on central.
Risk to taking this patch (and alternatives if risky): Basically no risk.
String or IDL/UUID changes made by this patch: None.

Comment 10

[Ryan VanderMeulen [:RyanVM]]

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

Comment 11

[Ryan VanderMeulen [:RyanVM]]

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