Closed Bug 623912 Opened 9 years ago Closed 9 years ago

Blank white screen for 10+ seconds on first-run library extraction

Categories

(Firefox for Android Graveyard :: General, defect)

All
Android
defect
Not set

Tracking

(fennec2.0+)

VERIFIED FIXED
Tracking Status
fennec 2.0+ ---

People

(Reporter: mbrubeck, Assigned: blassey)

References

Details

(Keywords: perf, polish)

Attachments

(1 file, 2 obsolete files)

Regression from bug 608042:  Fennec takes several seconds to extract libraries to the cache directory on first-run (on devices with >150MB storage free).  This happens before the splash screen appears, so the user sees only a white screen during this time.

We should display a splash screen with an informative message like "Installing..." during first-run, so that users know this is an expected delay but that it won't be repeated.
tracking-fennec: --- → ?
On my Galaxy Tab, it takes over 10 seconds. I thought the app was hung.
Assignee: nobody → blassey.bugs
Attached patch patch (obsolete) — Splinter Review
this adds a second status message for loading the libraries. It also threads off loading the libraries and moves the gecko thread creation into java so we get to the point where we can draw the surface in time to show the "installing libraries..." status message. Finally it changes our default theme's background color to match that of the splash screen.
Attachment #502105 - Flags: review?(mbrubeck)
Attachment #502105 - Flags: review?(doug.turner)
Attached patch patch (obsolete) — Splinter Review
the last patch was missing themes.xml
Attachment #502105 - Attachment is obsolete: true
Attachment #502109 - Flags: review?(mbrubeck)
Attachment #502109 - Flags: review?(doug.turner)
Attachment #502105 - Flags: review?(mbrubeck)
Attachment #502105 - Flags: review?(doug.turner)
Comment on attachment 502109 [details] [diff] [review]
patch


>+++ b/embedding/android/GeckoApp.java
>@@ -69,6 +69,7 @@ abstract public class GeckoApp
>     public static GeckoSurfaceView surfaceView;
>     public static GeckoApp mAppContext;
>     public static boolean mFullscreen = false;
>+    Thread mLibLoadThread = null;

not static?  Should it be?  I would think that there should only be one library loading thread ever.

>+        if (intent == null)
>+            intent = getIntent();
>+        final Intent i = intent;

why not just use |i|?  Not sure what the reason is for the final Intent declaration here..


>+        new Thread() { 
>+            public void run() {
>+                try {
>+                    if (mLibLoadThread != null)
>+                        mLibLoadThread.join();
>+                } catch (InterruptedException ie) {}


Don't you want to return instead of the join?  If two different run()s happen, the first will run, the second will be blocked, the first will complete, then the second will continue to run.

>+                surfaceView.mSplashStatusMsg = 
>+                    getResources().getString(R.string.splash_screen_label);
>+                surfaceView.drawSplashScreen();
>+                // unpack files in the components directory
>+                try {
>+                    unpackComponents();
>+                } catch (FileNotFoundException fnfe) {
>+                    Log.e("GeckoApp", "error unpacking components", fnfe);
>+                    showErrorDialog(getString(R.string.error_loading_file));
>+                    return;
>+                } catch (IOException ie) {
>+                    Log.e("GeckoApp", "error unpacking components", ie);
>+                    String msg = ie.getMessage();
>+                    if (msg.equalsIgnoreCase("No space left on device"))
>+                        showErrorDialog(getString(R.string.no_space_to_start_error));
>+                    else
>+                        showErrorDialog(getString(R.string.error_loading_file));
>+                    return;

Could any other error happen here?  Maybe add a catch for the generic expection, report it, and return.
>+++ b/toolkit/xre/nsAndroidStartup.cpp
>@@ -57,8 +57,6 @@
> 
> #define LOG(args...) __android_log_print(ANDROID_LOG_INFO, MOZ_APP_NAME, args)
> 
>-static pthread_t gGeckoThread = 0;
>-
> struct AutoAttachJavaThread {
>     AutoAttachJavaThread() {
>         attached = mozilla_AndroidBridge_SetMainThread((void*)pthread_self());
>@@ -159,8 +157,6 @@ Java_org_mozilla_gecko_GeckoAppShell_nat
>     jenv->GetStringRegion(jargs, 0, len, wargs.BeginWriting());
>     char *args = ToNewUTF8String(wargs);
> 
>-    if (pthread_create(&gGeckoThread, NULL, GeckoStart, args) != 0) {
>-        LOG("pthread_create failed!");
>-    }
>+    GeckoStart(args);
> }
>

I don't understand this change.


Somewhat confused about some of the changes.  Sorry.  Minus for now.
Attachment #502109 - Flags: review?(doug.turner) → review-
Comment on attachment 502109 [details] [diff] [review]
patch

Looks good to me.  Some weird whitespace/indentation here:

>+        mLibLoadThread = new Thread(new Runnable() { 
>+            public void run() {
>+                GeckoAppShell.loadGeckoLibs(
>+                    getApplication().getPackageResourcePath());
>+                
>+                    }});

Running this on T-Mobile G2 takes about 5 seconds on the "installing" screen,
but on the Samsung Galaxy Tab it consistently takes around 40 seconds.  I think
this may be a regression compared to earlier builds; not sure if it's caused by
the splash screen patch or not.  On IRC we discussed moving the cache
extraction to a background process, if we can't reduce the delay.
Attachment #502109 - Flags: review?(mbrubeck) → review+
(In reply to comment #4)
> Comment on attachment 502109 [details] [diff] [review]
> patch
> 
> 
> >+++ b/embedding/android/GeckoApp.java
> >@@ -69,6 +69,7 @@ abstract public class GeckoApp
> >     public static GeckoSurfaceView surfaceView;
> >     public static GeckoApp mAppContext;
> >     public static boolean mFullscreen = false;
> >+    Thread mLibLoadThread = null;
> 
> not static?  Should it be?  I would think that there should only be one library
> loading thread ever.

That's already guarded against below, but it wouldn't hurt to make it static either.
> 
> >+        if (intent == null)
> >+            intent = getIntent();
> >+        final Intent i = intent;
Need it to be final for it to be accessible in the thread.
> 
> why not just use |i|?  Not sure what the reason is for the final Intent
> declaration here..
> 
> 
> >+        new Thread() { 
> >+            public void run() {
> >+                try {
> >+                    if (mLibLoadThread != null)
> >+                        mLibLoadThread.join();
> >+                } catch (InterruptedException ie) {}
> 

These are two different threads. One to load the libraries and the other to start gecko. The one defined here can't ruin until the lib loading thread is done.
> 
> Don't you want to return instead of the join?  If two different run()s happen,
> the first will run, the second will be blocked, the first will complete, then
> the second will continue to run.
> 
> >+                surfaceView.mSplashStatusMsg = 
> >+                    getResources().getString(R.string.splash_screen_label);
> >+                surfaceView.drawSplashScreen();
> >+                // unpack files in the components directory
> >+                try {
> >+                    unpackComponents();
> >+                } catch (FileNotFoundException fnfe) {
> >+                    Log.e("GeckoApp", "error unpacking components", fnfe);
> >+                    showErrorDialog(getString(R.string.error_loading_file));
> >+                    return;
> >+                } catch (IOException ie) {
> >+                    Log.e("GeckoApp", "error unpacking components", ie);
> >+                    String msg = ie.getMessage();
> >+                    if (msg.equalsIgnoreCase("No space left on device"))
> >+                        showErrorDialog(getString(R.string.no_space_to_start_error));
> >+                    else
> >+                        showErrorDialog(getString(R.string.error_loading_file));
> >+                    return;
> 
> Could any other error happen here?  Maybe add a catch for the generic
> expection, report it, and return.

There shouldn't be. This code isn't new, just reindented
> >+++ b/toolkit/xre/nsAndroidStartup.cpp
> >@@ -57,8 +57,6 @@
> > 
> > #define LOG(args...) __android_log_print(ANDROID_LOG_INFO, MOZ_APP_NAME, args)
> > 
> >-static pthread_t gGeckoThread = 0;
> >-
> > struct AutoAttachJavaThread {
> >     AutoAttachJavaThread() {
> >         attached = mozilla_AndroidBridge_SetMainThread((void*)pthread_self());
> >@@ -159,8 +157,6 @@ Java_org_mozilla_gecko_GeckoAppShell_nat
> >     jenv->GetStringRegion(jargs, 0, len, wargs.BeginWriting());
> >     char *args = ToNewUTF8String(wargs);
> > 
> >-    if (pthread_create(&gGeckoThread, NULL, GeckoStart, args) != 0) {
> >-        LOG("pthread_create failed!");
> >-    }
> >+    GeckoStart(args);
> > }
> >
> 
> I don't understand this change.

Since this is being called from a new thread in java, we don't need to thread it off here
> 
> 
> Somewhat confused about some of the changes.  Sorry.  Minus for now.
Attachment #502109 - Flags: review- → review?
Attachment #502109 - Flags: review? → review?(doug.turner)
the  other thing.  with tje last  patch ...  i always  see the "installing"  string  at startup.
Attached patch patchSplinter Review
carrying mbrubeck's review (fixed the indentation issue he pointed out)
Attachment #502109 - Attachment is obsolete: true
Attachment #502577 - Flags: review?(doug.turner)
Attachment #502109 - Flags: review?(doug.turner)
Comment on attachment 502577 [details] [diff] [review]
patch

i'd talk to madhava about the string for installing.  I'd prefer something closer to "optimization installation, one moment...".

also, if this screen lasts for more than 2-3 seconds, i'd really like to see a progress spiner.

follow-ups accepted.
Attachment #502577 - Flags: review?(doug.turner) → review+
This blocks fennec, assuming that we are keeping the install-to-sdcard installation optimization.
tracking-fennec: ? → 2.0+
pushed http://hg.mozilla.org/mozilla-central/rev/e830f4b554a4
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Verified fixed testing on Mozilla/5.0 (Android; Linux armv7l; rv:2.0b10pre) Gecko/20110112 Firefox/4.0b10pre Fennec/4.0b4pre
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.