Enable gecko MessageLoop in Android UI thread

RESOLVED FIXED in Firefox 53

Status

RESOLVED FIXED
2 years ago
2 months ago

People

(Reporter: rbarker, Assigned: rbarker)

Tracking

(Blocks: 1 bug)

Trunk
mozilla53
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

2 years 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

2 years ago
Assignee: nobody → rbarker
(Assignee)

Comment 1

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

Comment 2

2 years 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

2 years 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

2 years 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

2 years ago
Depends on: 1321642
(Assignee)

Updated

2 years ago
Blocks: 1321644
(Assignee)

Comment 5

2 years 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

2 years 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

2 years 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

2 years 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

2 years 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

2 years 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

2 years 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

2 years 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

2 years ago
Attachment #8819034 - Flags: review?(nfroyd)
(Assignee)

Updated

2 years ago
Attachment #8819035 - Flags: review?(nfroyd)
(Assignee)

Updated

2 years ago
Attachment #8819036 - Flags: review?(nchen)
(Assignee)

Updated

2 years 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

2 years ago
Blocks: 1324935
(Assignee)

Updated

2 years ago
Blocks: 1322781
(Assignee)

Comment 19

2 years 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

2 years 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

2 years 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

2 years 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

2 years 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

2 years 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: 2 years ago
status-firefox53: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 53

Updated

2 years ago
Depends on: 1326091
(Assignee)

Updated

2 years ago
Blocks: 1328747

Updated

2 months ago
Product: Firefox for Android → GeckoView
Target Milestone: Firefox 53 → mozilla53
You need to log in before you can comment on or make changes to this bug.