E/GeckoLocalBrowserDB(32071): Error bulk-inserting default favicons. E/GeckoLocalBrowserDB(32071): android.database.sqlite.SQLiteConstraintException: PRIMARY KEY must be unique (code 19) E/GeckoLocalBrowserDB(32071): at android.database.sqlite.SQLiteConnection.nativeExecuteForLastInsertedRowId(Native Method) E/GeckoLocalBrowserDB(32071): at android.database.sqlite.SQLiteConnection.executeForLastInsertedRowId(SQLiteConnection.java:972) E/GeckoLocalBrowserDB(32071): at android.database.sqlite.SQLiteSession.executeForLastInsertedRowId(SQLiteSession.java:788) E/GeckoLocalBrowserDB(32071): at android.database.sqlite.SQLiteStatement.executeInsert(SQLiteStatement.java:86) E/GeckoLocalBrowserDB(32071): at android.database.sqlite.SQLiteDatabase.insertWithOnConflict(SQLiteDatabase.java:1603) E/GeckoLocalBrowserDB(32071): at android.database.sqlite.SQLiteDatabase.insertOrThrow(SQLiteDatabase.java:1499) E/GeckoLocalBrowserDB(32071): at org.mozilla.gecko.db.BrowserProvider.insertInTransaction(BrowserProvider.java:487) E/GeckoLocalBrowserDB(32071): at org.mozilla.gecko.db.AbstractTransactionalProvider.bulkInsert(AbstractTransactionalProvider.java:266) E/GeckoLocalBrowserDB(32071): at android.content.ContentProvider$Transport.bulkInsert(ContentProvider.java:233) E/GeckoLocalBrowserDB(32071): at android.content.ContentResolver.bulkInsert(ContentResolver.java:1254) E/GeckoLocalBrowserDB(32071): at org.mozilla.gecko.db.LocalBrowserDB.addDefaultBookmarks(LocalBrowserDB.java:218) E/GeckoLocalBrowserDB(32071): at org.mozilla.gecko.db.BrowserDB.addDefaultBookmarks(BrowserDB.java:50) E/GeckoLocalBrowserDB(32071): at org.mozilla.gecko.GeckoProfile$1.run(GeckoProfile.java:706) E/GeckoLocalBrowserDB(32071): at android.os.Handler.handleCallback(Handler.java:733) E/GeckoLocalBrowserDB(32071): at android.os.Handler.dispatchMessage(Handler.java:95) E/GeckoLocalBrowserDB(32071): at android.os.Looper.loop(Looper.java:136) E/GeckoLocalBrowserDB(32071): at org.mozilla.gecko.util.GeckoBackgroundThread.run(GeckoBackgroundThread.java:32) Steps to Reproduce 1. Enter Guest Mode 2. Swipe Nightly off the recent app list to close the browser 3. Re-launch the browser

Uh oh. This implies that we're not deleting the guest mode browser DB after guest mode. If we did, it'd be empty, and the constraint wouldn't be violated.

Indeed, maybeCleanupGuestProfile/removeGuestProfile deletes the guest dir, and clears shared prefs, but doesn't delete any per-profile databases. It should.

(In reply to Richard Newman [:rnewman] from comment #2) > Indeed, maybeCleanupGuestProfile/removeGuestProfile deletes the guest dir, > and clears shared prefs, but doesn't delete any per-profile databases. It > should. I'm going to be pretty grumpy if these "per-profile" DBs are not actually in the profile folder. I believe the guest profile is deleted during the startup of the non-guest profile. Maybe we are racing between the startup and the deletion.

They *should* be (it's certainly the case for non-guest profiles -- I just verified), so that's probably a red herring. I also can't repro leakage between guest profiles (pinning a site), so the DB appears to be cleared. This is probably an activity resumption thing, where we end up thinking we're creating a guest mode profile when instead we're resuming.

OK, this is an activity lifecycle logic flaw. GeckoApp#onCreate: if (GuestSession.shouldUse(this, args)) { mProfile = GeckoProfile.createGuestProfile(this); GeckoProfile#createGuestProfile: /* * Now do the things that createProfileDirectory normally does -- * right now that's kicking off DB init. */ profile.enqueueInitialization(profile.getDir()); That is, GeckoApp is assuming that if Android is instantiating the activity, this must be the first time that a guest profile has been created in this process, and that the activity *is* the app, 1:1. That's absolutely not true — just check Don't Keep Activities and open Settings! When you hit Back, Android will recreate BrowserApp. The ContentProvider and other singletons will still be hanging around, and you'll get double init. The same goes for task switching, setting up Sync, etc. etc. We have two and a half options here. 1. Tie a guest profile's lifespan to a single GeckoApp. That means that we must clean up the guest profile every time we destroy (or create!) a GeckoApp. That seems short-sighted. 2. Don't try to manage profile lifecycles from GeckoApp. GeckoProfile should do it, lazily creating when necessary, and blanking when we declare that we're done with guest browsing. 3. Band-aid this by quietly not failing when we shake hands and introduce ourselves to someone we just met. I'm going to try down the path of #2.

Testing steps taken: * Create guest profile. Kill Fennec. Resume Fennec. Same guest profile (e.g., top sites), no log message about recreation. * Exit guest mode using the menu. Verify that original profile is present. * Create guest profile again. Verify that it doesn't contain data from the first guest profile. * Use notification to end guest profile. Snags: * If BrowserApp is dismissed, the notification sticks around (Bug 1074343). Tapping it launches BrowserApp, starts the "we'll clear this profile dialog", then the activity dies with this log: W/GeckoEventDispatcher(11272): No listeners for MediaPlayer:Get W/GeckoEventDispatcher(11272): No listeners for Accessibility:Ready I/GeckoConsole(11272): Adding HealthReport:RequestSnapshot observer. D/GeckoWebapps(11272): Saving /data/data/org.mozilla.fennec_rnewman/files/webapps/webapps.json W/GeckoScreenOrientation(11272): screenOrientationFromString: unknown orientation string I/Gecko (11272): Attempting load of libEGL.so E/GeckoLayerView(11272): Error registering compositor! E/GeckoLayerView(11272): java.lang.NullPointerException E/GeckoLayerView(11272): at org.mozilla.gecko.gfx.LayerView.registerCxxCompositor(LayerView.java:525) E/GeckoLayerView(11272): at dalvik.system.NativeStart.run(Native Method) E/GeckoLayerView(11272): at dalvik.system.NativeStart.run(Native Method) I/WindowState( 423): WIN DEATH: Window{428f0548 u0 org.mozilla.fennec_rnewman/org.mozilla.gecko.BrowserApp} I/WindowState( 423): WIN DEATH: Window{4253eec0 u0 org.mozilla.fennec_rnewman/org.mozilla.gecko.BrowserApp} I/WindowState( 423): WIN DEATH: null D/mali_winsys( 633): new_window_surface returns 0x3000 D/Zygote ( 120): Process 11272 exited cleanly (11) I don't think that's anything to do with this patch, and I'll address it in Bug 1074343.

Wes, review ping?

Comment on attachment 8499321 [details] [diff] [review] Don't initialize guest profile more than once. v1 Review of attachment 8499321 [details] [diff] [review]: ----------------------------------------------------------------- One question. Can we just set mProfile in onCreate? ::: mobile/android/base/BrowserApp.java @@ +520,5 @@ > mAboutHomeStartupTimer = new Telemetry.UptimeTimer("FENNEC_STARTUP_TIME_ABOUTHOME"); > > final Intent intent = getIntent(); > + final GeckoProfile p = GeckoProfile.get(this); > + if (p != null && !p.inGuestMode()) { p can't be null now, right? ::: mobile/android/base/GeckoProfile.java @@ +100,5 @@ > + > + if (GuestSession.shouldUse(context, args)) { > + GeckoProfile p = GeckoProfile.getOrCreateGuestProfile(context); > + if (isGeckoApp) { > + ((GeckoApp) context).mProfile = p; This makes me a bit nervous. Why not just set this in GeckoApp where you get the profile? Thats similar to what we did before. Argh. I just want to delete a bunch of this :( Its getting better. Sloooooowly. @@ +202,5 @@ > return success; > } > > public static GeckoProfile createGuestProfile(Context context) { > + Log.i(LOGTAG, "Creating guest profile."); Remove this. @@ +246,5 @@ > > + /** > + * Performs IO. Be careful of using this on the main thread. > + */ > + public static GeckoProfile getOrCreateGuestProfile(Context context) { With this, createGuestProfile is essentially private (except for a test usage). getGuestProfile will be at some point (if I can ever just make GuestSession extend GeckoProfile like I want).

(In reply to Wesley Johnston (:wesj) from comment #8) > This makes me a bit nervous. Why not just set this in GeckoApp where you get > the profile? Thats similar to what we did before. Argh. I just want to > delete a bunch of this :( Its getting better. Sloooooowly. We can probably improve this, but I'd rather save that for a follow-up so we can spot any regressions it introduces. It's not quite as simple as shifting that logic to BrowserApp.onCreate. BrowserApp.onCreate touches GeckoProfile first, but GeckoApp owns mProfile, and we want this to apply to WebappImpl etc., too. And to preserve the caching behavior. > p can't be null now, right? I dunno -- we're still getting NPEs, right? Or do you mean after you land some profile reworking? > With this, createGuestProfile is essentially private (except for a test > usage). getGuestProfile will be at some point (if I can ever just make > GuestSession extend GeckoProfile like I want). One day :D

Speculative proguard fix

https://hg.mozilla.org/mozilla-central/rev/c5b5f4adfd56 https://hg.mozilla.org/mozilla-central/rev/d6dbd2f1b734