517963 - New-tab opening should not synclaunch the content process

Status: RESOLVED FIXED

(Core :: IPC, defect)

Description

[Benjamin Smedberg]

Currently opening an IPC-bound tab synchronously waits for the content process to launch. This is unacceptable and needs to be fixed to be asynchronous.

Comment 1

[Josh Matthews [:jdm]]

Attachment: First pass at making content process launch async

As far as I can tell this patch makes the content process launch asynchronously.  The test shell works fine, and the main browser now crashes and burns with warnings about no docshells for remote frames.

Comment 2

[Josh Matthews [:jdm]]

The previous comment is misleading; the main browser was already crashing and burning without this patch.  That behaviour has not changed.

Comment 3

[Benjamin Smedberg]

Comment on attachment 409087 [details] [diff] [review]
First pass at making content process launch async

>diff --git a/dom/ipc/ContentProcessParent.cpp b/dom/ipc/ContentProcessParent.cpp

>@@ -141,7 +140,7 @@
>     mSubprocess = nsnull;
>     mon.Notify();
> }
>-
>+    

nit, extraneous whitespace.

>diff --git a/ipc/glue/GeckoChildProcessHost.cpp b/ipc/glue/GeckoChildProcessHost.cpp

>+    MessageLoop* ioLoop = 
>+        BrowserProcessSubThread::GetMessageLoop(BrowserProcessSubThread::IO);
>+    ioLoop->PostTask(FROM_HERE,
>+                    NewRunnableMethod(this,
>+                                      &GeckoChildProcessHost::InitializeChannel));

nit, indent doesn't line up

>@@ -71,16 +78,13 @@
> {
>   MessageLoop* loop = MessageLoop::current();
>   MessageLoop* ioLoop = 
>-    BrowserProcessSubThread::GetMessageLoop(BrowserProcessSubThread::IO);
>+      BrowserProcessSubThread::GetMessageLoop(BrowserProcessSubThread::IO);
>   NS_ASSERTION(loop != ioLoop, "sync launch from the IO thread NYI");
>-
>+    

nit, whitespace and this reindent is incorrect

Please attach a patch with nits fixed and request review from cjones: I don't know the chromium+glue code enough to review this for content.

Comment 4

[Josh Matthews [:jdm]]

Attachment: Same patch with nits addressed

Nits addressed.

Comment 5

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

Comment on attachment 409962 [details] [diff] [review]
Same patch with nits addressed

diff --git a/ipc/glue/GeckoChildProcessHost.cpp b/ipc/glue/GeckoChildProcessHost.cpp
--- a/ipc/glue/GeckoChildProcessHost.cpp
+++ b/ipc/glue/GeckoChildProcessHost.cpp
@@ -1,3 +1,4 @@
+/* -*- Mode: C++; tab-width: 8; indent-tabs-mode: nil; c-basic-offset: 2 -*-
 /* ***** BEGIN LICENSE BLOCK *****
  * Version: MPL 1.1/GPL 2.0/LGPL 2.1
  *
@@ -61,3 +62,4 @@
     mProcessType(aProcessType),
     mMonitor("mozilla.ipc.GeckChildProcessHost.mMonitor"),
     mLaunched(false),
+    mInitialized(false),

Nit: I'd prefer this be called |mChannelInitialized|.  |mInitialized| sounds like "subprocess initialized", which it isn't.

@@ -92,3 +99,17 @@
 bool
 GeckoChildProcessHost::AsyncLaunch(std::vector<std::wstring> aExtraOpts)
 {
+  MessageLoop* ioLoop = 
+    BrowserProcessSubThread::GetMessageLoop(BrowserProcessSubThread::IO);
+  ioLoop->PostTask(FROM_HERE,
+                   NewRunnableMethod(this,
+                                     &GeckoChildProcessHost::PerformAsyncLaunch,
+                                     aExtraOpts));
+
+  // This may look like the sync launch wait, but we only delay as
+  // long as it takes to create the channel.
+  MonitorAutoEnter mon(mMonitor);
+  while (!mInitialized) {
+    mon.Wait();
+  }
+

Clever, nicely done.

I think it might be worthwhile to add a comment here (or in PerformAsyncLaunch()) noting that this code relies on InitializeChannel() and PerformAsyncLaunch() being processed in that order, by the IO thread.  It would help future maintainers of the code get up to speed faster.

Comment 6

[Josh Matthews [:jdm]]

Attachment: Same patch + rename and comment

Nits addressed, comment added.  I'm assuming the review carries over?

Comment 7

[Josh Matthews [:jdm]]

Attachment: Same patch + missing change

Just kidding, this is the actual changed patch.  Silly hg qrefresh.

Comment 8

[Josh Matthews [:jdm]]

Attachment: Same patch unbitrotted

And it's already bitrotted.  That was fast.

Comment 9

[Josh Matthews [:jdm]]

Can somebody with commit access push this?

Comment 10

[Josh Matthews [:jdm]]

Attachment: Same patch rebased

Unbitrotted once again.

Comment 11

[Josh Matthews [:jdm]]

http://hg.mozilla.org/projects/electrolysis/file/37f41c054914

Comment 12

[Josh Matthews [:jdm]]

Whoops, I meant pushed as http://hg.mozilla.org/projects/electrolysis/rev/37f41c054914