Closed Bug 1013684 Opened 5 years ago Closed 5 years ago

Distribution handling is accidentally triggered by top sites, not when it's supposed to be

Categories

(Firefox for Android :: Data Providers, defect)

All
Android
defect
Not set

Tracking

()

RESOLVED FIXED
Firefox 32

People

(Reporter: rnewman, Assigned: rnewman)

References

Details

Attachments

(1 file, 1 obsolete file)

at org.mozilla.gecko.distribution.Distribution.doInit(Distribution.java:282)
at org.mozilla.gecko.distribution.Distribution.getDistributionFile(Distribution.java:185)
at org.mozilla.gecko.distribution.Distribution.getBookmarks(Distribution.java:231)
at org.mozilla.gecko.distribution.Distribution.getBookmarks(Distribution.java:149)
at org.mozilla.gecko.db.BrowserDatabaseHelper.createDistributionBookmarks(BrowserDatabaseHelper.java:799)
at org.mozilla.gecko.db.BrowserDatabaseHelper.onCreate(BrowserDatabaseHelper.java:769)
at android.database.sqlite.SQLiteOpenHelper.getDatabaseLocked(SQLiteOpenHelper.java:252)
at android.database.sqlite.SQLiteOpenHelper.getWritableDatabase(SQLiteOpenHelper.java:164)
at org.mozilla.gecko.db.DBUtils.ensureDatabaseIsNotLocked(DBUtils.java:70)
at org.mozilla.gecko.db.PerProfileDatabases.getDatabaseHelperForProfile(PerProfileDatabases.java:67)
at org.mozilla.gecko.db.AbstractPerProfileDatabaseProvider.getReadableDatabase(AbstractPerProfileDatabaseProvider.java:43)
at org.mozilla.gecko.db.BrowserProvider.query(BrowserProvider.java:621)
at android.content.ContentProvider.query(ContentProvider.java:857)
at android.content.ContentProvider$Transport.query(ContentProvider.java:200)
at android.content.ContentResolver.query(ContentResolver.java:461)
at android.content.ContentResolver.query(ContentResolver.java:404)
at org.mozilla.gecko.db.LocalBrowserDB.getPinnedSites(LocalBrowserDB.java:1242)
at org.mozilla.gecko.db.BrowserDB.getTopSites$5514332a(BrowserDB.java:174)
at org.mozilla.gecko.home.TopSitesPanel$TopSitesLoader.loadCursor(TopSitesPanel.java:437)
at org.mozilla.gecko.home.SimpleCursorLoader.loadInBackground(SimpleCursorLoader.java:44)
at org.mozilla.gecko.home.SimpleCursorLoader.loadInBackground(SimpleCursorLoader.java:26)
at android.support.v4.content.AsyncTaskLoader.onLoadInBackground(AsyncTaskLoader.java:242)
at android.support.v4.content.AsyncTaskLoader$LoadTask.doInBackground(AsyncTaskLoader.java:51)
at android.support.v4.content.AsyncTaskLoader$LoadTask.doInBackground(AsyncTaskLoader.java:40)
at android.support.v4.content.ModernAsyncTask$2.call(ModernAsyncTask.java:123)
at java.util.concurrent.FutureTask.run(FutureTask.java:237)
at java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1112)
at java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:587)
at java.lang.Thread.run(Thread.java:841)
I have a patch for this underway.

The general approach I'm taking is to allow consumers to queue up work to be done when the distribution loader is done loading the distribution.

This becomes particularly important when loading the distribution might take fifteen seconds due to network latency.

This might end up as a useful micropattern for startup: right now we tend to rely on background runnables finishing in the right order, which is easy until they rely on something happening on the UI thread in some onCreate method.
Blocks: 1014242
Blocks: 1014283
Not yet cleaned up.
Depends on: 1014338
Attachment #8426619 - Attachment is obsolete: true
No longer blocks: 1014242, 1014283
Comment on attachment 8426714 [details] [diff] [review]
Delay loading of distribution bookmarks.

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

::: mobile/android/base/db/BrowserDatabaseHelper.java
@@ +768,5 @@
> +        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);

We should make sure to update the documentation about this change. I think we did this partially to dictate the order that thumbnails would appear in top sites, but that's changing with suggested sites anyway. Hopefully distributions won't be disappointed with our 3 default bookmarks appearing before their other bookmarks.
Attachment #8426714 - Flags: feedback+
Comment on attachment 8426714 [details] [diff] [review]
Delay loading of distribution bookmarks.

Please ignore for the moment the scary TODO and the lack of doc changes when considering review. I'll double-check concurrency before I land.
Attachment #8426714 - Flags: review?(margaret.leibovic)
Comment on attachment 8426714 [details] [diff] [review]
Delay loading of distribution bookmarks.

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

Thanks for the doc updates!
Attachment #8426714 - Flags: review?(margaret.leibovic) → review+
I'm making three changes from the reviewed patch as I land this:

1. Rather than fetching a new Distribution instance in createDistributionBookmarks, we take the one that called us as an argument. Makes sense to me.

2. Rather than queuing up a new background runnable for each distro bookmark to create the favicon, do it right then. This makes sense for several reasons:

  * All of this work is now happening in a background runnable, so yay. No worries about blocking the UI, which is why we queued the runnables in the first place.
  * We save creating 2+ runnables. Much quicker to just do the work. Yay efficiency.

3. I spotted a null safety issue with BitmapUtils.getBitmapFromDataURI, which I fixed by adding a null check.

Scream if any of that sounds bad.
https://hg.mozilla.org/mozilla-central/rev/ece2f032c42a
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 32
You need to log in before you can comment on or make changes to this bug.