731341 - Crash when accessing form history early in startup

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Wesley Johnston (:wesj)]

Attachment: Patch

I left out an ensureSqliteLoaded call here. Good timing must have been helping my tests. 

Patch builds fine. Doesn't crash. Sync team is testingfrom their end right now. If its good there I'll mark for review.

Comment 1

[Nick Alexander :nalexander [he/him]]

Hard to say if this is your problem or not.  This is from Android Sync unit test code, so presumably there is no Gecko App.

D FormHistoryProvider(973)    No profile provided, using default
D FormHistoryProvider(973)    Using path: /data/data/org.mozilla.fennec_ncalexan/files/mozilla/npkn063c.default
E DatabaseUtils(973)          Writing exception to parcel
E DatabaseUtils(973)          java.lang.NullPointerException
E DatabaseUtils(973)          	at org.mozilla.gecko.GeckoAppShell.loadLibsSetup(GeckoAppShell.java:245)
E DatabaseUtils(973)          	at org.mozilla.gecko.GeckoAppShell.ensureSQLiteLibsLoaded(GeckoAppShell.java:334)
E DatabaseUtils(973)          	at org.mozilla.fennec_ncalexan.db.GeckoProvider.getDB(GeckoProvider.java:92)
E DatabaseUtils(973)          	at org.mozilla.fennec_ncalexan.db.GeckoProvider.getDatabaseForProfile(GeckoProvider.java:134)
E DatabaseUtils(973)          	at org.mozilla.fennec_ncalexan.db.GeckoProvider.getDatabase(GeckoProvider.java:178)
E DatabaseUtils(973)          	at org.mozilla.fennec_ncalexan.db.GeckoProvider.query(GeckoProvider.java:257)
E DatabaseUtils(973)          	at android.content.ContentProvider$Transport.bulkQuery(ContentProvider.java:174)
E DatabaseUtils(973)          	at android.content.ContentProviderNative.onTransact(ContentProviderNative.java:111)
E DatabaseUtils(973)          	at android.os.Binder.execTransact(Binder.java:320)
E DatabaseUtils(973)          	at dalvik.system.NativeStart.run(Native Method)

Comment 2

[Richard Newman [:rnewman]]

Why is the CP calling into GeckoAppShell anyway? *headdesk*

Comment 3

[Gian-Carlo Pascutto [:gcp]]

>Why is the CP calling into GeckoAppShell anyway? *headdesk*

Because it needs Gecko's SQLite library, and there's some work that must be done before that is available.

Comment 4

[Wesley Johnston (:wesj)]

Yep. We could pull all those pieces out of GeckoAppShell into some other utility class, but I would rather not if we don't have to.

This crash is happening because our library loading code assumes that GeckoApp.mAppContext exists. I fixed it in the crypto patch but didn't pull that bit down here. I'm unbitrotting around kat's changes and testing locally...

Comment 5

[Wesley Johnston (:wesj)]

Attachment: Patch

This is almost entirely just moving things around so that we don't have to go through all of loadLibsSetup (30-50ms timing on my phone, most of that in setupPluginEnvironment and setupDownloadEnvironment) just for the contentProviders. It splits the bits that are more related to starting Gecko into a setupGeckoEnvironment function, and only leaves the pieces required to set GRE_HOME, MOZ_LINKER_CACHE, and for clearing the library cache in loadLibsSetup. 

It also modifies loadLibsSetup to take a generic context, since we may not have GeckoApp when we're loading libraries.

I also removed the initialization of getFreeSpace().

Tested by calling into the formhistory very early in GeckoApp onCreate(). The world seems to stay alive.

Comment 6

[Nick Alexander :nalexander [he/him]]

Attachment: First run of Fennec failing on emulator (adb logcat)

Comment 7

[Nick Alexander :nalexander [he/him]]

Attachment: First run of Fennec succeeding on device (adb logcat) for comparison

Comment 8

[Nick Alexander :nalexander [he/him]]

Definitely getting closer.  I wanted to inspect the DB so I tried using this patch on an emulator, with no luck, and thought you should know.

Comment 9

[Gian-Carlo Pascutto [:gcp]]

- If blassey confirms its no longer needed, we can kill off all the extractLibs stuff. When you do this, uncomment the part in profileMigrator that cleans up extracted libs.
- We have "loadGeckoLibs" "loadMozGlue" but "ensureSQLiteLibsLoaded". Need to make the naming consistent over all 3.
- GeckoAppShell.ensureSQLiteLibsLoaded should call loadMozGlue and not leave that up to its caller (I'm looking at GeckoProvider.java.in)

Comment 10

[Wesley Johnston (:wesj)]

Comment on attachment 601437 [details] [diff] [review]
Patch

Since blassey is out of commission this week, throwing this to dougt (or he can toss to someone else he's confident knows the linker bits well enough to approve?)

I can't reproduce the emulator crash on my Android 4.0 emulator. It can't find libmozglue.so, which is pretty bad, and could be caused by the change here. Can you give more steps to reproduce, what version of Android you're running. Are you running a sync or starting Fennec? Maybe if you start a sync with no Gecko, mContext is different and we're looking in the wrong place for libraries...

Comment 11

[Nick Alexander :nalexander [he/him]]

> I can't reproduce the emulator crash on my Android 4.0 emulator. It can't
> find libmozglue.so, which is pretty bad, and could be caused by the change
> here. Can you give more steps to reproduce, what version of Android you're
> running. Are you running a sync or starting Fennec? Maybe if you start a
> sync with no Gecko, mContext is different and we're looking in the wrong
> place for libraries...

Here's the AVD.  Are the versions/levels wrong?

archo:~ ncalexan$ android list avd
Available Android Virtual Devices:
    Name: g
    Path: /Users/ncalexan/.android/avd/g.avd
  Target: Android 2.3.3 (API level 10)
     ABI: armeabi
    Skin: WVGA800
  Sdcard: 2047M

I start and install Fennec (yesterday's inbound, with your patch on top):

archo:~ ncalexan$ $ANDROID_HOME/tools/emulator -wipe-data -avd g -partition-size 2047

archo:mozilla-inbound ncalexan$ $ANDROID_HOME/platform-tools/adb install -r objdir-droid/dist/fennec*apk
2955 KB/s (20106294 bytes in 6.643s)
	pkg: /data/local/tmp/fennec-13.0a1.en-US.android-arm.apk
Success

archo:mozilla-inbound ncalexan$ adb logcat -c

Then I try to run Fennec for the first time in the emulator, by clicking on the applications icon, then on the Fennec icon, and I get the attached log.

(This emulator has not had any Android Sync development/testing done on it.)

Comment 12

[Nick Alexander :nalexander [he/him]]

Attachment: First run of Fennec failing on emulator (adb logcat) (second attempt)

Comment 13

[Wesley Johnston (:wesj)]

Ahh. I can see it now. Thanks.

Comment 14

[Wesley Johnston (:wesj)]

Hmm... crash happens even with no patches applied. I'm not sure we've ever run well in a 2.3.3 emulator, or maybe some of the recent linker changes have made us worse. Testing with nightlies for the past few weeks next.

Comment 15

[Nick Alexander :nalexander [he/him]]

I can second that we've never run well in this AVD.  If the answer is to run a newer AVD, that's fine by me.

Comment 16

[Richard Newman [:rnewman]]

I've only ever been able to launch GeckoApp in an ARMv7 v14+ emulator, which is Dog Slow. The more usable emulators have always resulted in a crash.

The Content Providers were typically fine, because they didn't use any functionality that wasn't pure Android, so Sync's automated tests could run on an earlier emulator.

Comment 17

[Doug Turner (:dougt)]

Comment on attachment 601437 [details] [diff] [review]
Patch

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

::: mobile/android/base/GeckoAppShell.java
@@ +112,5 @@
>      static File sHomeDir = null;
>      static private int sDensityDpi = 0;
>      private static Boolean sSQLiteLibsLoaded = false;
> +    private static Boolean sLibsSetup = false;
> +    private static Boolean sMozGlueLoaded = false;

Do you really need both of these flags?

@@ +251,4 @@
>  
> +        File cacheFile = getCacheDir(context);
> +        // the location of the extracted libraries
> +        if (context instanceof GeckoApp) {

I do not understand this if branch.  what other contexts would be getting here?

@@ +251,5 @@
>  
> +        File cacheFile = getCacheDir(context);
> +        // the location of the extracted libraries
> +        if (context instanceof GeckoApp) {
> +            GeckoAppShell.putenv("GRE_HOME=" + GeckoApp.sGREDir.getPath());

Why don't we set GRE_HOME in setupGeckoEnvironment()?

@@ +256,5 @@
> +
> +            // if this is a debug launch, clear the extracted libs cache
> +            Intent i = ((Activity)context).getIntent();
> +            extractLibs = GeckoApp.ACTION_DEBUG.equals(i.getAction());
> +            if (!extractLibs) {

We do not need to extractLibs any longer for debug.  We can also remove the debug intent.  Please triple check this by using jim chen's gdb to attach to a build with this patch.

@@ +371,5 @@
>  
> +    public static void loadMozGlue() {
> +        if (sMozGlueLoaded)
> +            return;
> +        System.loadLibrary("mozglue");

I do not think you need sMozGlueLoaded.  Calling System.loadLibrary() multiple times is safe.

Comment 18

[Richard Newman [:rnewman]]

(In reply to Doug Turner (:dougt) from comment #17)

> > +        if (context instanceof GeckoApp) {
> 
> I do not understand this if branch.  what other contexts would be getting
> here?

Via Content Provider calls, context can be supplied by Sync's SyncAdapter.

Comment 19

[Wesley Johnston (:wesj)]

Thanks for looking over this doug!

> Do you really need both of these flags?
Probably not.

> I do not understand this if branch.  what other contexts would be getting
> here?
Sync as Richard mentioned. We also run the passwords provider in its own process since it needs access to its own NSS. I can't remember off the top of my head what Context it passes here if you call into it from a GeckoApp context. I'll double check.

> Why don't we set GRE_HOME in setupGeckoEnvironment()?
This was the main reason I was breaking this out. To run a Content Provider (or the profile migrator) we need GRE_HOME set so that the linker knows where to look, but we don't need all this other stuff done. i.e. We wanted loadLibsSetup to actually be things needed for loading libs, and not things needed for running Gecko.

> We do not need to extractLibs any longer for debug.  We can also remove the
> debug intent.  Please triple check this by using jim chen's gdb to attach to
> a build with this patch.

Will do!

> I do not think you need sMozGlueLoaded.  Calling System.loadLibrary()
> multiple times is safe.

Cool. Just being a little paranoid.

Comment 20

[Wesley Johnston (:wesj)]

Attachment: Patch v2

> Do you really need both of these flags?
Removed sMozGlueLoaded. We're not checking it anymore at all.

> We do not need to extractLibs any longer for debug.  We can also remove the
> debug intent.  Please triple check this by using jim chen's gdb to attach to
> a build with this patch.

I removed that from here, which also meant some changes in APKOpen.cpp. This still has the delete libs cache stuff. I'll post another bug to remove it while we figure out if that's good or bad.

There's a bunch more to be done here to remove extractlibs entirely. These bits seemed fairly innocent for now.

Need to test this a bunch, but dougt said to toss it up early because he's heading out of town.

Comment 22

[Mike Hommey [:glandium]]

> We can also remove the debug intent.

I think it's still needed to give time to jimdb to attach.

Comment 23

[Brad Lassey [:blassey] (use needinfo?)]

Comment on attachment 602009 [details] [diff] [review]
Patch v2

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

::: mobile/android/base/GeckoAppShell.java
@@ +252,5 @@
> +        if (context instanceof GeckoApp) {
> +            GeckoAppShell.putenv("GRE_HOME=" + GeckoApp.sGREDir.getPath());
> +
> +            // clear the extracted libs cache
> +            File[] files = cacheFile.listFiles();

how much does this cost? can we move it to background thread?

@@ +274,5 @@
> +        }
> +        sLibsSetup = true;
> +    }
> +
> +    private static void setupPluginEnvironment(Context context) {

this should take a GeckoApp arg

@@ +312,5 @@
>              Log.i(LOGTAG, "No download directory has been found: " + e);
>          }
> +    }
> +
> +    public static void setupGeckoEnvironment(Context context) {

this should take a GeckoApp arg

@@ +323,5 @@
> +        // profile home path
> +        GeckoAppShell.putenv("HOME=" + profile.getFilesDir().getPath());
> +
> +        Intent i = null;
> +        if (context instanceof Activity) {

i doesn't look to be used outside of this if's scope, but then again the if isn't needed if you change the type of context

Comment 24

[Mike Hommey [:glandium]]

(In reply to Brad Lassey [:blassey] from comment #23)
> ::: mobile/android/base/GeckoAppShell.java
> @@ +252,5 @@
> > +        if (context instanceof GeckoApp) {
> > +            GeckoAppShell.putenv("GRE_HOME=" + GeckoApp.sGREDir.getPath());
> > +
> > +            // clear the extracted libs cache
> > +            File[] files = cacheFile.listFiles();
> 
> how much does this cost? can we move it to background thread?

This should just go away.

Comment 25

[Wesley Johnston (:wesj)]

Yeah. I was playing with that yesterday. Things runs fine, but I can't get JIM_DB to attach anymore (it runs fine, and attaches, but Gecko never starts). I'm going to pull all the library extraction stuff out of this bug and post it as a second patch over in the other one. I need this to land for the Passwords and FormHistory sync features and I don't think removing the library extraction code should block it.

Comment 26

[Mike Hommey [:glandium]]

Ah yeah, Gecko not starting is bug 732117. Somehow it fell off my radar.

Comment 27

[Wesley Johnston (:wesj)]

Attachment: Patch v3

Sorry for the churn. This is the same patch with the GeckoApp changes, but I left in the DEBUG intent piece and all the extractLibs stuff.

I'll remove all that in bug 732069. Like I said, I just didn't want to hang this up on that code (which is apparently ok!)

Comment 28

[Wesley Johnston (:wesj)]

grrr. that LoadLibsSetups(GeckoApp.mAppContext) in loadSqliteLibs should (obviously) be loadSqliteLibs(context). Fixed locally.

Comment 29

[Wesley Johnston (:wesj)]

Comment on attachment 603309 [details] [diff] [review]
Patch v3

Moving review since dougt is out of the country.

Comment 30

[Wesley Johnston (:wesj)]

Attachment: Patch v4

Adds GeckoAppShell.getGREDir(Context context)

Comment 31

[Wesley Johnston (:wesj)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/a965cebe4462

Comment 32

[Wesley Johnston (:wesj)]

backedout: https://hg.mozilla.org/integration/mozilla-inbound/rev/5940fe8d1af9
relanded: https://hg.mozilla.org/integration/mozilla-inbound/rev/1352b8b4848b

Comment 33

[Marco Bonardo [:mak]]

https://hg.mozilla.org/mozilla-central/rev/1352b8b4848b