Last Comment Bug 572329 - Fennec is not suitable to be prestarted on device startup
: Fennec is not suitable to be prestarted on device startup
Status: NEW
:
Product: Fennec Graveyard
Classification: Graveyard
Component: General (show other bugs)
: Trunk
: ARM Maemo
: -- normal with 1 vote (vote)
: ---
Assigned To: Nobody; OK to take it and work on it
: maemo-linux
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2010-06-16 03:47 PDT by Miika Jarvinen
Modified: 2011-06-21 09:22 PDT (History)
10 users (show)
See Also:
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
Patch which SIGSTOPs process on first window show (884 bytes, patch)
2010-07-23 13:45 PDT, Miika Jarvinen
no flags Details | Diff | Splinter Review
startup scripts for fennec (5.27 KB, patch)
2010-09-22 08:27 PDT, Marko Gronbarj
azakai: review-
Details | Diff | Splinter Review
Patch to add SIGSTOP functionality on window Show (4.98 KB, patch)
2010-09-22 08:39 PDT, Marko Gronbarj
romaxa: review-
Details | Diff | Splinter Review

Description Miika Jarvinen 2010-06-16 03:47:35 PDT
Precondition: 
The Fennec browser is started upon device boot.

Expected result:
When clicking the browser Icon from the application menu, the browser UI is started immediately. When the browser is closed it should only hide the user interface and return to the prestarted state. When the browser is in the prestarted state the amount of wakeups should be reduced, and the browser should behave like it's not running at all.

Actual result:
It takes several seconds before the browser user interface appears. The time taken for this can be reduced by using the faststart component and -faststart-hidden, but this still isn't fast enough. To give an immediate response to the user the whole user interface needs to be started as a minimized window and just brought to foreground. 

Prestarting whole UI causes many problems with connectivity. Currently Fennec's default homepage is connecting to update servers in order to check whether there are updates for addons. This causes the browser to ask for connectivity, which leads to the connectivity dialog request from a process that doesn't have any user visible. There are also update timers for XULRunner/Fennec, which would be triggered later.  With the current implementation of the faststart component, downloads are still proceeding in the background after the last window has been closed.

Addons also cause problems with prestarting, because they would not know when the browser has been completely started or returned to prestarted state. 

There is also a high amount of wakeups, as can be seen from bugs 564118 and 567339. Bug 564118 is really bad for an application that is always running, because timer triggering every second would cause the battery to drain really quickly. 

Proposals:
I would propose that the Fennec UI startup is divided into two phases. The first phase would initialize Fennecs XUL, and second phase initialization would load the user's homepage. I also propose that there would be an additional notification signal (for example through nsIObserverService), which would announce to extensions when the browser is started by the user or put back to the prestarted state.
Comment 1 Miika Jarvinen 2010-07-23 13:45:48 PDT
Created attachment 459906 [details] [diff] [review]
Patch which SIGSTOPs process on first window show

How about sending SIGSTOP signal to process instead of using -faststart-hidden? 

Here is a patch which does that on Qt port when MOZ_SIGSTOP_ONWINDOWSHOW enviroment variable is set. This would require, that when user clicks on Fennec icon, SIGCONT is sent from script which gets launched. For my opinion we can't send SIGCONT signal from Fennec binary itself, because there is a timeframe when user can click on Fennec's icon, and prestarted Fennec hasn't yet created remote window (because it's still trying to initialize XULRunner). Also using remote component to regocnice whether Fennec is already running causes considerable delay, as it's available only after libxul.so has been loaded XULRunner has been initialized.
Comment 2 Alon Zakai (:azakai) 2010-08-02 10:10:33 PDT
Looks very useful. Two questions:

1. How about if this is done with an interface like --no-remote currently has? Maybe

  ./fennec --preload-freeze

would make it freeze itself as in this patch, and later running

  ./fennec

(without parameters) would check if there is a frozen process, and unfreeze it if so. (So, similar to the current method of checking for another process, and attaching to it if so, unless --no-remote is set.)

2. How about if the freezing were done just of the child process - then it could be part of the stuff debated in bug 579982. Question is, will freezing the child process be enough, or are there too many wakeups in the parent process.
Comment 3 Miika Jarvinen 2010-08-06 11:19:57 PDT
I'm bit suspicious about fennec process itself sending SIGCONT. This would require loading of shared libraries which takes some time. At least if SIGCONT is sent from same place where remote is currently handled  (XRE_main), it would cause big overhead.

Another thing is that how frozen process id should be sent to current process? I used pidof to handle this, and this works, but that isn't pretty in any way.

Stopping child process doesn't help for prestarted case. When fennec is prestarted child process isn't created before user opens some web page (about: pages are loaded without tabs.remote=true). And we definetedly wan't to prevent UI from showing up on device startup, which requires stopping of parent.
Comment 4 Alon Zakai (:azakai) 2010-08-06 14:43:05 PDT
(In reply to comment #3)
> I'm bit suspicious about fennec process itself sending SIGCONT. This would
> require loading of shared libraries which takes some time. At least if SIGCONT
> is sent from same place where remote is currently handled  (XRE_main), it would
> cause big overhead.
> 
> Another thing is that how frozen process id should be sent to current process?
> I used pidof to handle this, and this works, but that isn't pretty in any way.
> 

Good point about loading shared libraries being slow. However, by using the ./fennec binary, we would be able to use the code&method --no-remote uses for attaching or not attaching to a running process. I didn't look at the code, but I presume there is a file somewhere with the process id that is checked, using some locking mechanism. It's hard to do this stuff in a cross-platform way that works perfectly everywhere, so I tend to think that reusing the --no-remote approach would be best, despite it being slower.

(About the speed, this would only be a slowdown on the second load of ./fennec - after the libraries are already in memory. So, doesn't sound too bad.)

> Stopping child process doesn't help for prestarted case. When fennec is
> prestarted child process isn't created before user opens some web page (about:
> pages are loaded without tabs.remote=true). And we definetedly wan't to prevent
> UI from showing up on device startup, which requires stopping of parent.

I see what you mean. Ok, ignore what I said about freezing just the child process.
Comment 5 Miika Jarvinen 2010-08-09 10:42:21 PDT
Remote checking uses invisible x-window to check whether Fennec is already running. It also does communication to other process by setting XAtoms on that window. 

Althroug libraries would be in memory using this approach would cause slowdown when MOZ_ENABLE_MEEGOTOUCH is used, because MApplication is created before XRemote is handled in XRE_main. (http://hg.mozilla.org/mozilla-central/file/a73ba51c5b1d/toolkit/xre/nsAppRunner.cpp#3143). 

There is a way on meego to use cached MApplication instance (http://apidocs.meego.com/mtf/launcher.html), but we can't use that because we use parameters, which are not compatible with booster (-style for example).
Comment 6 Alon Zakai (:azakai) 2010-08-17 16:46:20 PDT
Did you check how big the slowdown actually is? I think it would be close to negligible. If you are really worried, then we should do some tests to measure it.

The benefits of using the remote checking code is that it would be cross-platform, and safer (the remote checking code already handles all the corner cases here).
Comment 7 Miika Jarvinen 2010-08-20 11:41:28 PDT
I made tests: when I launch fennec binary time taken to reach remote client code varies from 1.2 second to 1.6 second. When I use mozilla-xremote-client to bring up browser window, time is about 0.2 second.
Comment 8 Alon Zakai (:azakai) 2010-08-23 12:54:44 PDT
Ok, that is indeed significant. Let's do it as you suggested.
Comment 9 Alon Zakai (:azakai) 2010-08-26 10:11:32 PDT
So as I said, I support Miika's approach and patch. I think the next steps for this bug should be

1. An updated patch that includes the outside script that restarts Fennec (i.e. that includes everything relevant).
2. A review from the Qt module owner (which I believe is Oleg?).

(There is no need to wait on bug 568054, which I thought would be useful here.)
Comment 10 Marko Gronbarj 2010-09-22 08:27:51 PDT
Created attachment 477508 [details] [diff] [review]
startup scripts for fennec

This  will create two scripts to enable MOZ_SIGSTOP_ONWINDOWSHOW usage. fennec_startup.sh will run in a loop with MOZ_SIGSTOP_ONWINDOWSHOW set and fennec.sh will use SIGCONT if process Stopped.
Comment 11 Marko Gronbarj 2010-09-22 08:39:31 PDT
Created attachment 477511 [details] [diff] [review]
Patch to add SIGSTOP functionality on window Show

Continued Miika's patch. In addition to SIGSTOP on Window show, patch will create fennec.lock, when closing the application (listening quit-application signal), file to prevent starting the process until its is ready, fennec.lock file is deleted when application is ready to be started with SIGCONT.
Comment 12 Oleg Romashin (:romaxa) 2010-09-22 09:51:05 PDT
Comment on attachment 477511 [details] [diff] [review]
Patch to add SIGSTOP functionality on window Show

> 
>+NS_IMPL_ISUPPORTS1(MozCloseEventObserver, nsIObserver)
>+

Don't like observers in nsWindow code...


> 

>+        if (aEvent->type() == QEvent::Show) {
>+            //Send SIGSTOP to ourselve if MOZ_SIGSTOP_ONWINDOWSHOW enviroment
>+            //variable is set clear this env to do this only on first startup
>+            if (getenv("MOZ_SIGSTOP_ONWINDOWSHOW")) {
>+                unsetenv("MOZ_SIGSTOP_ONWINDOWSHOW");
>+
>+                //delete locks to prevent
>+                QFile file("/tmp/fennec.lock");
this should use some XPCOM temp files

>+                if(file.exists())
>+                    file.remove();

and probably better use nsIFile...

>+    NS_DECL_ISUPPORTS
>+
>+            MozCloseEventObserver()
        ^ indent
>+    {
>+        nsCOMPtr<nsIObserverService> obs

>+
>+    NS_IMETHODIMP
>+            Observe(nsISupports* aSubject, const char *aTopic,
>+                    const PRUnichar *aData)
           ^ indent
>+    {
>+        QFile file("/tmp/fennec.lock");
>+        file.open(QIODevice::ReadWrite );
>+        file.close();
use XPCOM files

>+
>+
no double/triple empty lines
>+
>+/**
> 
>+static MozCloseEventObserver* mObserver = 0;
>+
>+
empty lines?

> #define NS_WINDOW_TITLE_MAX_LENGTH 4095
> 

>     gBufferPixmapUsageCount++;
>+    if(getenv("MOZ_SIGSTOP_ONWINDOWSHOW") && !mObserver)
>+        mObserver = new MozCloseEventObserver();
>+

who is destroying this?


will check general idea later
Comment 13 Alon Zakai (:azakai) 2010-09-22 11:11:54 PDT
Comment on attachment 477508 [details] [diff] [review]
startup scripts for fennec

1. I am not sure what the goal of this is:

63	 if ( test $# -gt 0 ); then
64	   sleep 1
65	   /usr/lib/fennec/fennec "$@"
66	 fi

?

2. Indentation seems a little off, probably mixing tabs and spaces? Best to use only spaces.

3. Why aren't parameters carried over here?

+while [ true ]; do
+ /usr/lib/fennec/fennec.sh
+done

4. Typo in a comment,

  # if MOZ_SIGSTOP_ONWINDOWSHOW set set lock file

(|set set|).

5. Need a comment in fennec.sh that refers to where the process is actually stopped - which filename specifically (not just function name). That will help people get to all the relevant code quickly. I would put this in a big comment at the start, which gives an overview of what this script does and how that fits in with the stopping code.
Comment 14 Oleg Romashin (:romaxa) 2010-09-26 14:32:17 PDT
Comment on attachment 477511 [details] [diff] [review]
Patch to add SIGSTOP functionality on window Show

> #include <QtGui/QApplication>
> #include <QtGui/QGraphicsView>
> #include <QtGui/QGraphicsWidget>
>+#include <QtCore/QFile>
>+

use nsFile implementation


> 
>+#include "nsIServiceManager.h"
>+#include "nsIObserver.h"

no observers in nsWindow code... 
>+
>+                //delete locks to prevent
>+                QFile file("/tmp/fennec.lock");

use nsIFile

>     gBufferPixmapUsageCount++;
>+    if(getenv("MOZ_SIGSTOP_ONWINDOWSHOW") && !mObserver)
>+        mObserver = new MozCloseEventObserver();

still no understanding who is destroying this creature.
Comment 15 Oleg Romashin (:romaxa) 2010-09-26 14:36:58 PDT
Comment on attachment 477511 [details] [diff] [review]
Patch to add SIGSTOP functionality on window Show

also I don't think this is good  idea to keep pre-start stuff in nsWidget code... I think it is better to make common NotifyObserve signal for gtk/qt implementation and handle that signal somewhere in nsNativeAppSupporXXX implementation
Comment 16 Alon Zakai (:azakai) 2010-09-27 09:52:30 PDT
I also am not very happy with putting it in widget code. However, after thinking about it, I'm not sure what the best thing to do is. The main issue is that widget code is platform-specific, so it might be the best spot to know when we can properly do the preload stuff (such as, when enough of the app is loaded, but not too much, for it to be frozen).
Comment 17 timeless 2010-10-07 06:28:17 PDT
nativeappsupport is the right place for this

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