Closed Bug 837816 Opened 7 years ago Closed 7 years ago

GeckoAppShell.loadMozGlue gets called three times on startup

Categories

(Firefox for Android :: General, defect)

All
Android
defect
Not set

Tracking

()

RESOLVED FIXED
Firefox 21

People

(Reporter: kats, Assigned: kats)

Details

Attachments

(1 file)

Once from GeckoAppShell.loadSQLiteLibs, once from GeckoAppShell.loadNSSLibs, and once from GeckoApp.onCreate, just for good measure. (Not necessarily in that order)
Also, I just noticed the synchronization blocks in loadNSSLibs and loadSQLiteLibs are pretty bad. The sSQLiteLibsLoaded and sNSSLibsLoaded objects are booleans, and specifically Boolean.TRUE and Boolean.FALSE because of autoboxing. That means any other code accidentally synchronizing on those objects will block these functions (I verified this by inserting this code in GeckoApp.create:

        new Thread() {
            public void run() {
                synchronized (Boolean.FALSE) {
                    try {
                        Thread.sleep(60 * 1000);
                    } catch (Exception e) {
                    }
                }
            }
        }.start();

and noticed that it blocked gecko startup for a minute, as expected). Additionally the object being synchronized on is reassigned within the synchronization block, which is also generally bad practice and can defeat the synchronization block in subtle ways on platforms where assignments are not atomic.
Working on a fix.
Assignee: nobody → bugmail.mozilla
Attached patch PatchSplinter Review
This does a bunch of cleanup with the bool/locking to fix the issues I identified above. It also adds a new bool to guard against loading mozglue multiple times unnecessarily.

Try run at https://tbpl.mozilla.org/?tree=Try&rev=ce81e1d1de54 - I expect this to show a startup time improvement as well.
Attachment #709908 - Flags: review?(cpeterson)
Comment on attachment 709908 [details] [diff] [review]
Patch

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

LGTM
Attachment #709908 - Flags: review?(cpeterson) → review+
(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #3)
> Try run at https://tbpl.mozilla.org/?tree=Try&rev=ce81e1d1de54 - I expect
> this to show a startup time improvement as well.

The ts startup times from this try push look a little worse (compared to m-i).
I looked at the recent inbound ts numbers before and after my change. Looks like it got a bit worse on 2.2 and a bit better on 4.0 but on both it's well within the margin of error. I'd call it insigificant.

               | Before my change | After my change
---------------+------------------+-----------------
Android 2.2 ts | Samples: 16      | Samples: 21
               | Average: 4163.5  | Average: 4580.86
               | Stddev: 1285.51  | Stddev: 1257.27
               | Max: 5564        | Max: 5708
               | Min: 3506        | Min: 3518
---------------+------------------+-----------------
Android 4.0 ts | Samples: 17      | Samples: 21
               | Average: 3881.53 | Average: 3866.29
               | Stddev: 944.467  | Stddev: 846.491
               | Max: 4047        | Max: 4016
               | Min: 3738        | Min: 3703
https://hg.mozilla.org/mozilla-central/rev/755d7faad3c0
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 21
You need to log in before you can comment on or make changes to this bug.