Currently Gecko for Android does not support a GPU process. In order to support the GPU process it is necessary for the Android nsWindow not access the CompositorBridgeParent since it will not be present in the parent process when a GPU process is created. The nsWindow currently access the CompositorBridgeParent to perform operations such as pause and resume the compositor. Normally the code would be ported to use the CompositorBridgeChild. Unfortunately control of the compositor is performed from the Android UI thread where the CompositorBridgeChild may not be accessed. It will be necessary to create a new top level protocol to control the compositor that may be accessed from the Android UI thread.

You should definitely talk to dvander before you go too far down this road.

(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #2) > You should definitely talk to dvander before you go too far down this road. I did in Hawaii. This patch is what he and I agreed upon.

OK, thanks!

Comment on attachment 8824241 [details] [diff] [review] 0002-Bug-1328752-part-2-Update-Android-nsWindow-to-use-UiCompositorControllerChild-inplace-of-CompositorBridgeParent-17010514-69fbfa1.patch Review of attachment 8824241 [details] [diff] [review]: ----------------------------------------------------------------- ::: widget/android/nsWindow.cpp @@ +1120,1 @@ > if (LockedWindowPtr window{mWindow}) { We should bail if this is false (i.e. `window` is nullptr). @@ +1120,2 @@ > if (LockedWindowPtr window{mWindow}) { > + id = window->GetRootLayerId(); Also seems like if this fails (`GetRootLayerId` returns 0) we should bail too? @@ +1136,1 @@ > if (LockedWindowPtr window{mWindow}) { Same @@ +1153,1 @@ > if (LockedWindowPtr window{mWindow}) { Same, but we want `mSurface = aSurface;` to run even if this is false. @@ +1198,1 @@ > if (LockedWindowPtr window{mWindow}) { Same @@ +1207,5 @@ > > + if (!AndroidBridge::IsJavaUiThread()) { > + RefPtr<nsThread> uiThread = GetAndroidUiThread(); > + if (uiThread) { > + uiThread->Dispatch(NewRunnableMethod<const int64_t&>(child, &UiCompositorControllerChild::DoInvalidateAndRender, id), nsIThread::DISPATCH_NORMAL); Two lines. Also ideally we want to call `SyncInvalidateAndScheduleComposite` again, because it checks to see the window still exists. @@ +1214,3 @@ > } > + > + MOZ_ASSERT(AndroidBridge::IsJavaUiThread()); Don't need this assert.

(In reply to Jim Chen [:jchen] [:darchons] from comment #7) > Comment on attachment 8824241 [details] [diff] [review] > 0002-Bug-1328752-part-2-Update-Android-nsWindow-to-use- > UiCompositorControllerChild-inplace-of-CompositorBridgeParent-17010514- > 69fbfa1.patch > > Review of attachment 8824241 [details] [diff] [review]: > ----------------------------------------------------------------- > > ::: widget/android/nsWindow.cpp > @@ +1207,5 @@ > > > > + if (!AndroidBridge::IsJavaUiThread()) { > > + RefPtr<nsThread> uiThread = GetAndroidUiThread(); > > + if (uiThread) { > > + uiThread->Dispatch(NewRunnableMethod<const int64_t&>(child, &UiCompositorControllerChild::DoInvalidateAndRender, id), nsIThread::DISPATCH_NORMAL); > > Two lines. Also ideally we want to call `SyncInvalidateAndScheduleComposite` > again, because it checks to see the window still exists. > Calling SyncInvalidateAndScheduleComposite again was my initial thought but since nsWindow::LayerViewSupport isn't ref counted I would have to use NewNonOwningRunnableMethod which would make checking if the window is still valid pointless since if window was deleted nsWindow then LayerViewSupport would also be deleted? Or is that wrong? At least the UiCompositorControllerChild is ref counted (and also a singleton) so it isn't going to get deleted between calls and can handle the case if the compositor associated with the root layer id no longer exists. Additionally, after sleeping on this, I'm inclined to change this so that it always dispatches on the main thread instead of the UI thread. That would make the UICompositorController simpler. Any thoughts?

Address review comments.

(In reply to Randall Barker [:rbarker] from comment #8) > Additionally, after sleeping on this, I'm inclined to change this so that it > always dispatches on the main thread instead of the UI thread. That would > make the UICompositorController simpler. Any thoughts? After discussion on irc it was decided to leave the dispatch on the UI thread as there didn't appear to be any value in changing dispatch to the main thread.

Comment on attachment 8824556 [details] [diff] [review] 0002-Bug-1328752-part-2-Update-Android-nsWindow-to-use-UiCompositorControllerChild-inplace-of-CompositorBridgeParent-17010613-4e64b45.patch Review of attachment 8824556 [details] [diff] [review]: ----------------------------------------------------------------- ::: widget/android/nsWindow.cpp @@ +1120,4 @@ > if (LockedWindowPtr window{mWindow}) { > + id = window->GetRootLayerId(); > + } else { > + return; Hm I guess you don't need this if you're checking for 'id == 0' below.

(In reply to Jim Chen [:jchen] [:darchons] from comment #11) > Comment on attachment 8824556 [details] [diff] [review] > 0002-Bug-1328752-part-2-Update-Android-nsWindow-to-use- > UiCompositorControllerChild-inplace-of-CompositorBridgeParent-17010613- > 4e64b45.patch > > Review of attachment 8824556 [details] [diff] [review]: > ----------------------------------------------------------------- > > ::: widget/android/nsWindow.cpp > @@ +1120,4 @@ > > if (LockedWindowPtr window{mWindow}) { > > + id = window->GetRootLayerId(); > > + } else { > > + return; > > Hm I guess you don't need this if you're checking for 'id == 0' below. Yeah, I thought of that after I had uploaded the patch and ask for review. I'll go ahead and remove it.

Randall, you created bug 1329319 because you had issues with dispatching a call to a non-ref-counted-base-class method. It's now been fixed, so you can use that (if still relevant) instead of the workaround you described in bug 1329319 comment 4.

Comment on attachment 8824240 [details] [diff] [review] 0001-Bug-1328752-part-1-Add-UiCompositorController-17010514-bb784c3.patch Review of attachment 8824240 [details] [diff] [review]: ----------------------------------------------------------------- This looks good, I have a few comments below. You probably also want to handle GPU process restarts. GPUProcessManager won't create a UiCompositorController if one already exists, but I *think* nothing guarantees that sChild is null when we go to recreate the rendering stack. (I just realized we don't do this for ImageBridge or VRManager either, which is a bug...) Probably the best thing to do is remove the |sChild = nullptr| assignment in DeallocPUiCompositorControllerChild, then expose a new function called Shutdown. Have that post a Close message to the Ui thread (like the Parent does), and unassign sChild there. Then call Shutdown from GPUProcessManager::DestroyProcess [1]. If Android cares about shutdown leaks, you might also need to call Shutdown from gfxPlatform [2]. [1] http://searchfox.org/mozilla-central/rev/225ab0637ed51b8b3f9f4ee2f9c339a37a65b626/gfx/ipc/GPUProcessManager.cpp#470 [2] http://searchfox.org/mozilla-central/rev/8144fcc62054e278fe5db9666815cc13188c96fa/gfx/thebes/gfxPlatform.cpp#958 ::: gfx/layers/ipc/UiCompositorControllerChild.cpp @@ +28,5 @@ > +{ > +} > + > +/* static */ UiCompositorControllerChild* > +UiCompositorControllerChild::Get() It looks like this is called on the main thread in GPUProcessManager, which is racy. Since it just needs to know whether or not a UiCompositorControllerChild already exists, it's better to just expose a new boolean function. If that's the only non-Ui-thread use of this function, please add a thread assert in ::Get() as well. Otherwise we'll have to do more to make this threadsafe :) @@ +126,5 @@ > +{ > + if (mProcessToken) { > + GPUProcessManager::Get()->NotifyRemoteActorDestroyed(mProcessToken); > + mProcessToken = 0; > + } You probably want to unconditionally assign |sParent = nullptr| here so it doesn't leak? (Though it's probably only a shutdown leak) @@ +132,5 @@ > + > +void > +UiCompositorControllerChild::DeallocPUiCompositorControllerChild() > +{ > + sChild = nullptr; It makes me a little nervous to have the only ref be held by a global variable. For example say the GPU process restarts, we could reassign |sChild| before IPDL recognizes the actor is gone, and it might be a use-after-free. Instead, I'd do what you did for UiCompositorControllerParent: When you assign sChild/mLoop, also call AddRef(). In DeallocPUiCompositorControllerChild, also call Release(). That way IPDL always owns a reference. ::: gfx/layers/ipc/UiCompositorControllerParent.h @@ +38,5 @@ > + void Open(Endpoint<PUiCompositorControllerParent>&& aEndpoint); > + void ShutdownImpl(); > + > +private: > + bool mOpen; You can remove mOpen, and the code that uses it. It's safe to unconditionally call Close() in IPDL now. ::: gfx/layers/moz.build @@ +410,5 @@ > 'ipc/PCompositorBridge.ipdl', > 'ipc/PImageBridge.ipdl', > 'ipc/PLayerTransaction.ipdl', > 'ipc/PTexture.ipdl', > + 'ipc/PUiCompositorController.ipdl', Is it possible to put these moz.build inclusions behind MOZ_WIDGET_ANDROID?

(In reply to Gerald Squelart [:gerald] from comment #13) > Randall, you created bug 1329319 because you had issues with dispatching a > call to a non-ref-counted-base-class method. > It's now been fixed, so you can use that (if still relevant) instead of the > workaround you described in bug 1329319 comment 4. Thanks, I removed the workaround wrapper from my patch and am able to dispatch the Send function directly.

(In reply to David Anderson [:dvander] from comment #14) > Comment on attachment 8824240 [details] [diff] [review] > 0001-Bug-1328752-part-1-Add-UiCompositorController-17010514-bb784c3.patch > > Review of attachment 8824240 [details] [diff] [review]: > ----------------------------------------------------------------- > > This looks good, I have a few comments below. You probably also want to > handle GPU process restarts. GPUProcessManager won't create a > UiCompositorController if one already exists, but I *think* nothing > guarantees that sChild is null when we go to recreate the rendering stack. > (I just realized we don't do this for ImageBridge or VRManager either, which > is a bug...) > > Probably the best thing to do is remove the |sChild = nullptr| assignment in > DeallocPUiCompositorControllerChild, then expose a new function called > Shutdown. Have that post a Close message to the Ui thread (like the Parent > does), and unassign sChild there. Then call Shutdown from > GPUProcessManager::DestroyProcess [1]. If Android cares about shutdown > leaks, you might also need to call Shutdown from gfxPlatform [2]. > > [1] > http://searchfox.org/mozilla-central/rev/ > 225ab0637ed51b8b3f9f4ee2f9c339a37a65b626/gfx/ipc/GPUProcessManager.cpp#470 > [2] > http://searchfox.org/mozilla-central/rev/ > 8144fcc62054e278fe5db9666815cc13188c96fa/gfx/thebes/gfxPlatform.cpp#958 > I did my best to implement this but Bug 1325504 makes this impossible to test since XPCOM is not currently being shutdown on exit. It does make me wonder how anyone is testing for memory leaks on exit. Maybe it works on Windows? I didn't have a Windows machine to test on. > ::: gfx/layers/ipc/UiCompositorControllerChild.cpp > @@ +28,5 @@ > > +{ > > +} > > + > > +/* static */ UiCompositorControllerChild* > > +UiCompositorControllerChild::Get() > > It looks like this is called on the main thread in GPUProcessManager, which > is racy. Since it just needs to know whether or not a > UiCompositorControllerChild already exists, it's better to just expose a new > boolean function. If that's the only non-Ui-thread use of this function, > please add a thread assert in ::Get() as well. Otherwise we'll have to do > more to make this threadsafe :) > I implemented the boolean IsInitialized() function, how ever ::Get() is called from main process in nsWindow.cpp. I think this is okay how ever because it does gracefully handle a nullptr and doesn't actually invoke anything on the main thread but instead dispatches the call to the UI thread[1]. Do you still think there is a race condition? Since everyplace that calls ::Get checks for the nullptr I think it is okay? The previous code that called CompositorBridgeParent also had to handle nullptr. [1] https://bugzilla.mozilla.org/attachment.cgi?id=8824556&action=diff#a/widget/android/nsWindow.cpp_sec5 > ::: gfx/layers/moz.build > @@ +410,5 @@ > > 'ipc/PCompositorBridge.ipdl', > > 'ipc/PImageBridge.ipdl', > > 'ipc/PLayerTransaction.ipdl', > > 'ipc/PTexture.ipdl', > > + 'ipc/PUiCompositorController.ipdl', > > Is it possible to put these moz.build inclusions behind MOZ_WIDGET_ANDROID? I can. I was trying to keep the #ifdef'ing to a minimum but I guess I can ensure that none of the UiCompositorController code is compiled except for Android.

(In reply to David Anderson [:dvander] from comment #14) > Comment on attachment 8824240 [details] [diff] [review] > 0001-Bug-1328752-part-1-Add-UiCompositorController-17010514-bb784c3.patch > > Review of attachment 8824240 [details] [diff] [review]: > ----------------------------------------------------------------- > ::: gfx/layers/moz.build > @@ +410,5 @@ > > 'ipc/PCompositorBridge.ipdl', > > 'ipc/PImageBridge.ipdl', > > 'ipc/PLayerTransaction.ipdl', > > 'ipc/PTexture.ipdl', > > + 'ipc/PUiCompositorController.ipdl', > > Is it possible to put these moz.build inclusions behind MOZ_WIDGET_ANDROID? I tried but I don't think it is possible since gfx/ipdl/PGPU.ipdl has the InitUiCompositorController and I don't see a way to #ifdef it out. Maybe I'm missing something? I couldn't figure out how to only build it for Android.

Comment on attachment 8826685 [details] [diff] [review] 0001-Bug-1328752-part-1-Add-UiCompositorController-r-dvander-17011310-fc59c9c.patch Review of attachment 8826685 [details] [diff] [review]: ----------------------------------------------------------------- Looks great! Very sorry for the late review, r=me with nits fixed. ::: gfx/layers/ipc/UiCompositorControllerChild.cpp @@ +45,5 @@ > +{ > + RefPtr<UiCompositorControllerChild> child = sChild; > + if (child) { > + child->Close(); > + } I think you have to set sInitialized to false here. @@ +88,5 @@ > + MOZ_ASSERT(sParent); > + MOZ_ASSERT(!sChild); > + MOZ_ASSERT(IsOnUiThread()); > + > + if(!Open(sParent->GetIPCChannel(), ^-- nit, space

Pushed by rbarker@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/7ceb7d18b5e0 part 1, Add UiCompositorController r=dvander https://hg.mozilla.org/integration/mozilla-inbound/rev/8aa58e657751 part 2, Update Android nsWindow to use UiCompositorControllerChild in place of CompositorBridgeParent r=jchen

https://hg.mozilla.org/mozilla-central/rev/7ceb7d18b5e0 https://hg.mozilla.org/mozilla-central/rev/8aa58e657751