Enable gecko MessageLoop in Android UI thread

RESOLVED FIXED in Firefox 53

Status

()

Firefox for Android
GeckoView
RESOLVED FIXED
a year ago
a year ago

People

(Reporter: rbarker, Assigned: rbarker)

Tracking

(Blocks: 1 bug)

Trunk
Firefox 53
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox53 fixed)

Details

Attachments

(4 attachments, 5 obsolete attachments)

2.86 KB, patch
froydnj
: review+
Details | Diff | Splinter Review
1.99 KB, patch
froydnj
: review+
Details | Diff | Splinter Review
4.12 KB, patch
jchen
: review+
Details | Diff | Splinter Review
10.38 KB, patch
froydnj
: review+
Details | Diff | Splinter Review
(Assignee)

Description

a year ago
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.
(Assignee)

Updated

a year ago
Assignee: nobody → rbarker
(Assignee)

Comment 1

a year ago
Created attachment 8816188 [details] [diff] [review]
0001-Bug-1319850-part-1-Add-MessagePumpForAndroidUI-r-16120109-4335f40.patch
(Assignee)

Comment 2

a year ago
Created attachment 8816189 [details] [diff] [review]
0002-Bug-1319850-part-2-Update-MessageLoop-so-that-it-supports-MessagePumpForAndroidUI-16120109-6cfe8cf.patch
(Assignee)

Comment 3

a year ago
Created 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
(Assignee)

Comment 4

a year ago
Created attachment 8816192 [details] [diff] [review]
0004-Bug-1319850-part-4-Code-to-initialize-and-support-AndroidUI-MessageLoop-and-nsIThread-16120109-9353ff5.patch
(Assignee)

Updated

a year ago
Depends on: 1321642
(Assignee)

Updated

a year ago
Blocks: 1321644
(Assignee)

Comment 5

a year ago
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.
Attachment #8816188 - Flags: feedback?(nfroyd)
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 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 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 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!
Attachment #8816188 - Flags: feedback?(nfroyd) → feedback+
(Assignee)

Comment 10

a year ago
(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
(Assignee)

Comment 11

a year ago
(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?
(Assignee)

Comment 12

a year ago
Created attachment 8819034 [details] [diff] [review]
0001-Bug-1319850-part-1-Add-MessagePumpForAndroidUI-r-16121513-399777b.patch
Attachment #8816188 - Attachment is obsolete: true
(Assignee)

Comment 13

a year ago
Created attachment 8819035 [details] [diff] [review]
0002-Bug-1319850-part-2-Update-MessageLoop-so-that-it-supports-MessagePumpForAndroidUI-16121513-30dc85a.patch
Attachment #8816189 - Attachment is obsolete: true
(Assignee)

Comment 14

a year ago
Created attachment 8819036 [details] [diff] [review]
0003-Bug-1319850-part-3-Convert-AndroidBridge-PostTaskToUiThread-to-use-nsIRunnable-instead-of-mozilla-Runnable-r-jchen-16121513-6992c5f.patch
Attachment #8816190 - Attachment is obsolete: true
(Assignee)

Comment 15

a year ago
Created attachment 8819037 [details] [diff] [review]
0004-Bug-1319850-part-4-Code-to-initialize-and-support-AndroidUI-MessageLoop-and-nsIThread-16121513-82cca72.patch
Attachment #8816192 - Attachment is obsolete: true
(Assignee)

Comment 16

a year ago
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?
Flags: needinfo?(nfroyd)
(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.
Flags: needinfo?(nfroyd)
(Assignee)

Updated

a year ago
Attachment #8819034 - Flags: review?(nfroyd)
(Assignee)

Updated

a year ago
Attachment #8819035 - Flags: review?(nfroyd)
(Assignee)

Updated

a year ago
Attachment #8819036 - Flags: review?(nchen)
(Assignee)

Updated

a year ago
Attachment #8819037 - Flags: review?(nfroyd)
Attachment #8819036 - Flags: review?(nchen) → review+
Attachment #8819034 - Flags: review?(nfroyd) → review+
Attachment #8819035 - Flags: review?(nfroyd) → review+
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?
Attachment #8819037 - Flags: review?(nfroyd) → feedback+
(Assignee)

Updated

a year ago
Blocks: 1324935
(Assignee)

Updated

a year ago
Blocks: 1322781
(Assignee)

Comment 19

a year ago
Created attachment 8820454 [details] [diff] [review]
0004-Bug-1319850-part-4-Code-to-initialize-and-support-AndroidUI-MessageLoop-and-nsIThread-r-nfroyd-16122015-b06ccae.patch
Attachment #8819037 - Attachment is obsolete: true
(Assignee)

Comment 20

a year ago
(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.
(Assignee)

Updated

a year ago
Attachment #8820454 - Flags: review?(nfroyd)
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()".
Attachment #8820454 - Flags: review?(nfroyd) → review+

Comment 22

a year ago
Pushed by rbarker@mozilla.com:
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
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
Flags: needinfo?(rbarker)

Comment 25

a year ago
Pushed by rbarker@mozilla.com:
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

a year ago
bugherder
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
Status: NEW → RESOLVED
Last Resolved: a year ago
status-firefox53: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 53

Updated

a year ago
Depends on: 1326091
(Assignee)

Updated

a year ago
Blocks: 1328747
You need to log in before you can comment on or make changes to this bug.