Don't process distribution files for webapp profiles

RESOLVED FIXED in Firefox 33

Status

()

defect
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: rnewman, Assigned: rnewman)

Tracking

32 Branch
Firefox 33
All
Android
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(5 attachments)

Just what it says on the tin: we don't need this stuff happening during webapp startup, because they won't use the data.
Depends on: 1021342
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.
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).
(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.
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.
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.
Flags: needinfo?(myk)
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 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.
Attachment #8438888 - Flags: review?(margaret.leibovic)
Assignee: nobody → rnewman
Status: NEW → ASSIGNED
(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!
Flags: needinfo?(myk)
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.
Attachment #8438888 - Flags: review?(margaret.leibovic) → review+
Comments addressed:

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

Thanks, Margaret!
Thanks, Wes. These tests are now racy, so I guess I'd better fix them!
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.
(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?
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.
Flags: needinfo?(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.
I spotted a bunch of unused code and unnecessarily non-private methods. Fixxored.
Attachment #8442198 - Flags: review?(margaret.leibovic)
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
Attachment #8442199 - Flags: review?(margaret.leibovic)
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.
Attachment #8442204 - Flags: review?(margaret.leibovic)
This patch makes it acceptable (if noisy) to screw up while inserting default or distribution bookmarks.
Attachment #8442208 - Flags: review?(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 :)
Attachment #8442198 - Flags: review?(margaret.leibovic) → review+
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 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.
Attachment #8442199 - Flags: review?(margaret.leibovic) → review+
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?
Attachment #8442208 - Flags: review?(margaret.leibovic) → review+
(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.)
> > +     * 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 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! :)
Attachment #8442204 - Flags: review?(margaret.leibovic) → review+
Depends on: 1030770
Depends on: 1040806
You need to log in before you can comment on or make changes to this bug.