1319850 - Enable gecko MessageLoop in Android UI thread

Status: RESOLVED FIXED

(GeckoView :: General, defect)

Description

[Randall Barker [:rbarker]]

The Android UI thread event loop is controlled by Android and currently does not support the gecko MessageLoop. This means IPC with IPDL derived classes may not be utilized in the Android UI thread. This presents a problem when enabling the the GPU process because it needs to be possible to synchronously pause and resume the compositor from the Android UI thread. With out support for IPC in the Android UI thread the request must be made synchronously to the main thread and then synchronously sent the the compositor thread/process via IPC. By supporting MessageLoop in the Android UI thread it would not be necessary to proxy calls through the main thread loop.

Comment 1

[Randall Barker [:rbarker]]

Attachment: 0001-Bug-1319850-part-1-Add-MessagePumpForAndroidUI-r-16120109-4335f40.patch

Comment 2

[Randall Barker [:rbarker]]

Attachment: 0002-Bug-1319850-part-2-Update-MessageLoop-so-that-it-supports-MessagePumpForAndroidUI-16120109-6cfe8cf.patch

Comment 3

[Randall Barker [:rbarker]]

Attachment: 0003-Bug-1319850-part-3-Convert-AndroidBridge-PostTaskToUiThread-to-use-nsIRunnable-instead-of-mozilla-Runnable-r-jchen-16120109-a824608.patch

Comment 4

[Randall Barker [:rbarker]]

Attachment: 0004-Bug-1319850-part-4-Code-to-initialize-and-support-AndroidUI-MessageLoop-and-nsIThread-16120109-9353ff5.patch

Comment 5

[Randall Barker [:rbarker]]

Comment on attachment 8816188 [details] [diff] [review]
0001-Bug-1319850-part-1-Add-MessagePumpForAndroidUI-r-16120109-4335f40.patch

The four patches attached to this bug enable MessageLoop and nsThread in Android UI thread. Thanks for taking a look at my approach.

Comment 6

[Nathan Froyd [:froydnj]]

Comment on attachment 8816188 [details] [diff] [review]
0001-Bug-1319850-part-1-Add-MessagePumpForAndroidUI-r-16120109-4335f40.patch

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

::: ipc/glue/MessagePump.cpp
@@ +467,5 @@
> +#if defined(MOZ_WIDGET_ANDROID)
> +void
> +MessagePumpForAndroidUI::Run(Delegate* delegate)
> +{
> +  NS_WARNING("MessagePumpForAndroidUI should never be Run.");

Can we change all the NS_WARNINGs in these methods to something stronger, like MOZ_ASSERT?

::: ipc/glue/MessagePump.h
@@ +164,5 @@
>  };
>  #endif // defined(XP_WIN)
>  
> +#if defined(MOZ_WIDGET_ANDROID)
> +class MessagePumpForAndroidUI : public base::MessagePump {

There should be a big comment here describing why we have this class and especially why all the interesting methods for the class do absolutely nothing.

Comment 7

[Nathan Froyd [:froydnj]]

Comment on attachment 8816190 [details] [diff] [review]
0003-Bug-1319850-part-3-Convert-AndroidBridge-PostTaskToUiThread-to-use-nsIRunnable-instead-of-mozilla-Runnable-r-jchen-16120109-a824608.patch

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

::: widget/android/AndroidBridge.cpp
@@ +1032,5 @@
>          return mTask.forget();
>      }
>  
>  private:
> +    RefPtr<nsIRunnable> mTask;

Can you make this nsCOMPtr for consistency?

@@ +1085,5 @@
>              return timeLeft;
>          }
>  
>          // Retrieve task before unlocking/running.
> +        RefPtr<nsIRunnable> nextTask(mUiTaskQueue[0].TakeTask());

Likewise here.

@@ +1090,1 @@
>          mUiTaskQueue.RemoveElementAt(0);

Maybe we should file a followup to make this a proper queue?

Comment 8

[Nathan Froyd [:froydnj]]

Comment on attachment 8816192 [details] [diff] [review]
0004-Bug-1319850-part-4-Code-to-initialize-and-support-AndroidUI-MessageLoop-and-nsIThread-16120109-9353ff5.patch

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

::: widget/android/AndroidUiThread.cpp
@@ +18,5 @@
> +
> +static RefPtr<AndroidThread> sThread;
> +static MessageLoop* sMessageLoop;
> +
> +class AndroidThread : public nsThread

Oh man.

Can we make this an implementation of nsIEventTarget that wraps an nsCOMPtr<nsIThread> or RefPtr<nsThread> instead?  There should be some NS_EVENTTARGET_FORWARD macros or some such to ease the boilerplate, and then we can call this something more evocative than "AndroidThread".

@@ +55,5 @@
> +}
> +
> +static void
> +PumpEvents() {
> +  while(NS_ProcessNextEvent(sThread.get(), /* aMayWait */ false));

I think you can replace this with NS_ProcessPendingEvents, can't you?

@@ +97,5 @@
> +class CreateOnUiThread : public Runnable {
> +public:
> +  CreateOnUiThread() : mCreationLock("AndroidUiThreadCreationLock")
> +  {}
> +  Monitor mCreationLock;

This should be a private member variable, and probably named something to reflect that it's a monitor and not just a mutex.

@@ +121,5 @@
> +  MOZ_ASSERT(!sThread);
> +  RefPtr<CreateOnUiThread> runnable = new CreateOnUiThread;
> +  MonitorAutoLock lock(runnable->mCreationLock);
> +  AndroidBridge::Bridge()->PostTaskToUiThread(do_AddRef(runnable), 0);
> +  lock.Wait();

Waiting on monitors should always be wrapped in some kind of loop; perhaps do something like:

bool done = false;
... runnable = new CreateOnUiThread(&done);
...
while (!done) {
  lock.Wait();
}

and then have the runnable do |mDone = true| before the notify.

::: widget/android/AndroidUiThread.h
@@ +7,5 @@
> +#define AndroidUiThread_h__
> +
> +class MessageLoop;
> +
> +MessageLoop* GetAndroidUiThreadMessageLoop();

WDYT about putting this function in namespace mozilla?

::: widget/android/nsAppShell.cpp
@@ +73,5 @@
>  #else
>  #define EVLOG(args...) do { } while (0)
>  #endif
>  
> +extern void InitAndroidUiThread();

Same question here for mozilla:: namespacing.

Comment 9

[Nathan Froyd [:froydnj]]

Comment on attachment 8816188 [details] [diff] [review]
0001-Bug-1319850-part-1-Add-MessagePumpForAndroidUI-r-16120109-4335f40.patch

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

Feedback provided!

Comment 10

[Randall Barker [:rbarker]]

(In reply to Nathan Froyd [:froydnj] from comment #7)
> Comment on attachment 8816190 [details] [diff] [review]
> 0003-Bug-1319850-part-3-Convert-AndroidBridge-PostTaskToUiThread-to-use-
> nsIRunnable-instead-of-mozilla-Runnable-r-jchen-16120109-a824608.patch
> 
> Review of attachment 8816190 [details] [diff] [review]:
> -----------------------------------------------------------------
> @@ +1090,1 @@
> >          mUiTaskQueue.RemoveElementAt(0);
> 
> Maybe we should file a followup to make this a proper queue?

Bug 1322781

Comment 11

[Randall Barker [:rbarker]]

(In reply to Nathan Froyd [:froydnj] from comment #8)
> Comment on attachment 8816192 [details] [diff] [review]
> 0004-Bug-1319850-part-4-Code-to-initialize-and-support-AndroidUI-MessageLoop-
> and-nsIThread-16120109-9353ff5.patch
> 
> Review of attachment 8816192 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: widget/android/AndroidUiThread.cpp
> @@ +18,5 @@
> > +
> > +static RefPtr<AndroidThread> sThread;
> > +static MessageLoop* sMessageLoop;
> > +
> > +class AndroidThread : public nsThread
> 
> Oh man.
> 
> Can we make this an implementation of nsIEventTarget that wraps an
> nsCOMPtr<nsIThread> or RefPtr<nsThread> instead?  There should be some
> NS_EVENTTARGET_FORWARD macros or some such to ease the boilerplate, and then
> we can call this something more evocative than "AndroidThread".
> 

I don't think this will work. If someone were to obtain the nsThread from nsThreadManager::GetCurrentThread() and then dispatched to it, the runnable would get stuck in the thread runnable queue until a synchronous runnable was dispatched to the Android UI MessageLoop. Am I missing something? I can easily rename the class but I assume the name isn't the only issue?

Comment 12

[Randall Barker [:rbarker]]

Attachment: 0001-Bug-1319850-part-1-Add-MessagePumpForAndroidUI-r-16121513-399777b.patch

Comment 13

[Randall Barker [:rbarker]]

Attachment: 0002-Bug-1319850-part-2-Update-MessageLoop-so-that-it-supports-MessagePumpForAndroidUI-16121513-30dc85a.patch

Comment 14

[Randall Barker [:rbarker]]

Attachment: 0003-Bug-1319850-part-3-Convert-AndroidBridge-PostTaskToUiThread-to-use-nsIRunnable-instead-of-mozilla-Runnable-r-jchen-16121513-6992c5f.patch

Comment 15

[Randall Barker [:rbarker]]

Attachment: 0004-Bug-1319850-part-4-Code-to-initialize-and-support-AndroidUI-MessageLoop-and-nsIThread-16121513-82cca72.patch

Comment 16

[Randall Barker [:rbarker]]

Thanks for the feedback. I've uploaded new patches that address most feedback. However, I have an issue I described in Comment #11. Are those concerns incorrect?

Comment 17

[Nathan Froyd [:froydnj]]

(In reply to Randall Barker [:rbarker] from comment #11)
> I don't think this will work. If someone were to obtain the nsThread from
> nsThreadManager::GetCurrentThread() and then dispatched to it, the runnable
> would get stuck in the thread runnable queue until a synchronous runnable
> was dispatched to the Android UI MessageLoop. Am I missing something?

Bleh.  I mean, people really shouldn't be doing that on the UI thread, as it's all running Java code, right?  I guess it's probably better to not have this sort of footgun waiting for people.

OK, let's keep the current AndroidThread, but renamed (AndroidUIThread?) and with some comments describing what's going on.

Comment 18

[Nathan Froyd [:froydnj]]

Comment on attachment 8819037 [details] [diff] [review]
0004-Bug-1319850-part-4-Code-to-initialize-and-support-AndroidUI-MessageLoop-and-nsIThread-16121513-82cca72.patch

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

I just want to re-review this for the comment I suggested below.

::: widget/android/AndroidUiThread.cpp
@@ +15,5 @@
> +namespace {
> +
> +class AndroidUiThread;
> +
> +static RefPtr<AndroidUiThread> sThread;

Please use a StaticRefPtr here.

@@ +18,5 @@
> +
> +static RefPtr<AndroidUiThread> sThread;
> +static MessageLoop* sMessageLoop;
> +
> +class AndroidUiThread : public nsThread

A comment here about why we have a "real" thread vs. a wrapper around a thread object would be good, similar to the explanation you gave to me.

@@ +101,5 @@
> +
> +  NS_IMETHOD Run() override {
> +    MonitorAutoLock lock(mThreadCreationMonitor);
> +
> +    sThread = new AndroidUiThread();

I guess we just leak this and don't care, since we don't have debug and/or leak-checking build/test on Android?

Comment 19

[Randall Barker [:rbarker]]

Attachment: 0004-Bug-1319850-part-4-Code-to-initialize-and-support-AndroidUI-MessageLoop-and-nsIThread-r-nfroyd-16122015-b06ccae.patch

Comment 20

[Randall Barker [:rbarker]]

(In reply to Nathan Froyd [:froydnj] from comment #18)
> Comment on attachment 8819037 [details] [diff] [review]
> 0004-Bug-1319850-part-4-Code-to-initialize-and-support-AndroidUI-MessageLoop-
> and-nsIThread-16121513-82cca72.patch
> 
> Review of attachment 8819037 [details] [diff] [review]:
> -----------------------------------------------------------------
> @@ +101,5 @@
> > +
> > +  NS_IMETHOD Run() override {
> > +    MonitorAutoLock lock(mThreadCreationMonitor);
> > +
> > +    sThread = new AndroidUiThread();
> 
> I guess we just leak this and don't care, since we don't have debug and/or
> leak-checking build/test on Android?

I spent probably too much time investigating if this could be cleaned up. Short answer is no. For slightly longer answer see Bug 1324935. I added code to clean up on shutdown but it is currently disabled.

Comment 21

[Nathan Froyd [:froydnj]]

Comment on attachment 8820454 [details] [diff] [review]
0004-Bug-1319850-part-4-Code-to-initialize-and-support-AndroidUI-MessageLoop-and-nsIThread-r-nfroyd-16122015-b06ccae.patch

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

r=me with the comment changes below.  Thanks!

::: widget/android/AndroidUiThread.cpp
@@ +22,5 @@
> +static MessageLoop* sMessageLoop;
> +
> +/*
> + * The AndroidUiThread is derived from nsThread so that nsIRunnable objects that get
> + * dispatch may be intercepted. Only nsIRunnable objects that need to be synchronously

Nit: "dispatched"

@@ +27,5 @@
> + * executed are passed into the nsThread to be queued. All other nsIRunnable object
> + * are immediately dispatched to the Android UI thread via the AndroidBridge.
> + * AndroidUiThread is derived from nsThread instead of being an nsIEventTarget
> + * wrapper that contains an nsThread object because if nsIRunnable objects with a
> + * delay were dispatch directly to an nsThread object, they could get stuck in the

I think this would be clearer if it said something like "...to an nsThread object, such as obtained from nsThreadManager::GetCurrentThread()".

Comment 22

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/8717bea884c9
part 1, Add MessagePumpForAndroidUI r=nfroyd
https://hg.mozilla.org/integration/mozilla-inbound/rev/2a8012945a74
part 2, Update MessageLoop so that it supports MessagePumpForAndroidUI r=nfroyd
https://hg.mozilla.org/integration/mozilla-inbound/rev/e31107c3f677
part 3, Convert AndroidBridge::PostTaskToUiThread to use nsIRunnable instead of mozilla::Runnable r=jchen
https://hg.mozilla.org/integration/mozilla-inbound/rev/15b92bb6d810
part 4, Code to initialize and support AndroidUI MessageLoop and nsIThread r=nfroyd

Comment 23

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

I had to back this (and the other bug from your push) out for android xpcshell bustage like https://treeherder.mozilla.org/logviewer.html#?job_id=40966554&repo=mozilla-inbound

https://hg.mozilla.org/integration/mozilla-inbound/rev/b1213723e15613628949342a2aad4b068cd45aad

Comment 24

[Randall Barker [:rbarker]]

Try run with fix:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=78c7660face8292810b29d39e24e6308fba4bbf1

Comment 25

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/ab1843cc574e
part 1, Add MessagePumpForAndroidUI r=nfroyd
https://hg.mozilla.org/integration/mozilla-inbound/rev/7f1ec68f6e5b
part 2, Update MessageLoop so that it supports MessagePumpForAndroidUI r=nfroyd
https://hg.mozilla.org/integration/mozilla-inbound/rev/7b28041341d6
part 3, Convert AndroidBridge::PostTaskToUiThread to use nsIRunnable instead of mozilla::Runnable r=jchen
https://hg.mozilla.org/integration/mozilla-inbound/rev/e99463c6d071
part 4, Code to initialize and support AndroidUI MessageLoop and nsIThread r=nfroyd

Comment 26

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/ab1843cc574e
https://hg.mozilla.org/mozilla-central/rev/7f1ec68f6e5b
https://hg.mozilla.org/mozilla-central/rev/7b28041341d6
https://hg.mozilla.org/mozilla-central/rev/e99463c6d071