Closed
Bug 1319850
Opened 8 years ago
Closed 8 years ago
Enable gecko MessageLoop in Android UI thread
Categories
(GeckoView :: General, defect)
GeckoView
General
Tracking
(firefox53 fixed)
RESOLVED
FIXED
mozilla53
Tracking | Status | |
---|---|---|
firefox53 | --- | fixed |
People
(Reporter: rbarker, Assigned: rbarker)
References
(Blocks 1 open bug)
Details
Attachments
(4 files, 5 obsolete files)
(deleted),
patch
|
froydnj
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
froydnj
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
jchen
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
froydnj
:
review+
|
Details | Diff | Splinter Review |
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•8 years ago
|
Assignee: nobody → rbarker
Assignee | ||
Comment 1•8 years ago
|
||
Assignee | ||
Comment 2•8 years ago
|
||
Assignee | ||
Comment 3•8 years ago
|
||
Assignee | ||
Comment 4•8 years ago
|
||
Assignee | ||
Comment 5•8 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 6•8 years ago
|
||
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•8 years ago
|
||
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•8 years ago
|
||
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•8 years ago
|
||
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•8 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•8 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•8 years ago
|
||
Attachment #8816188 -
Attachment is obsolete: true
Assignee | ||
Comment 13•8 years ago
|
||
Attachment #8816189 -
Attachment is obsolete: true
Assignee | ||
Comment 14•8 years ago
|
||
Attachment #8816190 -
Attachment is obsolete: true
Assignee | ||
Comment 15•8 years ago
|
||
Attachment #8816192 -
Attachment is obsolete: true
Assignee | ||
Comment 16•8 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)
Comment 17•8 years ago
|
||
(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•8 years ago
|
Attachment #8819034 -
Flags: review?(nfroyd)
Assignee | ||
Updated•8 years ago
|
Attachment #8819035 -
Flags: review?(nfroyd)
Assignee | ||
Updated•8 years ago
|
Attachment #8819036 -
Flags: review?(nchen)
Assignee | ||
Updated•8 years ago
|
Attachment #8819037 -
Flags: review?(nfroyd)
Updated•8 years ago
|
Attachment #8819036 -
Flags: review?(nchen) → review+
Updated•8 years ago
|
Attachment #8819034 -
Flags: review?(nfroyd) → review+
Updated•8 years ago
|
Attachment #8819035 -
Flags: review?(nfroyd) → review+
Comment 18•8 years ago
|
||
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 | ||
Comment 19•8 years ago
|
||
Attachment #8819037 -
Attachment is obsolete: true
Assignee | ||
Comment 20•8 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•8 years ago
|
Attachment #8820454 -
Flags: review?(nfroyd)
Comment 21•8 years ago
|
||
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•8 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)
Assignee | ||
Comment 24•8 years ago
|
||
Try run with fix:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=78c7660face8292810b29d39e24e6308fba4bbf1
Flags: needinfo?(rbarker)
Comment 25•8 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•8 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
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 53
Updated•6 years ago
|
Product: Firefox for Android → GeckoView
Updated•6 years ago
|
Target Milestone: Firefox 53 → mozilla53
You need to log in
before you can comment on or make changes to this bug.
Description
•