Closed Bug 992589 Opened 10 years ago Closed 10 years ago

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

Categories

(Core Graveyard :: DOM: Apps, defect)

ARM
Gonk (Firefox OS)
defect
Not set
normal

Tracking

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

RESOLVED FIXED
mozilla31
Tracking Status
firefox30 --- fixed
firefox31 --- fixed
b2g-v1.4 --- fixed
b2g-v2.0 --- fixed

People

(Reporter: macajc, Assigned: macajc)

References

Details

Attachments

(1 file, 1 obsolete file)

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
Attached 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+
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)
Attachment #8402240 - Attachment is obsolete: true
Attachment #8403186 - Flags: review+
Keywords: checkin-needed
(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: 10 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+
Depends on: 1010321
Product: Core → Core Graveyard
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: