Re-fix http://crbug.com/87176, and add a test.

Re-fix http://crbug.com/87176, and add a test.

This also reverts the workaround in extension_host.cc that I added to fix issue 178542.

BUG=87176, 178542
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=206182

[Jeffrey Yasskin, 2013-06-05 00:21:12]

Here's an initial test for the 87176 fix that simulates the whole sequence of
tab creations and navigations, although it omits many of the unnecessary(?)
renderer->browser IPCs. I can shorten it by removing the simulation of an actual
navigation and a swapped-out RenderViewHost, since those don't contribute to the
calculation in OnShutdownRequest. However, I wanted to show you the whole thing
and get your thoughts, in case I've missed a reason to keep the larger test.

[Charlie Reis, 2013-06-05 19:07:10]

Many thanks for finding a way to test this!  Seems like a good approach to me,
though it'd be good to get jam's eyes on it as well once we're ready to go. 
Just a few nits below.

Also, can you add something to the CL description about adding a framework for
unit-testing RenderProcessHosts?  That explains where most of the complexity
comes from.

https://codereview.chromium.org/16267002/diff/16001/content/browser/renderer_...
File content/browser/renderer_host/render_process_host_impl.cc (right):

https://codereview.chromium.org/16267002/diff/16001/content/browser/renderer_...
content/browser/renderer_host/render_process_host_impl.cc:607:
channel_->AddFilter(new AudioInputRendererHost(audio_manager,
Does this also need to be behind the if?

https://codereview.chromium.org/16267002/diff/16001/content/browser/renderer_...
content/browser/renderer_host/render_process_host_impl.cc:1689: // Don't shut
down if there are more active RenderViews than the one asking
Please update this comment:
s/more active RenderViews than the one asking to close/active RenderViews/

https://codereview.chromium.org/16267002/diff/16001/content/browser/renderer_...
File content/browser/renderer_host/render_process_host_impl_unittest.cc (right):

https://codereview.chromium.org/16267002/diff/16001/content/browser/renderer_...
content/browser/renderer_host/render_process_host_impl_unittest.cc:182:
SiteInstance* site_instance_;
It's not clear why this needs a SiteInstance, since processes aren't created
with a SiteInstance.  Looks like we're only using it to get the
StoragePartition.  Perhaps we should take that in instead?

https://codereview.chromium.org/16267002/diff/16001/content/browser/renderer_...
content/browser/renderer_host/render_process_host_impl_unittest.cc:229:
tab->GetController().LoadURL(kUrl, Referrer(), PAGE_TRANSITION_TYPED, "");
How does example.com commit?  Am I missing a part where we return a hard-coded
response for it?

https://codereview.chromium.org/16267002/diff/16001/content/browser/renderer_...
content/browser/renderer_host/render_process_host_impl_unittest.cc:236:
EXPECT_EQ(NULL,
Any reason not to use EXPECT_FALSE?  That seems to be a common pattern
elsewhere.

https://codereview.chromium.org/16267002/diff/16001/content/browser/renderer_...
content/browser/renderer_host/render_process_host_impl_unittest.cc:243: //
ViewMsg_WasSwappedOut from a previous navigation) doesn't arrive until
Interesting.  I like that we don't have to simulate the previous navigation,
since just sending the ShutdownRequest is sufficient.

https://codereview.chromium.org/16267002/diff/16001/content/browser/renderer_...
content/browser/renderer_host/render_process_host_impl_unittest.cc:247:
->Send(new ChildProcessHostMsg_ShutdownRequest());
nit: -> on previous line

https://codereview.chromium.org/16267002/diff/16001/content/browser/renderer_...
content/browser/renderer_host/render_process_host_impl_unittest.cc:249:
EXPECT_EQ(NULL,
EXPECT_FALSE?

[Jeffrey Yasskin, 2013-06-05 21:23:27]

+jam for content/ and ipc/ review.

This depends on https://codereview.chromium.org/15799009/ and
https://codereview.chromium.org/15793016, which are winding their ways through
the commit queue.

https://codereview.chromium.org/16267002/diff/16001/content/browser/renderer_...
File content/browser/renderer_host/render_process_host_impl.cc (right):

https://codereview.chromium.org/16267002/diff/16001/content/browser/renderer_...
content/browser/renderer_host/render_process_host_impl.cc:607:
channel_->AddFilter(new AudioInputRendererHost(audio_manager,
On 2013/06/05 19:07:10, creis wrote:
> Does this also need to be behind the if?

The test passes with it outside the if, likely because I'm not sending any
messages that the AudioInputRendererHost wants to process using its arguments.
It looks like I could actually remove the 'if' entirely by removing the two
DCHECKs in AudioRendererHost()
(https://code.google.com/p/chromium/codesearch/#chromium/src/content/browser/r...),
which would cause it to fail only when a unittest actually sends a message that
needs to go to this filter.

I think it's slightly better to put the 'if' here, since that reduces the
dependencies of the unittest, and a unittest that's trying to test the
AudioRendererHost should probably just instantiate that and send messages
through it directly. The downside is that a test author who expects to send
messages to the RPH and have the AudioRendererHost process them will find their
bug faster if the first message they send crashes, rather than just being
ignored.

https://codereview.chromium.org/16267002/diff/16001/content/browser/renderer_...
content/browser/renderer_host/render_process_host_impl.cc:1689: // Don't shut
down if there are more active RenderViews than the one asking
On 2013/06/05 19:07:10, creis wrote:
> Please update this comment:
> s/more active RenderViews than the one asking to close/active RenderViews/

Thanks, done.

https://codereview.chromium.org/16267002/diff/16001/content/browser/renderer_...
File content/browser/renderer_host/render_process_host_impl_unittest.cc (right):

https://codereview.chromium.org/16267002/diff/16001/content/browser/renderer_...
content/browser/renderer_host/render_process_host_impl_unittest.cc:182:
SiteInstance* site_instance_;
On 2013/06/05 19:07:10, creis wrote:
> It's not clear why this needs a SiteInstance, since processes aren't created
> with a SiteInstance.  Looks like we're only using it to get the
> StoragePartition.  Perhaps we should take that in instead?

Sure, done.

Another possible refactoring would be to pass the SiteInstance and possibly the
ContentBrowserClient to CreateRenderProcessHost() at
https://code.google.com/p/chromium/codesearch/#chromium/src/content/browser/s...,
so that both branches have access to the same information.

https://codereview.chromium.org/16267002/diff/16001/content/browser/renderer_...
content/browser/renderer_host/render_process_host_impl_unittest.cc:229:
tab->GetController().LoadURL(kUrl, Referrer(), PAGE_TRANSITION_TYPED, "");
On 2013/06/05 19:07:10, creis wrote:
> How does http://example.com commit?  Am I missing a part where we return a
hard-coded
> response for it?

No, AFAIK, there's no hard-coded response for it, and since there's no actual
renderer process, it probably wouldn't request the response anyway. I don't
understand the RPH's flow very well, but I suspect that it's not currently
committing, and that I'd need to fake one or more ViewHostMsg_Something in order
to push the RVH through the expected sequence of events.

Can you describe the extra assertion that'll check if example.com has committed?
After that, I can add enough messages to make it happen.

https://codereview.chromium.org/16267002/diff/16001/content/browser/renderer_...
content/browser/renderer_host/render_process_host_impl_unittest.cc:236:
EXPECT_EQ(NULL,
On 2013/06/05 19:07:10, creis wrote:
> Any reason not to use EXPECT_FALSE?  That seems to be a common pattern
> elsewhere.

No particular reason. Done.

https://codereview.chromium.org/16267002/diff/16001/content/browser/renderer_...
content/browser/renderer_host/render_process_host_impl_unittest.cc:247:
->Send(new ChildProcessHostMsg_ShutdownRequest());
On 2013/06/05 19:07:10, creis wrote:
> nit: -> on previous line

Done. Since clang-format puts the -> at the beginning of the line, I've filed
http://b/9299359 too.

https://codereview.chromium.org/16267002/diff/16001/content/browser/renderer_...
content/browser/renderer_host/render_process_host_impl_unittest.cc:249:
EXPECT_EQ(NULL,
On 2013/06/05 19:07:10, creis wrote:
> EXPECT_FALSE?

Done.

[Jeffrey Yasskin, 2013-06-05 21:46:48]

+mpcomplete too for revert of r190543 in extension_host.cc

[Charlie Reis, 2013-06-05 21:54:09]

https://codereview.chromium.org/16267002/diff/16001/content/browser/renderer_...
File content/browser/renderer_host/render_process_host_impl_unittest.cc (right):

https://codereview.chromium.org/16267002/diff/16001/content/browser/renderer_...
content/browser/renderer_host/render_process_host_impl_unittest.cc:182:
SiteInstance* site_instance_;
On 2013/06/05 21:23:27, Jeffrey Yasskin wrote:
> On 2013/06/05 19:07:10, creis wrote:
> > It's not clear why this needs a SiteInstance, since processes aren't created
> > with a SiteInstance.  Looks like we're only using it to get the
> > StoragePartition.  Perhaps we should take that in instead?
> 
> Sure, done.
> 
> Another possible refactoring would be to pass the SiteInstance and possibly
the
> ContentBrowserClient to CreateRenderProcessHost() at
>
https://code.google.com/p/chromium/codesearch/#chromium/src/content/browser/s...,
> so that both branches have access to the same information.

Good point!  That seems like a better solution, so we don't have to store either
here.

https://codereview.chromium.org/16267002/diff/16001/content/browser/renderer_...
content/browser/renderer_host/render_process_host_impl_unittest.cc:229:
tab->GetController().LoadURL(kUrl, Referrer(), PAGE_TRANSITION_TYPED, "");
On 2013/06/05 21:23:27, Jeffrey Yasskin wrote:
> On 2013/06/05 19:07:10, creis wrote:
> > How does http://example.com commit?  Am I missing a part where we return a
> hard-coded
> > response for it?
> 
> No, AFAIK, there's no hard-coded response for it, and since there's no actual
> renderer process, it probably wouldn't request the response anyway. I don't
> understand the RPH's flow very well, but I suspect that it's not currently
> committing, and that I'd need to fake one or more ViewHostMsg_Something in
order
> to push the RVH through the expected sequence of events.
> 
> Can you describe the extra assertion that'll check if http://example.com has
committed?
> After that, I can add enough messages to make it happen.

Well, we usually use a TestWebContents and call TestDidNavigate to simulate
commit.  There's examples of that in web_contents_impl_unittest.cc (e.g.,
SimpleNavigation).  I'm not sure if that's compatible with what you're doing
here, though.

Perhaps it doesn't matter if it commits for the purpose of this test, though. 
The new tab will be considered an active view, and that's all we need.  That
means your comment/check below about "the loading tab isn't pending" is a bit of
a red herring.  We just care that the ShutdownRequest message is sent to the
tab's current process, so you could check that
tab->GetSiteInstance()->GetProcess() is equal to child_processes_[0].

https://codereview.chromium.org/16267002/diff/16001/content/browser/renderer_...
content/browser/renderer_host/render_process_host_impl_unittest.cc:247:
->Send(new ChildProcessHostMsg_ShutdownRequest());
On 2013/06/05 21:23:27, Jeffrey Yasskin wrote:
> On 2013/06/05 19:07:10, creis wrote:
> > nit: -> on previous line
> 
> Done. Since clang-format puts the -> at the beginning of the line, I've filed
> http://b/9299359 too.

Thanks.  Not sure if there's an official rule on this, but there's certainly
lots of code in content/ that puts it at the end, so that will be good for
consistency.

[Jeffrey Yasskin, 2013-06-05 22:53:20]

https://codereview.chromium.org/16267002/diff/16001/content/browser/renderer_...
File content/browser/renderer_host/render_process_host_impl_unittest.cc (right):

https://codereview.chromium.org/16267002/diff/16001/content/browser/renderer_...
content/browser/renderer_host/render_process_host_impl_unittest.cc:182:
SiteInstance* site_instance_;
On 2013/06/05 21:54:09, creis wrote:
> On 2013/06/05 21:23:27, Jeffrey Yasskin wrote:
> > On 2013/06/05 19:07:10, creis wrote:
> > > It's not clear why this needs a SiteInstance, since processes aren't
created
> > > with a SiteInstance.  Looks like we're only using it to get the
> > > StoragePartition.  Perhaps we should take that in instead?
> > 
> > Sure, done.
> > 
> > Another possible refactoring would be to pass the SiteInstance and possibly
> the
> > ContentBrowserClient to CreateRenderProcessHost() at
> >
>
https://code.google.com/p/chromium/codesearch/#chromium/src/content/browser/s...,
> > so that both branches have access to the same information.
> 
> Good point!  That seems like a better solution, so we don't have to store
either
> here.

I've put that in https://codereview.chromium.org/16490003/. Assuming that one
gets approved soon, I'll rebase this on top of it; otherwise, I'll add this file
to that CL.

https://codereview.chromium.org/16267002/diff/16001/content/browser/renderer_...
content/browser/renderer_host/render_process_host_impl_unittest.cc:229:
tab->GetController().LoadURL(kUrl, Referrer(), PAGE_TRANSITION_TYPED, "");
On 2013/06/05 21:54:09, creis wrote:
> On 2013/06/05 21:23:27, Jeffrey Yasskin wrote:
> > On 2013/06/05 19:07:10, creis wrote:
> > > How does http://example.com commit?  Am I missing a part where we return a
> > hard-coded
> > > response for it?
> > 
> > No, AFAIK, there's no hard-coded response for it, and since there's no
actual
> > renderer process, it probably wouldn't request the response anyway. I don't
> > understand the RPH's flow very well, but I suspect that it's not currently
> > committing, and that I'd need to fake one or more ViewHostMsg_Something in
> order
> > to push the RVH through the expected sequence of events.
> > 
> > Can you describe the extra assertion that'll check if http://example.com has
> committed?
> > After that, I can add enough messages to make it happen.
> 
> Well, we usually use a TestWebContents and call TestDidNavigate to simulate
> commit.  There's examples of that in web_contents_impl_unittest.cc (e.g.,
> SimpleNavigation).  I'm not sure if that's compatible with what you're doing
> here, though.

I started this test with a TestWebContents, but TestWebContents casts its
RenderViewHost to a TestRenderViewHost in CommitPendingNavigation(), and I can't
use TestRenderViewHost because it almost entirely avoids using the
RenderProcessHost that I'm trying to test.

> Perhaps it doesn't matter if it commits for the purpose of this test, though. 
> The new tab will be considered an active view, and that's all we need.  That
> means your comment/check below about "the loading tab isn't pending" is a bit
of
> a red herring.

That double-check was intended to make sure that pending_views_==0, but of
course the pending_render_view_host() isn't directly connected to that field
either. Oh well; I've removed it.

> We just care that the ShutdownRequest message is sent to the
> tab's current process, so you could check that
> tab->GetSiteInstance()->GetProcess() is equal to child_processes_[0].

To some extent, knowing that child_processes_.size()==1 already tells us that,
since every RenderProcessHostImpl creates a ChildProcess, so there aren't any
other RenderProcessHostImpl instances to send anything to. But this double-check
doesn't hurt anything; done.

https://codereview.chromium.org/16267002/diff/39001/content/browser/renderer_...
File content/browser/renderer_host/render_process_host_impl_unittest.cc (right):

https://codereview.chromium.org/16267002/diff/39001/content/browser/renderer_...
content/browser/renderer_host/render_process_host_impl_unittest.cc:201:
base::MessageLoopForIO message_loop_;
I'll be able to replace this list of threads with a TestBrowserThreadBundle once
https://codereview.chromium.org/14197014/ gets in.

[Charlie Reis, 2013-06-05 23:08:43]

LGTM, once you incorporate https://codereview.chromium.org/16490003/.

[jam, 2013-06-06 16:52:46]

question: can we avoid the changes to child_process_launcher by making RPHImpl
think it's in single process mode? and perhaps if necessary, another ForTesting
method to RPHImpl?

[Jeffrey Yasskin, 2013-06-06 17:09:57]

On 2013/06/06 16:52:46, jam wrote:
> question: can we avoid the changes to child_process_launcher by making RPHImpl
> think it's in single process mode? and perhaps if necessary, another
ForTesting
> method to RPHImpl?

I don't think so. That would require implementing single-process mode in
SPLIT_DLL mode
(https://code.google.com/p/chromium/codesearch/#chromium/src/content/browser/r...),
and it wouldn't work for general unittests here since single-process mode
implies a renderer thread, which wouldn't allow control over the timing.

On the other hand, it's possible we could replace the single-process flag with
another implementation of ChildProcessLauncher, in a separate CL.

[jam, 2013-06-06 17:17:02]

On 2013/06/06 17:09:57, Jeffrey Yasskin wrote:
> On 2013/06/06 16:52:46, jam wrote:
> > question: can we avoid the changes to child_process_launcher by making
RPHImpl
> > think it's in single process mode? and perhaps if necessary, another
> ForTesting
> > method to RPHImpl?
> 
> I don't think so. That would require implementing single-process mode in
> SPLIT_DLL mode
>
(https://code.google.com/p/chromium/codesearch/#chromium/src/content/browser/r...),


I think CHROME_SPLIT_DLL is something that's orthogonal. there are already
(browser) tests that depend on single process anyways.

also, i think that CHROME_SPLIT_DLL ifdef was put in before the current approach
of exporting missing symbols, so it will work in tests afaik.

> and it wouldn't work for general unittests here since single-process mode
> implies a renderer thread, which wouldn't allow control over the timing.

what control are you referring to? note that there's
RPHImpl::GetInProcessRendererThreadForTesting which gives access to the loop.

the other question i'd have is: if the test was an implemented as a browser
test, would there be less changes needed to production code?

> 
> On the other hand, it's possible we could replace the single-process flag with
> another implementation of ChildProcessLauncher, in a separate CL.

[Jeffrey Yasskin, 2013-06-06 20:28:25]

On 2013/06/06 17:17:02, jam wrote:
> On 2013/06/06 17:09:57, Jeffrey Yasskin wrote:
> > On 2013/06/06 16:52:46, jam wrote:
> > > question: can we avoid the changes to child_process_launcher by making
> RPHImpl
> > > think it's in single process mode? and perhaps if necessary, another
> > ForTesting
> > > method to RPHImpl?
> > 
> > I don't think so. That would require implementing single-process mode in
> > SPLIT_DLL mode
> >
>
(https://code.google.com/p/chromium/codesearch/#chromium/src/content/browser/r...),
> 
> 
> I think CHROME_SPLIT_DLL is something that's orthogonal. there are already
> (browser) tests that depend on single process anyways.
> 
> also, i think that CHROME_SPLIT_DLL ifdef was put in before the current
approach
> of exporting missing symbols, so it will work in tests afaik.

CHROME_SPLIT_DLL was added on May 8 and removes the in-process renderer branch
entirely. Are you saying that we can just delete that #if, and everything will
keep working? 

> > and it wouldn't work for general unittests here since single-process mode
> > implies a renderer thread, which wouldn't allow control over the timing.
> 
> what control are you referring to? note that there's
> RPHImpl::GetInProcessRendererThreadForTesting which gives access to the loop.

Access to the renderer thread's MessageLoop isn't enough to cause a renderer to
pause between receiving an IPC and replying to it.

In this particular case, that amount of control turned out to be unnecessary,
since I could provoke the bug by just sending an extra message to the
RenderProcessHost. (That's also what makes the browsertest below even possible.)
However, it's easy to imagine behaviors that happen if an IPC in the middle of a
back-and-forth chain is delayed, and the framework in this CL will let you test
them.

> the other question i'd have is: if the test was an implemented as a browser
> test, would there be less changes needed to production code?

https://codereview.chromium.org/16593002 implements this as a browser test. It
takes ~900ms to run, as opposed to 70ms for the unit test, and assuming I'm no
better at avoiding flaky tests than you are, has about an 80% chance of being
flaky or fragile
(http://test-results.appspot.com/dashboards/overview.html#group=%40DEPS%20-%20...).
In order to avoid slowing down other Chrome developers, that basically excludes
the browsertest option when a unittest is possible.

[Matt Perry, 2013-06-06 21:01:27]

lgtm

https://codereview.chromium.org/16267002/diff/50001/content/browser/child_pro...
File content/browser/child_process_launcher.h (right):

https://codereview.chromium.org/16267002/diff/50001/content/browser/child_pro...
content/browser/child_process_launcher.h:68: scoped_ptr<ChildProcessLauncher>
NewChildProcessLauncher(
nit: I generally prefer factory methods like this to be static methods on the
class

[Jeffrey Yasskin, 2013-06-06 21:06:56]

https://codereview.chromium.org/16267002/diff/50001/content/browser/child_pro...
File content/browser/child_process_launcher.h (right):

https://codereview.chromium.org/16267002/diff/50001/content/browser/child_pro...
content/browser/child_process_launcher.h:68: scoped_ptr<ChildProcessLauncher>
NewChildProcessLauncher(
On 2013/06/06 21:01:27, Matt Perry wrote:
> nit: I generally prefer factory methods like this to be static methods on the
> class

I have no problem with doing this, but I'd like jam or creis to endorse the
renaming first (and maybe pick a spelling), since it's in their API.

[jam, 2013-06-07 00:37:27]

On 2013/06/06 20:28:25, Jeffrey Yasskin wrote:
> On 2013/06/06 17:17:02, jam wrote:
> > On 2013/06/06 17:09:57, Jeffrey Yasskin wrote:
> > > On 2013/06/06 16:52:46, jam wrote:
> > > > question: can we avoid the changes to child_process_launcher by making
> > RPHImpl
> > > > think it's in single process mode? and perhaps if necessary, another
> > > ForTesting
> > > > method to RPHImpl?
> > > 
> > > I don't think so. That would require implementing single-process mode in
> > > SPLIT_DLL mode
> > >
> >
>
(https://code.google.com/p/chromium/codesearch/#chromium/src/content/browser/r...),
> > 
> > 
> > I think CHROME_SPLIT_DLL is something that's orthogonal. there are already
> > (browser) tests that depend on single process anyways.
> > 
> > also, i think that CHROME_SPLIT_DLL ifdef was put in before the current
> approach
> > of exporting missing symbols, so it will work in tests afaik.
> 
> CHROME_SPLIT_DLL was added on May 8 and removes the in-process renderer branch
> entirely. Are you saying that we can just delete that #if, and everything will
> keep working? 

right, that should work.

however i don't think you should be worrying about that mode in this branch. if
we switch to that mode, when that happens we will make sure single process mode
works.

> 
> > > and it wouldn't work for general unittests here since single-process mode
> > > implies a renderer thread, which wouldn't allow control over the timing.
> > 
> > what control are you referring to? note that there's
> > RPHImpl::GetInProcessRendererThreadForTesting which gives access to the
loop.
> 
> Access to the renderer thread's MessageLoop isn't enough to cause a renderer
to
> pause between receiving an IPC and replying to it.
> 
> In this particular case, that amount of control turned out to be unnecessary,
> since I could provoke the bug by just sending an extra message to the
> RenderProcessHost. (That's also what makes the browsertest below even
possible.)
> However, it's easy to imagine behaviors that happen if an IPC in the middle of
a
> back-and-forth chain is delayed, and the framework in this CL will let you
test
> them.
> 
> > the other question i'd have is: if the test was an implemented as a browser
> > test, would there be less changes needed to production code?
> 
> https://codereview.chromium.org/16593002 implements this as a browser test. It
> takes ~900ms to run, as opposed to 70ms for the unit test, and assuming I'm no
> better at avoiding flaky tests than you are, has about an 80% chance of being
> flaky or fragile
>
(http://test-results.appspot.com/dashboards/overview.html#group=%2540DEPS%2520...).
> In order to avoid slowing down other Chrome developers, that basically
excludes
> the browsertest option when a unittest is possible.

I'd much prefer the browser test over the unit test. I really prefer to not make
changes to the code for tests if we can avoid it.

Regarding the test run time, things like swarm which will be turned on for
browser tests in the near future mean that adding extra tests doesn't slow down
the time.

[Jeffrey Yasskin, 2013-06-07 19:39:16]

On 2013/06/07 00:37:27, jam wrote:
> On 2013/06/06 20:28:25, Jeffrey Yasskin wrote:
> > On 2013/06/06 17:17:02, jam wrote:
> > > On 2013/06/06 17:09:57, Jeffrey Yasskin wrote:
> > > > On 2013/06/06 16:52:46, jam wrote:
> > > > > question: can we avoid the changes to child_process_launcher by making
> > > RPHImpl
> > > > > think it's in single process mode? and perhaps if necessary, another
> > > > ForTesting
> > > > > method to RPHImpl?
> > > > 
> > > > I don't think so. That would require implementing single-process mode in
> > > > SPLIT_DLL mode
> > > >
> > >
> >
>
(https://code.google.com/p/chromium/codesearch/#chromium/src/content/browser/r...),
> > > 
> > > 
> > > I think CHROME_SPLIT_DLL is something that's orthogonal. there are already
> > > (browser) tests that depend on single process anyways.
> > > 
> > > also, i think that CHROME_SPLIT_DLL ifdef was put in before the current
> > approach
> > > of exporting missing symbols, so it will work in tests afaik.
> > 
> > CHROME_SPLIT_DLL was added on May 8 and removes the in-process renderer
branch
> > entirely. Are you saying that we can just delete that #if, and everything
will
> > keep working? 
> 
> right, that should work.
> 
> however i don't think you should be worrying about that mode in this branch.
if
> we switch to that mode, when that happens we will make sure single process
mode
> works.
> 
> > 
> > > > and it wouldn't work for general unittests here since single-process
mode
> > > > implies a renderer thread, which wouldn't allow control over the timing.
> > > 
> > > what control are you referring to? note that there's
> > > RPHImpl::GetInProcessRendererThreadForTesting which gives access to the
> loop.
> > 
> > Access to the renderer thread's MessageLoop isn't enough to cause a renderer
> to
> > pause between receiving an IPC and replying to it.
> > 
> > In this particular case, that amount of control turned out to be
unnecessary,
> > since I could provoke the bug by just sending an extra message to the
> > RenderProcessHost. (That's also what makes the browsertest below even
> possible.)
> > However, it's easy to imagine behaviors that happen if an IPC in the middle
of
> a
> > back-and-forth chain is delayed, and the framework in this CL will let you
> test
> > them.
> > 
> > > the other question i'd have is: if the test was an implemented as a
browser
> > > test, would there be less changes needed to production code?
> > 
> > https://codereview.chromium.org/16593002 implements this as a browser test.
It
> > takes ~900ms to run, as opposed to 70ms for the unit test, and assuming I'm
no
> > better at avoiding flaky tests than you are, has about an 80% chance of
being
> > flaky or fragile
> >
>
(http://test-results.appspot.com/dashboards/overview.html#group=%252540DEPS%25...).
> > In order to avoid slowing down other Chrome developers, that basically
> excludes
> > the browsertest option when a unittest is possible.
> 
> I'd much prefer the browser test over the unit test. I really prefer to not
make
> changes to the code for tests if we can avoid it.
> 
> Regarding the test run time, things like swarm which will be turned on for
> browser tests in the near future mean that adding extra tests doesn't slow
down
> the time.

chromium-dev appears to disagree with you about both the desirability of
changing production code to help testing and the expense of slow/flaky tests
running on swarm. However, they're nervous about disagreeing with you about this
code because you might have more context that implies a browser test is
appropriate in this particular case. Could you elaborate on that extra context?

[jam, 2013-06-11 22:20:56]

On 2013/06/07 19:39:16, Jeffrey Yasskin wrote:
> On 2013/06/07 00:37:27, jam wrote:
> > On 2013/06/06 20:28:25, Jeffrey Yasskin wrote:
> > > On 2013/06/06 17:17:02, jam wrote:
> > > > On 2013/06/06 17:09:57, Jeffrey Yasskin wrote:
> > > > > On 2013/06/06 16:52:46, jam wrote:
> > > > > > question: can we avoid the changes to child_process_launcher by
making
> > > > RPHImpl
> > > > > > think it's in single process mode? and perhaps if necessary, another
> > > > > ForTesting
> > > > > > method to RPHImpl?
> > > > > 
> > > > > I don't think so. That would require implementing single-process mode
in
> > > > > SPLIT_DLL mode
> > > > >
> > > >
> > >
> >
>
(https://code.google.com/p/chromium/codesearch/#chromium/src/content/browser/r...),
> > > > 
> > > > 
> > > > I think CHROME_SPLIT_DLL is something that's orthogonal. there are
already
> > > > (browser) tests that depend on single process anyways.
> > > > 
> > > > also, i think that CHROME_SPLIT_DLL ifdef was put in before the current
> > > approach
> > > > of exporting missing symbols, so it will work in tests afaik.
> > > 
> > > CHROME_SPLIT_DLL was added on May 8 and removes the in-process renderer
> branch
> > > entirely. Are you saying that we can just delete that #if, and everything
> will
> > > keep working? 
> > 
> > right, that should work.
> > 
> > however i don't think you should be worrying about that mode in this branch.
> if
> > we switch to that mode, when that happens we will make sure single process
> mode
> > works.
> > 
> > > 
> > > > > and it wouldn't work for general unittests here since single-process
> mode
> > > > > implies a renderer thread, which wouldn't allow control over the
timing.
> > > > 
> > > > what control are you referring to? note that there's
> > > > RPHImpl::GetInProcessRendererThreadForTesting which gives access to the
> > loop.
> > > 
> > > Access to the renderer thread's MessageLoop isn't enough to cause a
renderer
> > to
> > > pause between receiving an IPC and replying to it.
> > > 
> > > In this particular case, that amount of control turned out to be
> unnecessary,
> > > since I could provoke the bug by just sending an extra message to the
> > > RenderProcessHost. (That's also what makes the browsertest below even
> > possible.)
> > > However, it's easy to imagine behaviors that happen if an IPC in the
middle
> of
> > a
> > > back-and-forth chain is delayed, and the framework in this CL will let you
> > test
> > > them.
> > > 
> > > > the other question i'd have is: if the test was an implemented as a
> browser
> > > > test, would there be less changes needed to production code?
> > > 
> > > https://codereview.chromium.org/16593002 implements this as a browser
test.
> It
> > > takes ~900ms to run, as opposed to 70ms for the unit test, and assuming
I'm
> no
> > > better at avoiding flaky tests than you are, has about an 80% chance of
> being
> > > flaky or fragile
> > >
> >
>
(http://test-results.appspot.com/dashboards/overview.html#group=%25252540DEPS%...).
> > > In order to avoid slowing down other Chrome developers, that basically
> > excludes
> > > the browsertest option when a unittest is possible.
> > 
> > I'd much prefer the browser test over the unit test. I really prefer to not
> make
> > changes to the code for tests if we can avoid it.
> > 
> > Regarding the test run time, things like swarm which will be turned on for
> > browser tests in the near future mean that adding extra tests doesn't slow
> down
> > the time.
> 
> chromium-dev appears to disagree with you about both the desirability of
> changing production code to help testing and the expense of slow/flaky tests
> running on swarm. However, they're nervous about disagreeing with you about
this
> code because you might have more context that implies a browser test is
> appropriate in this particular case. Could you elaborate on that extra
context?

I have outlined my response on the thread. please change this to a browser test.

[Jeffrey Yasskin, 2013-06-12 21:29:31]

On 2013/06/11 22:20:56, jam wrote:
> On 2013/06/07 19:39:16, Jeffrey Yasskin wrote:
> > On 2013/06/07 00:37:27, jam wrote:
> > > On 2013/06/06 20:28:25, Jeffrey Yasskin wrote:
> > > > On 2013/06/06 17:17:02, jam wrote:
> > > > > On 2013/06/06 17:09:57, Jeffrey Yasskin wrote:
> > > > > > On 2013/06/06 16:52:46, jam wrote:
> > > > > > > question: can we avoid the changes to child_process_launcher by
> making
> > > > > RPHImpl
> > > > > > > think it's in single process mode? and perhaps if necessary,
another
> > > > > > ForTesting
> > > > > > > method to RPHImpl?
> > > > > > 
> > > > > > I don't think so. That would require implementing single-process
mode
> in
> > > > > > SPLIT_DLL mode
> > > > > >
> > > > >
> > > >
> > >
> >
>
(https://code.google.com/p/chromium/codesearch/#chromium/src/content/browser/r...),
> > > > > 
> > > > > 
> > > > > I think CHROME_SPLIT_DLL is something that's orthogonal. there are
> already
> > > > > (browser) tests that depend on single process anyways.
> > > > > 
> > > > > also, i think that CHROME_SPLIT_DLL ifdef was put in before the
current
> > > > approach
> > > > > of exporting missing symbols, so it will work in tests afaik.
> > > > 
> > > > CHROME_SPLIT_DLL was added on May 8 and removes the in-process renderer
> > branch
> > > > entirely. Are you saying that we can just delete that #if, and
everything
> > will
> > > > keep working? 
> > > 
> > > right, that should work.
> > > 
> > > however i don't think you should be worrying about that mode in this
branch.
> > if
> > > we switch to that mode, when that happens we will make sure single process
> > mode
> > > works.
> > > 
> > > > 
> > > > > > and it wouldn't work for general unittests here since single-process
> > mode
> > > > > > implies a renderer thread, which wouldn't allow control over the
> timing.
> > > > > 
> > > > > what control are you referring to? note that there's
> > > > > RPHImpl::GetInProcessRendererThreadForTesting which gives access to
the
> > > loop.
> > > > 
> > > > Access to the renderer thread's MessageLoop isn't enough to cause a
> renderer
> > > to
> > > > pause between receiving an IPC and replying to it.
> > > > 
> > > > In this particular case, that amount of control turned out to be
> > unnecessary,
> > > > since I could provoke the bug by just sending an extra message to the
> > > > RenderProcessHost. (That's also what makes the browsertest below even
> > > possible.)
> > > > However, it's easy to imagine behaviors that happen if an IPC in the
> middle
> > of
> > > a
> > > > back-and-forth chain is delayed, and the framework in this CL will let
you
> > > test
> > > > them.
> > > > 
> > > > > the other question i'd have is: if the test was an implemented as a
> > browser
> > > > > test, would there be less changes needed to production code?
> > > > 
> > > > https://codereview.chromium.org/16593002 implements this as a browser
> test.
> > It
> > > > takes ~900ms to run, as opposed to 70ms for the unit test, and assuming
> I'm
> > no
> > > > better at avoiding flaky tests than you are, has about an 80% chance of
> > being
> > > > flaky or fragile
> > > >
> > >
> >
>
(http://test-results.appspot.com/dashboards/overview.html#group=%2525252540DEP...).
> > > > In order to avoid slowing down other Chrome developers, that basically
> > > excludes
> > > > the browsertest option when a unittest is possible.
> > > 
> > > I'd much prefer the browser test over the unit test. I really prefer to
not
> > make
> > > changes to the code for tests if we can avoid it.
> > > 
> > > Regarding the test run time, things like swarm which will be turned on for
> > > browser tests in the near future mean that adding extra tests doesn't slow
> > down
> > > the time.
> > 
> > chromium-dev appears to disagree with you about both the desirability of
> > changing production code to help testing and the expense of slow/flaky tests
> > running on swarm. However, they're nervous about disagreeing with you about
> this
> > code because you might have more context that implies a browser test is
> > appropriate in this particular case. Could you elaborate on that extra
> context?
> 
> I have outlined my response on the thread. please change this to a browser
test.

Done, thanks.

[jam, 2013-06-13 17:43:22]

lgtm

https://codereview.chromium.org/16267002/diff/78001/content/browser/renderer_...
File content/browser/renderer_host/render_process_host_browsertest.cc (right):

https://codereview.chromium.org/16267002/diff/78001/content/browser/renderer_...
content/browser/renderer_host/render_process_host_browsertest.cc:21:
IN_PROC_BROWSER_TEST_F(RenderProcessHostTest,
nit: please add a comment explaining the purpose of this test. also helpful to
link to the bug

[Jeffrey Yasskin, 2013-06-13 17:54:04]

https://codereview.chromium.org/16267002/diff/78001/content/browser/renderer_...
File content/browser/renderer_host/render_process_host_browsertest.cc (right):

https://codereview.chromium.org/16267002/diff/78001/content/browser/renderer_...
content/browser/renderer_host/render_process_host_browsertest.cc:21:
IN_PROC_BROWSER_TEST_F(RenderProcessHostTest,
On 2013/06/13 17:43:22, jam wrote:
> nit: please add a comment explaining the purpose of this test. also helpful to
> link to the bug

Oops, I thought I had, but that was the unit test. Done.

[commit-bot: I haz the power, 2013-06-13 17:55:48]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/jyasskin@chromium.org/16267002/86001

[commit-bot: I haz the power, 2013-06-13 22:43:20]

Change committed as 206182