WebApp installation should happen in a background thread

RESOLVED WONTFIX

Status

Firefox Graveyard
Web Apps
P2
normal
RESOLVED WONTFIX
6 years ago
2 years ago

People

(Reporter: smaug, Assigned: marco)

Tracking

(Depends on: 4 bugs)

Details

(Whiteboard: [Snappy:p3])

Attachments

(3 obsolete attachments)

WebApps are installed using the main thread.
Perhaps it should happen in a background thread.

I don't think this is any major problem, since installation doesn't do
any heavy I/O.
Myk - Opinion on this? Is there value to do web app installation on a background thread?
Appcache population should certainly happen asynchronously.

Beyond that, I don't think there is much value to moving installation to a background thread, as it is not very resource-intensive, and since users seem unlikely to want to interact with the application in some other way in the short period of time during which files are being written/copied.

So I would initially avoid doing this, treating it as a premature optimization until we identify specific use cases in which there is a problem that it would solve.  But I'm also not an expert here, so I'd welcome additional opinions on the matter.
Olli - Do you have an argument for why we should move now to the background thread?
All main thread I/O cause responsiveness problems. Whether or not the problems in this case
are bad is not clear. My _guess_ is no.

(And yes, appcache population ofc should happen in background.)
Myk - Is this a valid bug to keep open then? Usually I think bugs like these should be opened when we know there's a perf problem that it causes, but if we aren't seeing the problem right now, then there's no reason to fix this right now.
This is certainly a valid bug. Whether this is important enough to fix immediately is different question.
The "investigate" part seems done, with the conclusion being: yes, it should happen on a background thread.  There are cases of premature optimization, and optimizations that are not worth doing (because the complexity/performance tradeoff is not worth it), but smaug suggests in comment 6 that this is not one of those cases, and he has the expertise in this area, so I trust his conclusion.  Thus I am changing the title of this bug to reflect it.

There's also a prioritization question, for which it isn't clear that we have enough information to make a definitive judgement.  But based on the hunches of the people who have weighed in to date, I suggest we make this [Snappy:P3] until we see evidence to suggest it is more important.  But I'll leave it to the Snappy drivers to make that determination.
Summary: Investigate if WebApp installation should happen in a background thread → WebApp installation should happen in a background thread
Opinion on Priority and Milestone:

Priority: P3
Milestone: Firefox 15

Reasoning:

There isn't any detected problems for responsiveness issues currently with web app installation (I've seen no feedback on responsiveness problems in this area). This is low priority until we find use cases where responsiveness becomes a problem due this bug.
I'm confused as to what this bug is actually about.

Does app installation today do any IO from the main thread? If so, what IO? Is it the IO to create the xulrunner app and/or the profile for the xulrunner app? Or is it downloading the appcache resources? Or writing those appcache resources to the newly created profile?
(In reply to Jonas Sicking (:sicking) from comment #9)
> I'm confused as to what this bug is actually about.
> 
> Does app installation today do any IO from the main thread? 
Yes.  

>If so, what IO?
https://mxr.mozilla.org/mozilla-central/source/browser/modules/WebappsInstaller.jsm?rev=97a2a24bbc1d#271

> Is it the IO to create the xulrunner app and/or the profile for the
> xulrunner app?
Seems like copying some file and creating the directories


> Or is it downloading the appcache resources? Or writing those
> appcache resources to the newly created profile?
This isn't done yet, and I assume this all to happen in background thread.
Priority: -- → P3
Target Milestone: --- → Future

Comment 11

6 years ago
This should be fixed. Main thread IO is often not a problem in testing, but IO is very variable by nature and can result in very poor user experience on slower machines or machines that happen to be busy doing io activity.
blocking-basecamp: --- → ?

Updated

6 years ago
blocking-kilimanjaro: --- → ?
The web apps integration into desktop feature isn't applicable to basecamp (it's out of scope) - removing that flag.
blocking-basecamp: ? → ---
There's very little happening on the main-thread during installation. Let me outline it:

- Creation of a few folders and the profile
- Copying the runtime stub and uninstaller to that folder
- Generating two .ini files
- Generating the icon file

Everything else, and most importantly, all the unbounded time steps, like downloading the manifest, downloading the icon, generating the .json files and populating appcache happens in the background for obvious reasons.

The main issue as I see is the icon generation, but that is also the hardest to change, since it needs to convert a .png to .ico and AFAIK imglib is main-thread only.

The other are certainly valid issues that should be fixed at some point in time but I don't think they are the best thing to prioritize at the moment.

Comment 14

6 years ago
(In reply to Felipe Gomes (:felipe) from comment #13)
> There's very little happening on the main-thread during installation. Let me
> outline it:
> 
> - Creation of a few folders and the profile
> - Copying the runtime stub and uninstaller to that folder
> - Generating two .ini files
> - Generating the icon file
> 
> Everything else, and most importantly, all the unbounded time steps, like
> downloading the manifest, downloading the icon, generating the .json files
> and populating appcache happens in the background for obvious reasons.
> 
> The main issue as I see is the icon generation, but that is also the hardest
> to change, since it needs to convert a .png to .ico and AFAIK imglib is
> main-thread only.
> 
> The other are certainly valid issues that should be fixed at some point in
> time but I don't think they are the best thing to prioritize at the moment.

We've taken main thread shortcuts elsewhere and, fixing those shortcuts is now the biggest set of tasks for snappy. I do not think we should regress here.

I see no concrete evidence that this wont cause unbounded io-triggered UI freezes.
We've learned from Telemetry that seemingly innocuous filesystem operations can take absurdly long in the wild.

There really is no such thing as a safe main-thread filesystem operation.
Does this IO happen on the WebRT instance, or in the running Firefox? If the former, then is the worst that will happen is that initial app load is slow (iow, nothing happens for a while then app opens)?

Updated

6 years ago
blocking-kilimanjaro: ? → -
My understanding is that it happens in the Firefox process, so it definitely has the potential to cause jank issues when installing.
Installation happens in Firefox process.

Updated

5 years ago
QA Contact: jsmith

Updated

5 years ago
Whiteboard: [Snappy] → [Snappy:p3]

Updated

5 years ago
status-firefox16: --- → wontfix
Priority: P3 → P2
Hardware: x86_64 → All
Target Milestone: Future → ---
(Assignee)

Comment 19

4 years ago
With the refactoring in bug 747552, moving most of the installation off the main-thread will be quite easy.
Marco: any chance you can take this on as a followup to bug 747552?
Depends on: 747552
(Assignee)

Comment 21

4 years ago
(In reply to Myk Melez [:myk] [@mykmelez] from comment #20)
> Marco: any chance you can take this on as a followup to bug 747552?

Yes, sure.
Assignee: nobody → mcastelluccio
Status: NEW → ASSIGNED
(Assignee)

Updated

4 years ago
Depends on: 909582
(Assignee)

Updated

4 years ago
Depends on: 772538
(Assignee)

Updated

4 years ago
Depends on: 866571
(Assignee)

Comment 22

4 years ago
Created attachment 796878 [details] [diff] [review]
Patch

This patch is also a refactoring of the installer code, I think it's more clean now. It also simplifies the updater code (after this patch the updater code should be really straightforward to develop).

I've removed the removeInstallation call at the beginning of the installation, I think it's no longer needed because we're anyway overwriting all the existing files and we now don't fail the installation if a directory already exists.

There's still some main thread IO usages, but we need to wait on other bugs to tackle them. In the meantime this should improve the situation performance- and code-wise.
Attachment #796878 - Flags: review?(myk)
(Assignee)

Comment 23

4 years ago
(In reply to Marco Castelluccio [:marco] from comment #22)
> Created attachment 796878 [details] [diff] [review]
> Patch

Instead of the try-catch blocks for _createDirectoryStructure I'll use the ignoreExisting option (I've just remembered it exists), this should make the function easier to read.
Comment on attachment 796878 [details] [diff] [review]
Patch

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

This patch no longer applies cleanly, although the fix is trivial: remove the third argument `null` to the DOMApplicationRegistry.confirmInstall() call.


(In reply to Marco Castelluccio [:marco] from comment #22)

> I've removed the removeInstallation call at the beginning of the
> installation, I think it's no longer needed because we're anyway overwriting
> all the existing files and we now don't fail the installation if a directory
> already exists.

It's risky to rely on overwriting the existing files, because the files created by the installation will change over time, and when they do, some obsolete files won't be removed by this approach, which means obsolete files will hang around, misleading bug reporters and investigators and potentially making it harder to identify a bug in code that continues to reference an obsolete file mistakenly.

The Firefox installer is careful to remove such files, although it does so via a more sophisticated mechanism than the runtime's cruder method of simply removing the existing installation directory.

So I would continue to remove the existing installation, although perhaps it would be better to throw it away (i.e. put it into the OS's notion of the "trash can") instead of deleting it.


(On another note, it isn't clear what the behavior is regarding removal of the profile directory, and if it's changed by this patch; but in general, the runtime should not remove app data when a user uninstalls/reinstalls an app.)


(In reply to Marco Castelluccio [:marco] from comment #23)
> Instead of the try-catch blocks for _createDirectoryStructure I'll use the
> ignoreExisting option (I've just remembered it exists), this should make the
> function easier to read.

Good idea!  I wonder why it isn't documented on <https://developer.mozilla.org/en-US/docs/JavaScript_OS.File/OS.File_for_the_main_thread#OS.File.makeDir%28%29>.

::: browser/modules/webappsUI.jsm
@@ +153,2 @@
>            DOMApplicationRegistry.denyInstall(aData);
> +        });

Nit: *ex* is a great name for the exception caught by a try/catch statement, but *error* is better for the parameter passed to a reject handler, which may or may not have been generated by an exception.

Also, nit: I would format this slightly differently to try to make its complex structure a bit clearer:

        let nativeApp = WebappsInstaller.init(aData);
        WebappsInstaller.createProfile(nativeApp).then(
          (aProfileDir) => {
            DOMApplicationRegistry.confirmInstall(aData, aProfileDir,
              (aManifest) => {
                Task.spawn(function() {
                  try {
                    yield WebappsInstaller.install(nativeApp, aData, aManifest);
                    if (this.downloads[manifestURL]) {
                      yield this.downloads[manifestURL].promise;
                    }
                    installationSuccessNotification(aData, nativeApp, bundle);
                  } catch (ex) {
                    Cu.reportError("Error installing webapp: " + ex);
                    // TODO: Notify user that the installation has failed
                  } finally {
                    delete this.downloads[manifestURL];
                  }
                }.bind(this));
              }
            );
          },
          (ex) => {
            Cu.reportError("Error installing webapp: " + ex);
            notification.remove();
            delete this.downloads[manifestURL];
            DOMApplicationRegistry.denyInstall(aData);
          }
        );

::: toolkit/webapps/WebappsInstaller.jsm
@@ +17,5 @@
>  Cu.import("resource://gre/modules/AppsUtils.jsm");
>  Cu.import("resource://gre/modules/Task.jsm");
>  Cu.import("resource://gre/modules/Promise.jsm");
>  
>  this.WebappsInstaller = {

(Aside: since WebappsInstaller.init() returns a NativeApp object; and the other two WebappsInstaller methods, createProfile() and install(), both take that object as their first parameter; it seems like we could simplify this API by exporting the NativeApp constructor, having it return a platform-specific *NativeApp instance, and merging createProfile() and install() into the NativeApp.createAppProfile() and *NativeApp.install() methods, respectively.)

@@ +37,5 @@
>  #else
>      return null;
>  #endif
>  
> +    return nativeApp;

Nit: instead of assigning the object to a local variable only to return it, this could simply return the new object.

@@ +41,5 @@
> +    return nativeApp;
> +  },
> +
> +  // Returns a promise that resolves to the app's profile path or null if the
> +  // profile isn't created.

Nit: comments like this would be better formatted in javadoc.  Also, it would be useful to explain why createProfile might resolve to `null` here.  I can see why by looking at the code, but at first read, this comment doesn't make sense.

@@ +81,5 @@
>          }
>        };
>  
>        Services.obs.notifyObservers(null, "webapp-installed", JSON.stringify(data));
> +    }.bind(this));

You don't need to bind the task function, since it doesn't reference *this*.

@@ +228,3 @@
>      } catch (ex if ex.result == Cr.NS_ERROR_ALREADY_INITIALIZED) {}
> +
> +    throw new Task.Result(null);

Nit: this would be easier to understand if the second *throw* was inside the *catch* block, since that's the only circumstance under which we want to return a `null` value.  It might be even clearer if the first *throw* was outside the *try* block, since then the function ends with the Task equivalent of a return statement, although it means declaring appProfile outside the block:

    let appProfile;

    try {
      appProfile = profSvc.createDefaultProfileForApp(this.uniqueName, null,
                                                      null);

    } catch (ex if ex.result == Cr.NS_ERROR_ALREADY_INITIALIZED) {
      throw new Task.Result(null);
    }

    throw new Task.Result(appProfile.localDir);


(Aside: it isn't clear that returning `null` when the profile has already been created is the right thing to do; but that's what the old code did, so we shouldn't change it here.)

@@ +386,5 @@
> +
> +    iconDir = OS.Path.join(iconDir, "default");
> +    try {
> +      yield OS.File.makeDir(iconDir);
> +    } catch (ex if ex instanceof OS.File.Error && ex.becauseExists) {}

(Aside: this code would be so much simpler if OS.File had the equivalent of `mkdir -p`, a.k.a. mkpath, which nsIFile::Create does by default.)

@@ +565,5 @@
>      return deferred.promise;
>    }
>  }
>  
>  #elifdef XP_MACOSX

(Aside: since only one platform-specific *NativeApp definition will ever be included in this file, I wonder why we don't use preprocessor directives to make the NativeApp definition itself be platform-specific.)

@@ +625,5 @@
>  
> +  _writeRuntimeFiles: function() {
> +    let webapprtPre = OS.Path.join(this.runtimeFolder.path, "webapprt-stub");
> +    let webapprt = OS.Path.join(this.macOSDir, "webapprt");
> +    yield OS.File.copy(webapprtPre, webapprt);

What does "Pre" mean in the name webapprtPre?  webapprtStub (or simply *stub*) would be a more obvious name.

@@ +870,3 @@
>      // $XDG_DATA_HOME/applications/owa-<webappuniquename>.desktop
> +    yield OS.File.open(this.desktopINI.path, { truncate: true },
> +                       { unixMode: FileUtils.PERMS_FILE });

Good catch!  And FileUtils.* constants seem reasonable here and elsewhere, although OS.File docs say to build such mode values from constants in OS.Constants.

@@ +969,2 @@
>    return stripped;
>  }

With your changes, this function's behavior no longer matches the description in its javadoc comment.  The comment should be updated to match the new behavior.

Also, while you're at it, update the comment inside the function to describe the regular expressions more accurately.  Currently it only describes the first expression.

@@ +981,2 @@
>   */
>  function getAvailableFileName(aFolderSet, aName, aExtension) {

Nit: the mix of exception-based and return-value-based error handling complicates the logic in this file.  In conjunction with the increased use of Task, it's worth converting functions like this one, which return "null" to indicate an error, to throw instead.

And it looks like it'd be fairly simple in this case, since this function is only called twice, and one of those times itself throws if this function returns null.  The other one doesn't, although it's unclear why; at first glance, it seems like it should.
Attachment #796878 - Flags: review?(myk) → review-
(Assignee)

Comment 25

4 years ago
(In reply to Myk Melez [:myk] [@mykmelez] from comment #24)
> It's risky to rely on overwriting the existing files, because the files
> created by the installation will change over time, and when they do, some
> obsolete files won't be removed by this approach, which means obsolete files
> will hang around, misleading bug reporters and investigators and potentially
> making it harder to identify a bug in code that continues to reference an
> obsolete file mistakenly.
> 
> The Firefox installer is careful to remove such files, although it does so
> via a more sophisticated mechanism than the runtime's cruder method of
> simply removing the existing installation directory.
> 
> So I would continue to remove the existing installation, although perhaps it
> would be better to throw it away (i.e. put it into the OS's notion of the
> "trash can") instead of deleting it.

I think we should do as the Firefox installer, also because we don't obsolete files too often.
If the situation will become complicated in the future, well, we can always switch back to removing the installation.

> (On another note, it isn't clear what the behavior is regarding removal of
> the profile directory, and if it's changed by this patch; but in general,
> the runtime should not remove app data when a user uninstalls/reinstalls an
> app.)

We remove the profile only if the installation fails (both before and after the patch).

> Also, nit: I would format this slightly differently to try to make its
> complex structure a bit clearer:

Hopefully this extremely messy structure will go away soon with the refactoring in bug 910473!
So maybe I could also avoid modifying it in this patch.

> (Aside: since WebappsInstaller.init() returns a NativeApp object; and the
> other two WebappsInstaller methods, createProfile() and install(), both take
> that object as their first parameter; it seems like we could simplify this
> API by exporting the NativeApp constructor, having it return a
> platform-specific *NativeApp instance, and merging createProfile() and
> install() into the NativeApp.createAppProfile() and *NativeApp.install()
> methods, respectively.)

After bug 910473, in WebappsInstaller we'll have a single |install| function without |init| and |createProfile|.

> (Aside: it isn't clear that returning `null` when the profile has already
> been created is the right thing to do; but that's what the old code did, so
> we shouldn't change it here.)

I agree, we should read the old profile directory path from the profiles.ini file and reuse it.

> @@ +386,5 @@
> > +
> > +    iconDir = OS.Path.join(iconDir, "default");
> > +    try {
> > +      yield OS.File.makeDir(iconDir);
> > +    } catch (ex if ex instanceof OS.File.Error && ex.becauseExists) {}
> 
> (Aside: this code would be so much simpler if OS.File had the equivalent of
> `mkdir -p`, a.k.a. mkpath, which nsIFile::Create does by default.)

Indeed, I think bug 772538 will enable this and also a recursive removal (we're now using the old nsIFile).
I hope soon enough we'll be able to switch completely to OS.File, we'd just need some other functions (recursive removal, mkpath, more OS.Constants, INIParser, shortcut creation on Windows).

> (Aside: since only one platform-specific *NativeApp definition will ever be
> included in this file, I wonder why we don't use preprocessor directives to
> make the NativeApp definition itself be platform-specific.)

That's true, I guess we can do that in a followup?

> @@ +625,5 @@
> >  
> > +  _writeRuntimeFiles: function() {
> > +    let webapprtPre = OS.Path.join(this.runtimeFolder.path, "webapprt-stub");
> > +    let webapprt = OS.Path.join(this.macOSDir, "webapprt");
> > +    yield OS.File.copy(webapprtPre, webapprt);
> 
> What does "Pre" mean in the name webapprtPre?  webapprtStub (or simply
> *stub*) would be a more obvious name.

Actually, I don't know :D
I've simply kept the already existing name :D
(Assignee)

Comment 26

4 years ago
I'll wait your reply before posting a new patch.
Flags: needinfo?(myk)
(In reply to Marco Castelluccio [:marco] from comment #25)
> (In reply to Myk Melez [:myk] [@mykmelez] from comment #24)
> > So I would continue to remove the existing installation, although perhaps it
> > would be better to throw it away (i.e. put it into the OS's notion of the
> > "trash can") instead of deleting it.
> 
> I think we should do as the Firefox installer, also because we don't
> obsolete files too often.
> If the situation will become complicated in the future, well, we can always
> switch back to removing the installation.

I'm not sure it's worth implementing Firefox's more sophisticated mechanism.  But even if it is, I'd rather keep the current behavior of removing the existing installation before writing a new one.  It gives us more freedom to introduce the more sophisticated mechanism later (instead of being forced to do so the moment we obsolete the first file).  And it reduces the risk of an install process failing and leaving the installation with a mix of old and new files (although we should build additional robustness in a followup fix that reverts to the old installation if the install process fails).


> > (On another note, it isn't clear what the behavior is regarding removal of
> > the profile directory, and if it's changed by this patch; but in general,
> > the runtime should not remove app data when a user uninstalls/reinstalls an
> > app.)
> 
> We remove the profile only if the installation fails (both before and after
> the patch).

Excellent. :-)


> > Also, nit: I would format this slightly differently to try to make its
> > complex structure a bit clearer:
> 
> Hopefully this extremely messy structure will go away soon with the
> refactoring in bug 910473!
> So maybe I could also avoid modifying it in this patch.

Yes, that's ok.


> > (Aside: since WebappsInstaller.init() returns a NativeApp object; and the
> > other two WebappsInstaller methods, createProfile() and install(), both take
> > that object as their first parameter; it seems like we could simplify this
> > API by exporting the NativeApp constructor, having it return a
> > platform-specific *NativeApp instance, and merging createProfile() and
> > install() into the NativeApp.createAppProfile() and *NativeApp.install()
> > methods, respectively.)
> 
> After bug 910473, in WebappsInstaller we'll have a single |install| function
> without |init| and |createProfile|.

Ah, ok, sounds good.


> > (Aside: it isn't clear that returning `null` when the profile has already
> > been created is the right thing to do; but that's what the old code did, so
> > we shouldn't change it here.)
> 
> I agree, we should read the old profile directory path from the profiles.ini
> file and reuse it.

Indeed!


> > (Aside: since only one platform-specific *NativeApp definition will ever be
> > included in this file, I wonder why we don't use preprocessor directives to
> > make the NativeApp definition itself be platform-specific.)
> 
> That's true, I guess we can do that in a followup?

Yes, absolutely, this is followup material.


> > What does "Pre" mean in the name webapprtPre?  webapprtStub (or simply
> > *stub*) would be a more obvious name.
> 
> Actually, I don't know :D
> I've simply kept the already existing name :D

Ah, ok.  Then nit: I'd change the name, since you're modifying this code anyway, although it isn't necessary.
Flags: needinfo?(myk)
(Assignee)

Comment 28

4 years ago
Created attachment 801681 [details] [diff] [review]
Patch
Attachment #796878 - Attachment is obsolete: true
(Assignee)

Comment 29

4 years ago
Created attachment 801690 [details] [diff] [review]
Patch

On Mac the _removeInstallation(true) call wasn't doing anything, because it was removing the temp directory that was just created.
I plan to fix this in another bug, if that's ok with you.
Attachment #801681 - Attachment is obsolete: true
Attachment #801690 - Flags: review?(myk)
(Assignee)

Comment 30

4 years ago
Comment on attachment 801690 [details] [diff] [review]
Patch

I'm pretty sure this patch doesn't apply anymore. I'd like to tackle other things before coming back to this.
Attachment #801690 - Flags: review?(myk)
(Assignee)

Updated

4 years ago
Depends on: 920721
(Assignee)

Updated

4 years ago
Depends on: 919771
(Assignee)

Updated

4 years ago
Depends on: 923540
(Assignee)

Updated

4 years ago
Depends on: 923568
(Assignee)

Updated

4 years ago
Depends on: 934283
(Assignee)

Updated

4 years ago
Depends on: 958888
(Assignee)

Updated

4 years ago
No longer depends on: 772538
(Assignee)

Comment 31

4 years ago
Comment on attachment 801690 [details] [diff] [review]
Patch

Most of the installer code is now using OS.File. There are still some usages left to fix (some require features that aren't in OS.File yet).
Attachment #801690 - Attachment is obsolete: true
(Assignee)

Updated

4 years ago
Depends on: 1001849

Updated

3 years ago
Depends on: 1028366

Updated

3 years ago
No longer depends on: 1028366
(Assignee)

Updated

3 years ago
Depends on: 1028983
(Assignee)

Updated

3 years ago
No longer depends on: 1001849
Blocks: 1111077
Per bug 1238079, we're going to disable the desktop web runtime and remove it
from the codebase, so we won't fix these bugs in the integration between Firefox and the runtime.
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
Resolution: --- → WONTFIX
Product: Firefox → Firefox Graveyard
You need to log in before you can comment on or make changes to this bug.