Closed Bug 516515 Opened 11 years ago Closed 10 years ago

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

Categories

(Core :: Plug-ins, defect)

x86
Windows NT
defect
Not set

Tracking

()

RESOLVED FIXED
Tracking Status
status1.9.2 --- .4-fixed

People

(Reporter: benjamin, Assigned: benjamin)

References

Details

(Whiteboard: [fixed-lorentz])

Attachments

(1 file, 3 obsolete files)

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.
Blocks: OOPP
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?
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.
I have WIP.
Assignee: nobody → benjamin
Status: NEW → ASSIGNED
Attached patch WIP, rev. 0 (obsolete) — Splinter Review
WIP, builds and works on Windows except for leaking issues
The leaks were a red herring caused by Win7 stuff, apparently. This should be ready for review.
Attachment #424642 - Attachment is obsolete: true
Attachment #424778 - Flags: review?(jones.chris.g)
The right patch.
Attachment #424778 - Attachment is obsolete: true
Attachment #424853 - Flags: review?(jones.chris.g)
Attachment #424778 - Flags: review?(jones.chris.g)
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.
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.
Attachment #424853 - Attachment is obsolete: true
Attachment #425023 - Flags: review?(jones.chris.g)
Attachment #424853 - Flags: review?(jones.chris.g)
Attachment #425023 - Flags: review?(jones.chris.g) → review+
http://hg.mozilla.org/mozilla-central/rev/62cc92f9dced
http://hg.mozilla.org/projects/electrolysis/rev/796821901a03
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
eh... Qt build again broken.
Yes, you'll need to figure out what to do about a UIMessageLoop for Qt.
Depends on: 544410
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
You need to log in before you can comment on or make changes to this bug.