Last Comment Bug 889696 - Firefox OS failed to boot with exception NS_ERROR_FILE_TARGET_DOES_NOT_EXIST
: Firefox OS failed to boot with exception NS_ERROR_FILE_TARGET_DOES_NOT_EXIST
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: DOM: Apps (show other bugs)
: Trunk
: All Linux
: P1 critical (vote)
: mozilla25
Assigned To: Changbin Park
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2013-07-02 18:34 PDT by leo.bugzilla.gecko
Modified: 2013-07-19 11:40 PDT (History)
6 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Prevent to make exception which make it stop running. (1013 bytes, patch)
2013-07-02 19:19 PDT, Changbin Park
fabrice: review-
Details | Diff | Splinter Review
Change dump() to debug() from previous patch (1.27 KB, patch)
2013-07-14 17:55 PDT, Changbin Park
fabrice: review+
Details | Diff | Splinter Review

Description leo.bugzilla.gecko 2013-07-02 18:34:40 PDT
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 2013-07-02 18:53:41 PDT
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 2013-07-02 19:19:07 PDT
Created attachment 770576 [details] [diff] [review]
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é 2013-07-03 14:38:23 PDT
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 2013-07-03 16:12:04 PDT
(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é 2013-07-11 10:20:32 PDT
(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 2013-07-14 17:49:28 PDT
(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 2013-07-14 17:55:59 PDT
Created attachment 775403 [details] [diff] [review]
Change dump() to debug() from previous patch

I change "dump()" to "debug()".
Comment 8 Ryan VanderMeulen [:RyanVM] 2013-07-19 07:15:23 PDT
https://hg.mozilla.org/projects/birch/rev/2245d28611d6
Comment 9 Ryan VanderMeulen [:RyanVM] 2013-07-19 11:40:59 PDT
https://hg.mozilla.org/mozilla-central/rev/2245d28611d6

Note You need to log in before you can comment on or make changes to this bug.