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

RESOLVED FIXED

Status

()

Core
Plug-ins
RESOLVED FIXED
9 years ago
8 years ago

People

(Reporter: Benjamin Smedberg, Assigned: Benjamin Smedberg)

Tracking

unspecified
x86
Windows NT
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(status1.9.2 .4-fixed)

Details

(Whiteboard: [fixed-lorentz])

Attachments

(1 attachment, 3 obsolete attachments)

(Assignee)

Description

9 years ago
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.
(Assignee)

Updated

9 years ago
Blocks: 478976
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?
(Assignee)

Comment 2

9 years ago
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.
(Assignee)

Comment 3

8 years ago
I have WIP.
Assignee: nobody → benjamin
Status: NEW → ASSIGNED
(Assignee)

Comment 4

8 years ago
Created attachment 424642 [details] [diff] [review]
WIP, rev. 0

WIP, builds and works on Windows except for leaking issues
(Assignee)

Comment 5

8 years ago
Created attachment 424778 [details] [diff] [review]
Plugin processes without XPCOM, rev. 1

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)
(Assignee)

Comment 6

8 years ago
Created attachment 424853 [details] [diff] [review]
Plugin processes without XPCOM, rev. 1.1

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.
(Assignee)

Comment 9

8 years ago
Created attachment 425023 [details] [diff] [review]
Plugin processes without XPCOM, rev. 2
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+
(Assignee)

Comment 10

8 years ago
http://hg.mozilla.org/mozilla-central/rev/62cc92f9dced
http://hg.mozilla.org/projects/electrolysis/rev/796821901a03
Status: ASSIGNED → RESOLVED
Last Resolved: 8 years ago
Resolution: --- → FIXED
eh... Qt build again broken.
(Assignee)

Comment 12

8 years ago
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
(Assignee)

Comment 15

8 years ago
Merged into 1.9.2 at http://hg.mozilla.org/releases/mozilla-1.9.2/rev/84ba4d805430
status1.9.2: --- → .4-fixed
You need to log in before you can comment on or make changes to this bug.