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

RESOLVED FIXED in Firefox 30

Status

defect
RESOLVED FIXED
5 years ago
2 years ago

People

(Reporter: macajc, Assigned: macajc)

Tracking

Trunk
mozilla31
ARM
Gonk (Firefox OS)
Dependency tree / graph

Firefox Tracking Flags

(firefox30 fixed, firefox31 fixed, b2g-v1.4 fixed, b2g-v2.0 fixed)

Details

Attachments

(1 attachment, 1 obsolete attachment)

Assignee

Description

5 years ago
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
Assignee

Comment 1

5 years ago
Posted patch Proposed patch (obsolete) — Splinter Review
Attachment #8402240 - Flags: review?(mar.castelluccio)
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.
Attachment #8402240 - Flags: review?(mar.castelluccio) → feedback+
Assignee

Updated

5 years ago
Attachment #8402240 - Flags: review?(ferjmoreno)
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.
Attachment #8402240 - Flags: review?(ferjmoreno) → review+
(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)
Assignee

Comment 5

5 years ago
Attachment #8402240 - Attachment is obsolete: true
Attachment #8403186 - Flags: review+
Assignee

Updated

5 years ago
Keywords: checkin-needed
Assignee

Comment 6

5 years ago
(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
https://hg.mozilla.org/mozilla-central/rev/33f98fa5c415
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
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.
Attachment #8403186 - Flags: approval-mozilla-beta?
Attachment #8403186 - Flags: approval-mozilla-beta? → approval-mozilla-beta+

Updated

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