1016611 - Don't process distribution files for webapp profiles

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Richard Newman [:rnewman]]

Just what it says on the tin: we don't need this stuff happening during webapp startup, because they won't use the data.

Comment 1

[Richard Newman [:rnewman]]

The root of this problem:

Our BrowserProvider is inited when the app launches. Even if it weren't, it would be touched as soon as a history URL needed to be recorded.

The database doesn't exist (new profile), so it is created.

BrowserDatabaseHelper's onCreate method creates tables, creates special folders, and then:


        int offset = createDefaultBookmarks(db, 0);

        // We'd like to create distribution bookmarks before our own default bookmarks,
        // but we won't necessarily have loaded the distribution yet. Oh well.
        enqueueDistributionBookmarkLoad(db, offset);

        createReadingListTable(db);


The issue here: BrowserDatabaseHelper doesn't know that this is a webapp profile. Indeed, it's on the other side of a ContentProvider boundary, so it's not cuddling up to GeckoApp to figure that out.

The right answer is likely to be to divorce the CP/DB init and the loading of default content, but that's not quite as simple as it sounds.

I'm going to think about this some more.

Comment 2

[Wesley Johnston (:wesj)]

I'd be happy if we could disable all of this database code in webapps. We definitely don't need bookmarks or favicons there. We don't really need history either (although that may cause some compatibility issues).

Comment 3

[Richard Newman [:rnewman]]

(In reply to Wesley Johnston (:wesj) from comment #2)
> I'd be happy if we could disable all of this database code in webapps. We
> definitely don't need bookmarks or favicons there. We don't really need
> history either (although that may cause some compatibility issues).

Yeah, history was my concern. Moving BrowserDB.initialize out of GeckoApp might be enough to avoid initing the DB, but that's no good if it'll just get touched as soon as a page loads.

Comment 4

[Richard Newman [:rnewman]]

Opinions welcome on this approach:

Trigger loading of default bookmarks and distribution bookmarks from GeckoProfile, at the end of profile creation.

GeckoProfile already knows whether this is a webapp or a regular profile. It also knows that we're creating the profile, which means browser.db is going to be created shortly thereafter.

We'll remove DB initialization from GeckoApp (GeckoProfile will take care of it), and shift default bookmark creation out of BrowserDatabaseHelper.onCreate.

This will avoid initing browser DB from webapps at all (until they record a history item, that is), and even then they won't have bookmarks added, and nor will they trigger distribution processing.

Comment 5

[Richard Newman [:rnewman]]

I should note that in theory, one could look for "webapp" in the DB path inside BrowserDatabaseHelper, which would be a two-line workaround for this. That's kinda hacky and awful, though -- I don't want to spread that kind of specific knowledge around the codebase.

Comment 6

[Mark Finkle (:mfinkle) (use needinfo?)]

There is an elegance to the approach described in comment 4. It feels like a good approach. Initializing DBs in the code that creates the profile makes sense to me.

Comment 7

[Richard Newman [:rnewman]]

Attachment: Don't process distribution files for webapp profiles. v1

I think this is ready for preliminary review.

I've tested the upgrade case, the clean install case, and the live distro downloading case. And I tested a webapp, of course. I haven't yet run automated tests, and I haven't tested the case of a distro download failure.

Contents:

* Assorted cleanup (e.g., imports). Sorry this isn't in a Part 0, but I was painting with a broad brush.
* Reorg and reimplementation for bookmark addition: LocalBrowserDB contains all the code for loading default and distribution bookmarks, and it's all modern and ContentValues-based rather than directly inserting.

Benefits:
* We can put distribution bookmarks first again!
* Possibly faster, despite using the CP instead of being inside BrowserDatabaseHelper: we use at most four bulkInserts, rather than making 2 * (N+M) DB calls to add two sets of bookmarks and favicons. Oh, and we don't need to pile up runnables for favicons!

And finally, to fix the bug:
* GeckoProfile knows if it's a webapp, and doesn't init the DB in that case.

I welcome driveby reviews on this, as always.

Comment 8

[Richard Newman [:rnewman]]

Try push:

https://tbpl.mozilla.org/?tree=Try&rev=eddef2736a41

Comment 9

[Myk Melez [:myk] [@mykmelez]]

(In reply to Mark Finkle (:mfinkle) from comment #6)
> There is an elegance to the approach described in comment 4. It feels like a
> good approach. Initializing DBs in the code that creates the profile makes
> sense to me.

I agree, it's a better place for it than GeckoApp!

Comment 10

[:Margaret Leibovic]

Comment on attachment 8438888 [details] [diff] [review]
Don't process distribution files for webapp profiles. v1

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

Looks good to me. For a faster review next time, split big chunks of copy/paste into a separate patch from clean-up fixes.

::: mobile/android/base/GeckoProfile.java
@@ +629,5 @@
> +
> +    /**
> +     * This method is called once, immediately before creation of the profile
> +     * directory completes.
> +     * 

Nit: whitespace

::: mobile/android/base/db/LocalBrowserDB.java
@@ +171,5 @@
> +                // Reflection failure.
> +            } catch (IllegalArgumentException e) {
> +                // Reflection failure.
> +            } catch (NoSuchFieldException e) {
> +                // Reflection failure.

We don't want to log in any of these failure cases?

@@ +176,5 @@
> +            }
> +        }
> +
> +        cr.bulkInsert(mFaviconsUriWithProfile, faviconValues.toArray(new ContentValues[faviconValues.size()]));
> +        final int inserted = cr.bulkInsert(mBookmarksUriWithProfile, bookmarkValues.toArray(new ContentValues[bookmarkValues.size()]));

Nice improvement over the individual inserts we were doing before.

@@ +349,5 @@
> +            return BitmapUtils.decodeResource(context, faviconId);
> +        } catch (java.lang.IllegalAccessException ex) {
> +            Log.e(LOGTAG, "[Drawable] Can't create favicon " + name, ex);
> +        } catch (java.lang.NoSuchFieldException ex) {
> +            // If the field does not exist, that means we intend to load via a file path.

This comment is technically wrong, since we would have already tried to load via a path and failed.

Comment 11

[Richard Newman [:rnewman]]

Comments addressed:

https://hg.mozilla.org/integration/fx-team/rev/62cce2cf895f

Thanks, Margaret!

Comment 12

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

I had to back this out in https://hg.mozilla.org/integration/fx-team/rev/7f3eddd4540f for causing some android failures: https://tbpl.mozilla.org/php/getParsedLog.php?id=41829085&tree=Fx-Team and https://tbpl.mozilla.org/php/getParsedLog.php?id=41827414&tree=Fx-Team

Comment 13

[Richard Newman [:rnewman]]

Thanks, Wes. These tests are now racy, so I guess I'd better fix them!

Comment 14

[Richard Newman [:rnewman]]

I just noticed: when we add default bookmarks, we *guess the favicon URL from the page URL*. That's so monumentally wrong that I might fix it when I reland this.

Apparently sometimes we guess a duplicate, too -- but only in robocop tests, if I force Robocop to do some of the work that Fennec does for real profiles.

There's a lot of wtf here.

Comment 15

[Wesley Johnston (:wesj)]

(In reply to Richard Newman [:rnewman] from comment #14)
> I just noticed: when we add default bookmarks, we *guess the favicon URL
> from the page URL*. That's so monumentally wrong that I might fix it when I
> reland this.

I'm not sure what you mean here, so I went back and looked. We read the favicon path out of our localization files, or if that fails, we look for a drawable with the right name:

http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/db/BrowserDatabaseHelper.java#946

Where are you seeing this "guess" based on the page url?

Comment 16

[Richard Newman [:rnewman]]

We prepare ContentValues with the page URL and no favicon URL. BrowserProvider will remove the page URL and guess a favicon URL for insertion into the favicon table.

Comment 17

[Richard Newman [:rnewman]]

rpr is still being a monster. And the log is full of wtf, because GeckoProfile seems to be non-thread-safe:

22:23:31     INFO -  06-17 22:23:28.734 D/GeckoProfile( 2184): Created new profile dir.
22:23:31     INFO -  06-17 22:23:28.734 D/Telemetry( 2184): StartUISession: firstrun.1
22:23:31     INFO -  06-17 22:23:28.750 I/GeckoProfile( 2184): Enqueuing profile init.
...
22:23:31     INFO -  06-17 22:23:28.773 D/GeckoProfile( 2184): Running post-distribution task: bookmarks.
...
22:23:31     INFO -  06-17 22:23:29.734 D/GeckoProfile( 2184): Created new profile dir.
22:23:31     INFO -  06-17 22:23:29.734 I/GeckoProfile( 2184): Enqueuing profile init.
22:23:31     INFO -  06-17 22:23:29.734 D/GeckoProfile( 2184): Running post-distribution task: bookmarks.


Goddamnit.

Comment 18

[Richard Newman [:rnewman]]

Attachment: Part 2: further cleanup. v1

I spotted a bunch of unused code and unnecessarily non-private methods. Fixxored.

Comment 19

[Richard Newman [:rnewman]]

Attachment: Part 3: basic thread safety in GeckoProfile. v1

Our rpr failures with this change seem to be due to GeckoProfile creating the profile directory *twice* (see Comment 17). This is an old bug that now has consequences.

GeckoView inits the profile, which creates the profile dir on the UI thread, but also any accesses to the profile dir -- of which there are many, including on the background thread -- will result in it being created. Oops.

GeckoProfile is totally not thread-safe. This patch makes it somewhat less unsafe: access to mProfileDir (which is the main culprit) is now entirely synchronized. While I was in the neighborhood, I made some of the other members volatile for somewhat less risk, but there's probably more work to do here.

Try push is running:

https://tbpl.mozilla.org/?tree=Try&rev=5bf08de5ec8a

Comment 20

[Richard Newman [:rnewman]]

Attachment: Part 4: fix testBookmarksPanel. v1

testBookmarksPanel assumes that the default bookmarks have been added. But they haven't necessarily been added by the time we run; indeed, I also observed that the creation methods weren't called at all!

Here we make sure that the profile is initialized before we do any checks. There is a potential timing issue here, but in practice it hasn't been problematic.

A follow-up part ensures that double-initialization (or initialization with conflicting contents already in the DB) isn't harmful. This is something we don't expect to happen, but better not to die if it does!

One situation in which it *does* happen is if you run Robocop from inside Eclipse. That uses the default profile without cleaning it up, which is a source of pain.

Comment 21

[Richard Newman [:rnewman]]

Attachment: Part 5: be resilient against failed insertions. v1

This patch makes it acceptable (if noisy) to screw up while inserting default or distribution bookmarks.

Comment 22

[:Margaret Leibovic]

Comment on attachment 8442198 [details] [diff] [review]
Part 2: further cleanup. v1

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

LGTM if it builds :)

Comment 23

[Richard Newman [:rnewman]]

Here's the second stack that was causing directory creation:

org.mozilla.gecko.GeckoProfile.createProfileDir(GeckoProfile.java:593)
org.mozilla.gecko.GeckoProfile.forceCreate(GeckoProfile.java:400)
org.mozilla.gecko.GeckoProfile.getDir(GeckoProfile.java:384)
org.mozilla.gecko.db.PerProfileDatabases.getDatabaseHelperForProfile(PerProfileDatabases.java:61)
org.mozilla.gecko.db.AbstractPerProfileDatabaseProvider.getReadableDatabase(AbstractPerProfileDatabaseProvider.java:43)
org.mozilla.gecko.db.BrowserProvider.query(BrowserProvider.java:615)
android.content.ContentProvider$Transport.query(ContentProvider.java:178)
android.content.ContentResolver.query(ContentResolver.java:311)
org.mozilla.gecko.db.LocalBrowserDB.filterAllSites(LocalBrowserDB.java:463)
org.mozilla.gecko.db.LocalBrowserDB.filter(LocalBrowserDB.java:511)
org.mozilla.gecko.tests.testBrowserProviderPerf.testBrowserProviderQueryPerf(testBrowserProviderPerf.java:273)


I don't see double-creation with these patches:

https://tbpl.mozilla.org/?tree=Try&rev=34536e00603a

Comment 24

[:Margaret Leibovic]

Comment on attachment 8442199 [details] [diff] [review]
Part 3: basic thread safety in GeckoProfile. v1

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

I verified the other uses of mProfileDir are already in synchronized methods.

::: mobile/android/base/GeckoProfile.java
@@ +50,5 @@
>      private final Context mApplicationContext;
>  
> +    /**
> +     * Access to this member should be synchronized to avoid
> +     * races during creation -- particularly between getDir and init.

What "init" is this comment referring to?

@@ +340,5 @@
> +        synchronized (this) {
> +            profileDir = mProfileDir;
> +        }
> +
> +        if (profileDir == null || !profileDir.exists()) {

This is inconsistent with how you did this up above in locked. To be conistent, you could just create a profileExists boolean here as well.

@@ +345,5 @@
>              return true;
>          }
>  
>          try {
> +            final File lockFile = new File(mProfileDir, LOCK_FILE_NAME);

But if you do keep the profileDir local variable, you could use it here.

Comment 25

[:Margaret Leibovic]

Comment on attachment 8442204 [details] [diff] [review]
Part 4: fix testBookmarksPanel. v1

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

::: mobile/android/base/tests/testBookmarksPanel.java
@@ +22,5 @@
>          openAboutHomeTab(AboutHomeTabs.BOOKMARKS);
>  
> +        // Check that the default bookmarks are displayed.
> +        // We need to wait for the distribution to have been processed
> +        // before this will succeed.

Are you just counting on the race to work? Is there a way to listen for when the default bookmarks are added? Or can we add a wait loop that waits and re-tries if the bookmarks aren't ready yet?

Comment 26

[Richard Newman [:rnewman]]

(In reply to :Margaret Leibovic from comment #25)

> Are you just counting on the race to work? Is there a way to listen for when
> the default bookmarks are added? Or can we add a wait loop that waits and
> re-tries if the bookmarks aren't ready yet?

Pretty much. There's no distribution, so the default bookmark work should be immediately queued on the background thread. Then the test waits for switching about:home, so we're pretty much guaranteed that the DB init will finish first. I've never seen this race while running this test.

(Of course, none of this should be necessary, because the first access to the profile dir should cause the DB to be inited, so this *should* all be academic.)

Comment 27

[Richard Newman [:rnewman]]

> > +     * races during creation -- particularly between getDir and init.
> 
> What "init" is this comment referring to?

GeckoView#init, which calls forceCreate. Clarified the comment.


> But if you do keep the profileDir local variable, you could use it here.

Yup, that's what I was aiming for :) Fixed.

Comment 28

[:Margaret Leibovic]

Comment on attachment 8442204 [details] [diff] [review]
Part 4: fix testBookmarksPanel. v1

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

There are definitely worse assumptions in our robocop code. But you're on the hook for any intermittent oranges! :)

Comment 29

[Richard Newman [:rnewman]]

Green try run:

https://tbpl.mozilla.org/?tree=Try&rev=b09fed45e02f

Landed in multiple parts for clarity:

   https://hg.mozilla.org/integration/fx-team/rev/6460524ba1a9
   https://hg.mozilla.org/integration/fx-team/rev/2b3bcade4155
   https://hg.mozilla.org/integration/fx-team/rev/270e99e71b91
   https://hg.mozilla.org/integration/fx-team/rev/7834fa7dbb95
   https://hg.mozilla.org/integration/fx-team/rev/e38671421b22

Thanks for the quick review, Margaret!

Comment 30

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/6460524ba1a9
https://hg.mozilla.org/mozilla-central/rev/2b3bcade4155
https://hg.mozilla.org/mozilla-central/rev/270e99e71b91
https://hg.mozilla.org/mozilla-central/rev/7834fa7dbb95
https://hg.mozilla.org/mozilla-central/rev/e38671421b22