889696 - Firefox OS failed to boot with exception NS_ERROR_FILE_TARGET_DOES_NOT_EXIST

Status: RESOLVED FIXED

(Core Graveyard :: DOM: Apps, defect, P1)

Description

[leo.bugzilla.gecko]

After I update my leo device with GOTA update, b2g process is not boot up with exception "NS_ERROR_FILE_TARGET_DOES_NOT_EXIST"

Comment 1

[Changbin Park]

The GOTA update has a removed apps which is under /system/b2g/webapps/.
After GOTA update is performed the removed apps remains empty directories.

Steps to reproduce
 1. An app is installed from /system/b2g/webapps/<app> directory.
 2. Remove content of /system/b2g/webapps/<app> directory but remain the directory as empty.
 3. Make device performs update
   - To make it run installPreinstalledApp method in Webapps.jsm while booting.

NS_ERROR_FILE_TARGET_DOES_NOT_EXIST exception is occured while trying to copy /system/b2g/webapps/<app>/manifest.webapp and boot failed.

Comment 2

[Changbin Park]

Attachment: Prevent to make exception which make it stop running.

2 patches are applied.
One for detecting and skip empty directory in /system/b2g/webapps.
Another to catch a exception from copyTo method and keep it running.

If unhandled exception is occured in "installPreinstalledApp" method, it prevent boot process.

Comment 3

[[:fabrice] Fabrice Desré]

Comment on attachment 770576 [details] [diff] [review]
Prevent to make exception which make it stop running.

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

::: dom/apps/src/Webapps.jsm
@@ +279,5 @@
>        baseDir = FileUtils.getDir("coreAppsDir", ["webapps", aId], false);
>        if (!baseDir.exists()) {
>          return;
> +      } else if (!baseDir.directoryEntries.hasMoreElements()) {
> +        dump("Error: Core app in " + baseDir.path + " is empty");

Nit: use debug() instead of dump().

@@ +320,5 @@
>          file.append(aFile);
> +        try {
> +          file.copyTo(destDir, aFile);
> +        } catch(e) {
> +          dump("Error: Failed to copy " + file.path + " to " + destDir.path);

Nit: Use debug() instead of dump().

In this case, we want to actually remove the app from the apps list, like we do when failing to extract something from the zip at the of the installPreinstalledApp() function.

Comment 4

[Changbin Park]

(In reply to Fabrice Desré [:fabrice] from comment #3)
> Nit: use debug() instead of dump().
> Nit: Use debug() instead of dump().
I think debug() is better for consistency, but did you know the function is commented?
IMHO, the cases should not happened and it's about boot/update process.
So, if there's a case that messages are shown in log, it should be warned and checked.
That's why I used dump() instead of debug().
What do you think about it?
> 
> In this case, we want to actually remove the app from the apps list, like we
> do when failing to extract something from the zip at the of the
> installPreinstalledApp() function.
Like I said, the problem is occured while processing update.
So, aId is comes from /data/local/webapps/webapps.json. And it means application named "aId" is already installed.
Therefore, I think, the app should not be removed.
Think about that, the app was preinstalled app but after update it is not exist as preinstalled app anymore for some reason. And obviously, it's already installed.
Should I remove the app?

The empty directory is better be removed but the partition is mounted as RO.
So, I can't find any good solution for this so far.
Please, let me know if you have any good idea.

Comment 5

[[:fabrice] Fabrice Desré]

(In reply to Changbin Park from comment #4)
> (In reply to Fabrice Desré [:fabrice] from comment #3)
> > Nit: use debug() instead of dump().
> > Nit: Use debug() instead of dump().
> I think debug() is better for consistency, but did you know the function is
> commented?
> IMHO, the cases should not happened and it's about boot/update process.
> So, if there's a case that messages are shown in log, it should be warned
> and checked.
> That's why I used dump() instead of debug().
> What do you think about it?

We use inconditional dump() for severe errors, when the issue is not recoverable. That's not the case here so debug() is enough.

> > In this case, we want to actually remove the app from the apps list, like we
> > do when failing to extract something from the zip at the of the
> > installPreinstalledApp() function.
> Like I said, the problem is occured while processing update.
> So, aId is comes from /data/local/webapps/webapps.json. And it means
> application named "aId" is already installed.
> Therefore, I think, the app should not be removed.
> Think about that, the app was preinstalled app but after update it is not
> exist as preinstalled app anymore for some reason. And obviously, it's
> already installed.
> Should I remove the app?

As a user, I would not like the app to be removed, no.

> The empty directory is better be removed but the partition is mounted as RO.
> So, I can't find any good solution for this so far.
> Please, let me know if you have any good idea.

I'd like to understand better why we end up with an empty directory during update. Is this because the update mar file contains an empty directory (this would be a bug in the build system) or because we're not smart enough during the update process? The partition is mounted rw when applying updates.

Comment 6

[Changbin Park]

(In reply to Fabrice Desré [:fabrice] from comment #5)
> We use inconditional dump() for severe errors, when the issue is not
> recoverable. That's not the case here so debug() is enough.
OK.. I'll change it to "debug()". 

> I'd like to understand better why we end up with an empty directory during
> update. Is this because the update mar file contains an empty directory
> (this would be a bug in the build system) or because we're not smart enough
> during the update process? The partition is mounted rw when applying updates.
It's a kind of update process bug. As described in first comment, GOTA update makes the empty directory and it obviously a buf of GOTA update process. I'll be fixed anyway.
But, still, I think, it's better be got over such a abnormal case.
Because, it's too trivial abnormal case to prevent boot process.

It's better to have a smart solution to get rid of such a abnormal case, I couldn't find so far.

Comment 7

[Changbin Park]

Attachment: Change dump() to debug() from previous patch

I change "dump()" to "debug()".

Comment 8

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/projects/birch/rev/2245d28611d6

Comment 9

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/2245d28611d6