|
|
Created:
7 years, 6 months ago by Jeffrey Yasskin Modified:
7 years, 6 months ago CC:
chromium-reviews, joi+watch-content_chromium.org, Aaron Boodman, darin-cc_chromium.org, chromium-apps-reviews_chromium.org Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionRe-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
Patch Set 1 #Patch Set 2 : Minimal RPHImpl test #Patch Set 3 : Test the whole fix #Patch Set 4 : Sync #Patch Set 5 : Much shorter test #
Total comments: 20
Patch Set 6 : Fix creis' comments, and split out 15793016 and 15799009 #Patch Set 7 : Second round of creis comments #
Total comments: 1
Patch Set 8 : Rebase on top of https://codereview.chromium.org/16490003/ #
Total comments: 2
Patch Set 9 : Use TestBrowserThreadBundle #Patch Set 10 : Switch to a browser test #
Total comments: 2
Patch Set 11 : Add comment #
Messages
Total messages: 21 (0 generated)
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.
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?
+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.
+mpcomplete too for revert of r190543 in extension_host.cc
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.
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.
LGTM, once you incorporate https://codereview.chromium.org/16490003/.
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?
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.
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.
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.
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
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.
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.
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?
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.
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.
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
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.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/jyasskin@chromium.org/16267002/86001
Message was sent while issue was closed.
Change committed as 206182 |