516515 - Electrolysis+plugins: don't bother with XPCOM for plugin children

Status: RESOLVED FIXED

(Core Graveyard :: Plug-ins, defect)

Description

[Benjamin Smedberg]

There's really no point in doing any XPCOM stuff at all for plugin processes: we should skip XRE_main and XPCOM and just use custom eventloop code.

Comment 1

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Wouldn't this mean tossing aside all our scary platform-specific event loop (toolkit?) code?  Or are you suggesting calling directly into toolkit, skipping XPCOM?

Comment 2

[Benjamin Smedberg]

you mean the widget code? Yes, this means not using our widget event loops. I really think that's a fair tradeoff, especially because I thought that we got most of that for free with chromium, and almost all of the hard parts don't apply to just plugin windows.

Comment 3

[Benjamin Smedberg]

I have WIP.

Comment 4

[Benjamin Smedberg]

Attachment: WIP, rev. 0

WIP, builds and works on Windows except for leaking issues

Comment 5

[Benjamin Smedberg]

Attachment: Plugin processes without XPCOM, rev. 1

The leaks were a red herring caused by Win7 stuff, apparently. This should be ready for review.

Comment 6

[Benjamin Smedberg]

Attachment: Plugin processes without XPCOM, rev. 1.1

The right patch.

Comment 7

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Comment on attachment 424853 [details] [diff] [review]
Plugin processes without XPCOM, rev. 1.1

> diff --git a/dom/plugins/PluginThreadChild.cpp b/dom/plugins/PluginThreadChild.cpp
> --- a/dom/plugins/PluginThreadChild.cpp
> +++ b/dom/plugins/PluginThreadChild.cpp
> @@ -41,20 +41,23 @@
>  
>  #include "prlink.h"
>  
>  #include "base/command_line.h"
>  #include "base/string_util.h"
>  #include "chrome/common/child_process.h"
>  #include "chrome/common/chrome_switches.h"
>  
> -using mozilla::ipc::GeckoThread;
> -
>  namespace mozilla {
>  namespace plugins {
>  
>  PluginThreadChild::PluginThreadChild(ProcessHandle aParentHandle) :
> -    GeckoThread(aParentHandle),
> +    ChildThread(base::Thread::Options(
> +                    MessageLoop::TYPE_UI,
> +                    0, // stack size
> +                    false)), // wait for Init()?
> +    mParentProcessHandle(aParentHandle),
>      mPlugin()
>  {
> +    NS_ASSERTION(!gInstance, "Two PluginThreadChild?");
> +    gInstance = this;
>  }
 
We have three leaf child thread types: plugin, IPDL test, and content.
Plugin threads and IPDL test threads don't need XPCOM, only content
does.  So rather than forcing each leaf type to directly inherit from
ChildThread and set up its own message loop type, I think it'd be
cleaner to (i) deCOMtaminate GeckoThread and have it use TYPE_UI; (ii)
keep the plugin thread and IPDL test threads as |GeckoThread|s; (iii)
put the XPCOM-y stuff that's currently in GeckoThread into an
"XPCOMThread"; (iv) have ContentProcessThread be an XPCOMThread.  This
isn't much additional code.


> diff --git a/ipc/chromium/src/base/message_loop.h b/ipc/chromium/src/base/message_loop.h
> --- a/ipc/chromium/src/base/message_loop.h
> +++ b/ipc/chromium/src/base/message_loop.h
> @@ -416,10 +425,10 @@ public:
>  // MessageLoopForUI extends MessageLoop with methods that are particular to a
>  // MessageLoop instantiated with TYPE_UI.
>  //
>  // This class is typically used like so:
>  //   MessageLoopForUI::current()->...call some method...
>  //
>  class MessageLoopForUI : public MessageLoop {
>   public:
> -  MessageLoopForUI() : MessageLoop(TYPE_UI) {
> +  MessageLoopForUI(Type type=TYPE_UI) : MessageLoop(type) {
>    }

If you're going to make the UI type configurable ...

>    // Returns the MessageLoopForUI of the current thread.
>    static MessageLoopForUI* current() {
>      MessageLoop* loop = MessageLoop::current();
>      DCHECK_EQ(MessageLoop::TYPE_UI, loop->type());
>      return static_cast<MessageLoopForUI*>(loop);
>    }

... then you need to either #ifdef out this function or change it to
save away a |static sUILoopType| or something.


> diff --git a/toolkit/xre/nsEmbedFunctions.cpp b/toolkit/xre/nsEmbedFunctions.cpp
> --- a/toolkit/xre/nsEmbedFunctions.cpp
> +++ b/toolkit/xre/nsEmbedFunctions.cpp
> @@ -495,17 +500,16 @@ struct RunnableMethodTraits<ContentProce
>  {
>      static void RetainCallee(ContentProcessChild* obj) { }
>      static void ReleaseCallee(ContentProcessChild* obj) { }
>  };
>  
>  void
>  XRE_ShutdownChildProcess()
>  {
> -    NS_ABORT_IF_FALSE(NS_IsMainThread(), "Wrong thread!");
> +  NS_ABORT_IF_FALSE(MessageLoopForUI::current(), "Wrong thread!");
>  
> -    MessageLoop* ioLoop = XRE_GetIOMessageLoop();
> -    NS_ABORT_IF_FALSE(!!ioLoop, "Bad shutdown order");
> +  MessageLoop* ioLoop = XRE_GetIOMessageLoop();
> +  NS_ABORT_IF_FALSE(!!ioLoop, "Bad shutdown order");
>  
> -    ioLoop->PostTask(FROM_HERE, new MessageLoop::QuitTask());
> +  ioLoop->PostTask(FROM_HERE, new MessageLoop::QuitTask());
>  }
 
In this patch, MessageLoopForUI::current() doesn't work in content and
IPDL-test subprocesses.  With the suggested refactoring above, it
would work for IPDL-test subprocesses but not content subprocesses.
One fix is to add a

  (MessageLoop::Type uiLoopType=MessageLoop::TYPE_UI)

param and assert that |uiLoopType == MessageLoop::current()|.  Then only
content subprocesses would need to pass non-default.


The rest of it looks good to me.  I'd like to take a look at rev2.

Comment 8

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Hmm ... I guess that "XPCOM" is a subset of "Gecko", so maybe "BaseThread" and "XPCOMThread" are better names.  But I don't care that much.

Comment 9

[Benjamin Smedberg]

Attachment: Plugin processes without XPCOM, rev. 2

Comment 10

[Benjamin Smedberg]

http://hg.mozilla.org/mozilla-central/rev/62cc92f9dced
http://hg.mozilla.org/projects/electrolysis/rev/796821901a03

Comment 11

[Oleg Romashin (:romaxa)]

eh... Qt build again broken.

Comment 12

[Benjamin Smedberg]

Yes, you'll need to figure out what to do about a UIMessageLoop for Qt.

Comment 13

[Benjamin Smedberg]

http://hg.mozilla.org/projects/firefox-lorentz/rev/35cf959c7a66

Comment 14

[Mike Beltzner [:beltzner, not reading bugmail]]

Blanket approval for Lorentz merge to mozilla-1.9.2
a=beltzner for 1.9.2.4 - please make sure to mark status1.9.2:.4-fixed

Comment 15

[Benjamin Smedberg]

Merged into 1.9.2 at http://hg.mozilla.org/releases/mozilla-1.9.2/rev/84ba4d805430