730528 - Extract LooperThread from GeckoAppShell

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect, P3)

Description

[Richard Newman [:rnewman]]

BrowserProvider (and the other Content Providers that are spawning) relies on precisely one part of GeckoAppShell:

    @Override
    public boolean onCreate() {
        debug("Creating BrowserProvider");

        GeckoAppShell.getHandler().post(new Runnable() {
            public void run() {
                // Kick this off early. It is synchronized so that other callers will wait
                try {
                    GeckoDirProvider.getProfileDir(getContext());
                } catch (Exception ex) {
                    Log.e(LOGTAG, "Error getting profile dir", ex);
                }
            }
        });
        synchronized (this) {
            mContext = getContext();
            mDatabasePerProfile = new HashMap<String, DatabaseHelper>();
        }

        return true;
    }


In doing so, it has to import GeckoAppShell, which allocates a bunch of static data structures and imports The Entire Universe. This seems like a waste of effort when it's just Sync trying to poke at a database.

Looking at the definition of LooperThread:

    // A looper thread, accessed by GeckoAppShell.getHandler
    private static class LooperThread extends Thread {
        public SynchronousQueue<Handler> mHandlerQueue =
            new SynchronousQueue<Handler>();
        
        public void run() {
            setName("GeckoLooper Thread");
            Looper.prepare();
            try {
                mHandlerQueue.put(new Handler());
            } catch (InterruptedException ie) {}
            Looper.loop();
        }
    }


and getHandler:

    // Get a Handler for a looper thread, or create one if it doesn't exist yet
    public static Handler getHandler() {
        if (sHandler == null) {
            LooperThread lt = new LooperThread();
            lt.start();
            try {
                sHandler = lt.mHandlerQueue.take();
            } catch (InterruptedException ie) {}

        }
        return sHandler;
    }

it seems like one could quite easily extract LooperThread to be its own class with a singleton accessor, and have both GeckoDirProvider and GeckoAppShell use LooperThread.

Am I crazy?

Unless someone screams blue murder, I'll put together a small patch for this.

Comment 1

[Richard Newman [:rnewman]]

Also, look for the glaring concurrency bug in getHandler that will result in two LooperThreads being started. Fun stuff.

Comment 2

[Richard Newman [:rnewman]]

Attachment: Part 1: extract LooperThread. v1

* Extract LooperThread.
* Use MPL2.0 in new file.
* Give it a private constructor.
* Fix race condition by making getHandler synchronized. Very small overhead in the success case, and infrequently called.
* Leave GeckoAppShell.getHandler to avoid touching other files or introducing imports elsewhere.

Comment 3

[Richard Newman [:rnewman]]

Attachment: Part 2: remove GeckoAppShell dependency from BrowserProvider. v1

* Add an ensureProfileDir method to GeckoDirProvider, lifting the contents out of BrowserProvider. (These were going to breed in each of the new providers, so it's good to lift them out.)
* Call this from BrowserProvider.
* Remove GeckoAppShell import.

I've tested with my own build locally, and browsing seems to work fine.

Try build (including Bug 730526) is here:

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

Comment 4

[Richard Newman [:rnewman]]

Attachment: Part 1: extract LooperThread. v2

Package declaration ended up in the wrong patch file.

Comment 5

[Richard Newman [:rnewman]]

Another try build, demonstrating the sadness that is our test suite (https://bugzilla.mozilla.org/show_bug.cgi?id=686084#c672).

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

Comment 6

[Richard Newman [:rnewman]]

Attachment: Extract LooperThread. v3

Altered slightly after the introduction of GeckoProfile. The shared call to ensure that the profile directory exists should be lifted out of the individual Provider classes, but I won't do that here.

Comment 7

[Brad Lassey [:blassey] (use needinfo?)]

Comment on attachment 600825 [details] [diff] [review]
Extract LooperThread. v3

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

just a naming nit, LooperThread isn't very descriptive (not that GeckoAppShell.getHandler() is either). How about GeckoBackgroundThread?

Comment 8

[Richard Newman [:rnewman]]

Thanks for the quick review!

Pushed to inbound with nit addressed:

https://hg.mozilla.org/integration/mozilla-inbound/rev/57504003e273

I also took the liberty of adding a (safe) `push` method to GeckoBackgroundThread; there's a NPE waiting to happen in any use of getHandler(), so this fixes the one callsite that I was already addressing.

Comment 9

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

https://hg.mozilla.org/mozilla-central/rev/57504003e273