845542 - Refactor threading in AboutHomeContent

Status: RESOLVED INVALID

(Firefox for Android Graveyard :: General, defect)

Description

[Brian Nicholson (:bnicholson)]

Attachment: Refactor threading in AboutHomeContent

In AboutHomeContent, we currently execute all callbacks in update() in a background thread. This creates a mixed set of methods that are expected to run on either the background thread or the UI thread, and it's not entirely clear which without a deeper inspection of the code.

In Android, any methods that touch views are required to run on the UI thread. Background processing is generally done locally, using AsyncTasks for chunks of background processing. To make our threading patterns more explicit, I think we should follow this paradigm; that is, all methods on AboutHomeContent are on the UI thread by default, and they can fire their own (Ui)AsyncTasks for any necessary background work.

Comment 1

[Brian Nicholson (:bnicholson)]

Attachment: Refactor threading in AboutHomeContent, v2

Oops, fixed some errors that popped up after a rebase.

Comment 2

[Richard Newman [:rnewman]]

Comment on attachment 718655 [details] [diff] [review]
Refactor threading in AboutHomeContent, v2

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

I am skeptical about the cross-thread accesses in this code.

As an exercise, I encourage you to change the 'm' prefix on each member to be 'ui' or 'b', and verify that every access in this entire file is either tied to the correct thread (e.g., that there is never an onPostExecute method that touches a bFoo variable), or is wrapped in an appropriately coarse synchronization primitive.

There are no synchronization primitives in this entire file, and there are definitely cross-thread accesses, so it is definitely unsafe in some way.

Even if much of this code is likely to be safe, it rests on assumptions and accidents, rather than robust partitioning. That's not what we want going forward.

Moar, please!

::: mobile/android/base/AboutHomeContent.java
@@ +125,5 @@
>  
> +    private static class LastTabEntry {
> +        private String mUrl;
> +        private String mTitle;
> +        private Bitmap mFavicon;

These should all be `final`, and the class should be commented as being immutable. (As well as documenting what it's for!)

@@ +150,5 @@
> +    private static class AddonEntry {
> +        private String mName;
> +        private String mVersion;
> +        private String mIconUrl;
> +        private String mHomepageUrl;

Similarly.

@@ +372,5 @@
>                  updateLayout();
>  
>                  // Free the old Cursor in the right thread now.
>                  if (oldCursor != null && !oldCursor.isClosed())
>                      oldCursor.close();

Two calls to loadTopSites will result in a double free on the old cursor. Each one captures, then queues up a task to operate on it.

That this guard has to be here is a little unpleasant, and perhaps indicative of a deeper structural problem.

@@ +378,5 @@
>                  // Even if AboutHome isn't necessarily entirely loaded if we
>                  // get here, for phones this is the part the user initially sees,
>                  // so it's the one we will care about for now.
>                  if (mLoadCompleteCallback != null)
>                      mLoadCompleteCallback.callback();

Does this make sense for two racing calls to loadTopSites? I know these tasks will be executed in sequence, but that doesn't mean it's sane…

There appears to be no concurrency protection for setLoadCompleteCallback, which is executed from a runnable in BrowserApp. Potential pain.

@@ +431,5 @@
> +        // Gecko background thread, which could possibly help bug 752828.
> +        (new UiAsyncTask<Void, Void, Map<String, Bitmap>>(GeckoAppShell.getHandler()) {
> +            @Override
> +            public Map<String, Bitmap> doInBackground(Void... params) {
> +                final List<String> urls = getTopSitesUrls();

getTopSitesUrls walks a cursor retrieved from mTopSitesAdapter, all on the background thread. loadTopSites modifies mTopSitesAdapter on the UI thread. Not safe.

@@ +657,5 @@
> +                            entries.add(new AddonEntry(name, version, iconUrl, homepageUrl));
> +                        }
> +
> +                    } catch (JSONException e) {
> +                        Log.i(LOGTAG, "error reading json file", e);

Log.w, and capital letters!

Comment 3

[Brian Nicholson (:bnicholson)]

We dropped AboutHomeContent, and we're doing a redesign of about:home on fig, so this is no longer relevant.