Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
9514016:
Fixing a problem, where a hung renderer process is not killed when navigating away (Closed)
|
Created:
8 years, 9 months ago by nasko Modified:
8 years, 9 months ago CC:
chromium-reviews, joi+watch-content_chromium.org, darin-cc_chromium.org, jam Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionFixing a problem, where a hung renderer process is not killed when navigating away
BUG=104346
TEST=Steps to reproduce listed in the bug.
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=126199
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=126409
Patch Set 1 : #
Total comments: 26
Patch Set 2 : Incorporating Charlie's feedback. #
Total comments: 18
Patch Set 3 : Fixes based on feedback. #Patch Set 4 : Removing the public API and replaced with iterator logic. #Patch Set 5 : Updating to HEAD #Patch Set 6 : Removed browser test assert. #Patch Set 7 : Another sync #Patch Set 8 : Fixing the mock code. #Patch Set 9 : Updated #Patch Set 10 : Fixing browsertest compile failure on ChromeOS. #Patch Set 11 : Improving the test to use Observer for reliability. #Patch Set 12 : Final update, fixing the input event bug. #Patch Set 13 : Final cleanup update. #
Messages
Total messages: 24 (0 generated)
This patch ensures we kill any hung renderer processes, while doing a cross-site navigation.
https://chromiumcodereview.appspot.com/9514016/diff/12004/content/browser/ren... File content/browser/renderer_host/mock_render_process_host.cc (right): https://chromiumcodereview.appspot.com/9514016/diff/12004/content/browser/ren... content/browser/renderer_host/mock_render_process_host.cc:218: return false; Is this necessary for tests? Usually the Mock version just hardcodes a reasonable answer, like true. https://chromiumcodereview.appspot.com/9514016/diff/12004/content/browser/ren... File content/browser/renderer_host/render_process_host_browsertest.cc (right): https://chromiumcodereview.appspot.com/9514016/diff/12004/content/browser/ren... content/browser/renderer_host/render_process_host_browsertest.cc:261: base::ProcessHandle ph = NULL; nit: Rename to process. https://chromiumcodereview.appspot.com/9514016/diff/12004/content/browser/ren... content/browser/renderer_host/render_process_host_browsertest.cc:271: GURL infinite_url(test_server()->GetURL("files/infinite_beforeunload.html")); Why did you choose to hang in beforeunload instead of unload? If there's a reason, might be good to have a comment. https://chromiumcodereview.appspot.com/9514016/diff/12004/content/browser/ren... content/browser/renderer_host/render_process_host_browsertest.cc:273: GURL regular_url(https_server.GetURL("files/english_page.html")); What does regular refer to? Maybe new_process_url? https://chromiumcodereview.appspot.com/9514016/diff/12004/content/browser/ren... content/browser/renderer_host/render_process_host_browsertest.cc:275: // Navigate the tab to the page which will lock up the process. nit: ...when we navigate away. https://chromiumcodereview.appspot.com/9514016/diff/12004/content/browser/ren... content/browser/renderer_host/render_process_host_browsertest.cc:286: EXPECT_FALSE(base::KillProcess(ph, 1, false)); Add a comment that this should fail because the process should already be killed, since it's sort of an indirect way to check that it's gone. https://chromiumcodereview.appspot.com/9514016/diff/12004/content/browser/ren... File content/browser/renderer_host/render_process_host_impl.h (right): https://chromiumcodereview.appspot.com/9514016/diff/12004/content/browser/ren... content/browser/renderer_host/render_process_host_impl.h:90: virtual bool AllowTerminateOnUnresponsive() const OVERRIDE; Let's rename this to TerminateOnUnresponsiveAllowed() and move it to below SuddenTerminationAllowed. https://chromiumcodereview.appspot.com/9514016/diff/12004/content/browser/ren... File content/browser/renderer_host/render_view_host.cc (right): https://chromiumcodereview.appspot.com/9514016/diff/12004/content/browser/ren... content/browser/renderer_host/render_view_host.cc:406: // only one aware of swapping render views on navigation. More specifically, it is the only class that swaps out RenderViewHosts. (I believe prerendering and instant will swap them in, but this doesn't affect that.) https://chromiumcodereview.appspot.com/9514016/diff/12004/content/browser/ren... content/browser/renderer_host/render_view_host.cc:415: // complexity to solve this edge case is not worthwhile. I'm having trouble following this comment... Not sure I have suggestions yet for how to improve it, though. https://chromiumcodereview.appspot.com/9514016/diff/12004/content/browser/ren... content/browser/renderer_host/render_view_host.cc:416: if (SuddenTerminationAllowed()) { nit: No braces on one-line if. https://chromiumcodereview.appspot.com/9514016/diff/12004/content/public/brow... File content/public/browser/render_process_host.h (right): https://chromiumcodereview.appspot.com/9514016/diff/12004/content/public/brow... content/public/browser/render_process_host.h:118: virtual bool AllowTerminateOnUnresponsive() const = 0; We should move this to below SuddenTerminationAllowed and make the name more consistent (with Allowed at the end). I'm trying to decide if something like TerminateAfterLeavingAllowed() is a better name, or if we want it to be more generic, like IsUsedByMultipleViews()... It's possible there may be other uses for this check.
Updated based on Charlie's feedback. https://chromiumcodereview.appspot.com/9514016/diff/12004/content/browser/ren... File content/browser/renderer_host/mock_render_process_host.cc (right): https://chromiumcodereview.appspot.com/9514016/diff/12004/content/browser/ren... content/browser/renderer_host/mock_render_process_host.cc:218: return false; On 2012/03/01 21:08:17, creis wrote: > Is this necessary for tests? Usually the Mock version just hardcodes a > reasonable answer, like true. Is there a reason to have a hardcoded value, especially with such a simple function body? https://chromiumcodereview.appspot.com/9514016/diff/12004/content/browser/ren... File content/browser/renderer_host/render_process_host_browsertest.cc (right): https://chromiumcodereview.appspot.com/9514016/diff/12004/content/browser/ren... content/browser/renderer_host/render_process_host_browsertest.cc:261: base::ProcessHandle ph = NULL; On 2012/03/01 21:08:17, creis wrote: > nit: Rename to process. Done. https://chromiumcodereview.appspot.com/9514016/diff/12004/content/browser/ren... content/browser/renderer_host/render_process_host_browsertest.cc:271: GURL infinite_url(test_server()->GetURL("files/infinite_beforeunload.html")); On 2012/03/01 21:08:17, creis wrote: > Why did you choose to hang in beforeunload instead of unload? If there's a > reason, might be good to have a comment. No real reason. After the latest discussions, it might actually be better to hang on the unload handler, so I made the second navigation use the beforeunload. https://chromiumcodereview.appspot.com/9514016/diff/12004/content/browser/ren... content/browser/renderer_host/render_process_host_browsertest.cc:273: GURL regular_url(https_server.GetURL("files/english_page.html")); On 2012/03/01 21:08:17, creis wrote: > What does regular refer to? Maybe new_process_url? Done. https://chromiumcodereview.appspot.com/9514016/diff/12004/content/browser/ren... content/browser/renderer_host/render_process_host_browsertest.cc:275: // Navigate the tab to the page which will lock up the process. On 2012/03/01 21:08:17, creis wrote: > nit: ...when we navigate away. Done. https://chromiumcodereview.appspot.com/9514016/diff/12004/content/browser/ren... content/browser/renderer_host/render_process_host_browsertest.cc:286: EXPECT_FALSE(base::KillProcess(ph, 1, false)); On 2012/03/01 21:08:17, creis wrote: > Add a comment that this should fail because the process should already be > killed, since it's sort of an indirect way to check that it's gone. Done. https://chromiumcodereview.appspot.com/9514016/diff/12004/content/browser/ren... File content/browser/renderer_host/render_process_host_impl.h (right): https://chromiumcodereview.appspot.com/9514016/diff/12004/content/browser/ren... content/browser/renderer_host/render_process_host_impl.h:90: virtual bool AllowTerminateOnUnresponsive() const OVERRIDE; On 2012/03/01 21:08:17, creis wrote: > Let's rename this to TerminateOnUnresponsiveAllowed() and move it to below > SuddenTerminationAllowed. Done. https://chromiumcodereview.appspot.com/9514016/diff/12004/content/browser/ren... File content/browser/renderer_host/render_view_host.cc (right): https://chromiumcodereview.appspot.com/9514016/diff/12004/content/browser/ren... content/browser/renderer_host/render_view_host.cc:406: // only one aware of swapping render views on navigation. On 2012/03/01 21:08:17, creis wrote: > More specifically, it is the only class that swaps out RenderViewHosts. (I > believe prerendering and instant will swap them in, but this doesn't affect > that.) Done. https://chromiumcodereview.appspot.com/9514016/diff/12004/content/browser/ren... content/browser/renderer_host/render_view_host.cc:416: if (SuddenTerminationAllowed()) { On 2012/03/01 21:08:17, creis wrote: > nit: No braces on one-line if. Done. https://chromiumcodereview.appspot.com/9514016/diff/12004/content/public/brow... File content/public/browser/render_process_host.h (right): https://chromiumcodereview.appspot.com/9514016/diff/12004/content/public/brow... content/public/browser/render_process_host.h:118: virtual bool AllowTerminateOnUnresponsive() const = 0; On 2012/03/01 21:08:17, creis wrote: > We should move this to below SuddenTerminationAllowed and make the name more > consistent (with Allowed at the end). > > I'm trying to decide if something like TerminateAfterLeavingAllowed() is a > better name, or if we want it to be more generic, like > IsUsedByMultipleViews()... It's possible there may be other uses for this > check. Done.
Thanks-- more comments inline. https://chromiumcodereview.appspot.com/9514016/diff/12004/content/browser/ren... File content/browser/renderer_host/mock_render_process_host.cc (right): https://chromiumcodereview.appspot.com/9514016/diff/12004/content/browser/ren... content/browser/renderer_host/mock_render_process_host.cc:218: return false; On 2012/03/01 22:50:15, nasko wrote: > On 2012/03/01 21:08:17, creis wrote: > > Is this necessary for tests? Usually the Mock version just hardcodes a > > reasonable answer, like true. > > Is there a reason to have a hardcoded value, especially with such a simple > function body? In general, it might get out of date if we change the other one. It might also not do what you think, depending on how listeners_ is updated in the Mock. https://chromiumcodereview.appspot.com/9514016/diff/12004/content/public/brow... File content/public/browser/render_process_host.h (right): https://chromiumcodereview.appspot.com/9514016/diff/12004/content/public/brow... content/public/browser/render_process_host.h:118: virtual bool AllowTerminateOnUnresponsive() const = 0; On 2012/03/01 22:50:15, nasko wrote: > On 2012/03/01 21:08:17, creis wrote: > > We should move this to below SuddenTerminationAllowed and make the name more > > consistent (with Allowed at the end). > > > > I'm trying to decide if something like TerminateAfterLeavingAllowed() is a > > better name, or if we want it to be more generic, like > > IsUsedByMultipleViews()... It's possible there may be other uses for this > > check. > > Done. Upon reflection, I'm leaning toward something like IsSharedByMultipleViews(). This is information about a process state (is it shared or not?) that might useful for many policy decisions-- whether to kill if leaving in an unresponsive state, whether we should prerender in this tab, and possibly others. https://chromiumcodereview.appspot.com/9514016/diff/18001/content/browser/ren... File content/browser/renderer_host/render_view_host.cc (right): https://chromiumcodereview.appspot.com/9514016/diff/18001/content/browser/ren... content/browser/renderer_host/render_view_host.cc:406: // only class that swaps render view hosts on navigation. Nit: swaps out https://chromiumcodereview.appspot.com/9514016/diff/18001/content/browser/ren... content/browser/renderer_host/render_view_host.cc:409: // If we load data URLs, such as about:blank, there will be no network nit: data URLs or about:blank https://chromiumcodereview.appspot.com/9514016/diff/18001/content/browser/ren... content/browser/renderer_host/render_view_host.cc:410: // request and we arrive here without timeout. Since the TabContents we arrive here without setting an unresponsiveness timer. https://chromiumcodereview.appspot.com/9514016/diff/18001/content/browser/ren... content/browser/renderer_host/render_view_host.cc:411: // sets the sudden termination allowed on real timeout, respect it and nit: sets SuddenTerminationAllowed in the case that a timer is set. https://chromiumcodereview.appspot.com/9514016/diff/18001/content/browser/ren... content/browser/renderer_host/render_view_host.cc:412: // don't kill the process. This allows a corner case where a navigation I think the problem I'm having is that this feels out of order. We should probably describe the common case first-- only kill the process if TabContents sets SuddenTerminationAllowed, since that indicates the timer expired. Then explain the corner case (data/blank URLs that don't hit that logic) and why we chose not to deal with it.
http://codereview.chromium.org/9514016/diff/18001/content/browser/renderer_ho... File content/browser/renderer_host/render_process_host_browsertest.cc (right): http://codereview.chromium.org/9514016/diff/18001/content/browser/renderer_ho... content/browser/renderer_host/render_process_host_browsertest.cc:268: FilePath(FILE_PATH_LITERAL("chrome/test/data"))); please don't hard code paths in tests. can you put this in content/test/data and use content::DIR_TEST_DATA? http://codereview.chromium.org/9514016/diff/18001/content/public/browser/rend... File content/public/browser/render_process_host.h (right): http://codereview.chromium.org/9514016/diff/18001/content/public/browser/rend... content/public/browser/render_process_host.h:185: virtual bool TerminateOnUnresponsiveAllowed() const = 0; i dont think this new method is necessary? can't the caller just check ListenersIterator() itself?
I've made a bunch of changes based on the feedback. http://codereview.chromium.org/9514016/diff/12004/content/browser/renderer_ho... File content/browser/renderer_host/mock_render_process_host.cc (right): http://codereview.chromium.org/9514016/diff/12004/content/browser/renderer_ho... content/browser/renderer_host/mock_render_process_host.cc:218: return false; On 2012/03/02 00:16:43, creis wrote: > On 2012/03/01 22:50:15, nasko wrote: > > On 2012/03/01 21:08:17, creis wrote: > > > Is this necessary for tests? Usually the Mock version just hardcodes a > > > reasonable answer, like true. > > > > Is there a reason to have a hardcoded value, especially with such a simple > > function body? > > In general, it might get out of date if we change the other one. It might also > not do what you think, depending on how listeners_ is updated in the Mock. If the function is now renamed to IsSharedByMultipleViews, does it make sense to hard code the value, especially when the mock host does proper accounting for listeners? http://codereview.chromium.org/9514016/diff/12004/content/public/browser/rend... File content/public/browser/render_process_host.h (right): http://codereview.chromium.org/9514016/diff/12004/content/public/browser/rend... content/public/browser/render_process_host.h:118: virtual bool AllowTerminateOnUnresponsive() const = 0; On 2012/03/02 00:16:43, creis wrote: > On 2012/03/01 22:50:15, nasko wrote: > > On 2012/03/01 21:08:17, creis wrote: > > > We should move this to below SuddenTerminationAllowed and make the name more > > > consistent (with Allowed at the end). > > > > > > I'm trying to decide if something like TerminateAfterLeavingAllowed() is a > > > better name, or if we want it to be more generic, like > > > IsUsedByMultipleViews()... It's possible there may be other uses for this > > > check. > > > > Done. > > Upon reflection, I'm leaning toward something like IsSharedByMultipleViews(). > This is information about a process state (is it shared or not?) that might > useful for many policy decisions-- whether to kill if leaving in an unresponsive > state, whether we should prerender in this tab, and possibly others. This makes sense, especially if we want the prerendering to use this type of information. http://codereview.chromium.org/9514016/diff/18001/content/browser/renderer_ho... File content/browser/renderer_host/render_process_host_browsertest.cc (right): http://codereview.chromium.org/9514016/diff/18001/content/browser/renderer_ho... content/browser/renderer_host/render_process_host_browsertest.cc:268: FilePath(FILE_PATH_LITERAL("chrome/test/data"))); On 2012/03/02 00:21:10, John Abd-El-Malek wrote: > please don't hard code paths in tests. > > can you put this in content/test/data and use content::DIR_TEST_DATA? I've moved the files to the content/test/data, but I can't use the PathService, as it gives back an absolute path and the test server expects a relative path. Also, I can't use the test_server part of the infrastructure, since it is hardcoded to the chrome path and no way to change it. http://codereview.chromium.org/9514016/diff/18001/content/browser/renderer_ho... File content/browser/renderer_host/render_view_host.cc (right): http://codereview.chromium.org/9514016/diff/18001/content/browser/renderer_ho... content/browser/renderer_host/render_view_host.cc:406: // only class that swaps render view hosts on navigation. On 2012/03/02 00:16:43, creis wrote: > Nit: swaps out Done. http://codereview.chromium.org/9514016/diff/18001/content/browser/renderer_ho... content/browser/renderer_host/render_view_host.cc:409: // If we load data URLs, such as about:blank, there will be no network On 2012/03/02 00:16:43, creis wrote: > nit: data URLs or about:blank Done. http://codereview.chromium.org/9514016/diff/18001/content/browser/renderer_ho... content/browser/renderer_host/render_view_host.cc:410: // request and we arrive here without timeout. Since the TabContents On 2012/03/02 00:16:43, creis wrote: > we arrive here without setting an unresponsiveness timer. Done. http://codereview.chromium.org/9514016/diff/18001/content/browser/renderer_ho... content/browser/renderer_host/render_view_host.cc:411: // sets the sudden termination allowed on real timeout, respect it and On 2012/03/02 00:16:43, creis wrote: > nit: sets SuddenTerminationAllowed in the case that a timer is set. Done. http://codereview.chromium.org/9514016/diff/18001/content/browser/renderer_ho... content/browser/renderer_host/render_view_host.cc:412: // don't kill the process. This allows a corner case where a navigation On 2012/03/02 00:16:43, creis wrote: > I think the problem I'm having is that this feels out of order. We should > probably describe the common case first-- only kill the process if TabContents > sets SuddenTerminationAllowed, since that indicates the timer expired. > > Then explain the corner case (data/blank URLs that don't hit that logic) and why > we chose not to deal with it. Rewrote the comment, hopefully it is better. http://codereview.chromium.org/9514016/diff/18001/content/public/browser/rend... File content/public/browser/render_process_host.h (right): http://codereview.chromium.org/9514016/diff/18001/content/public/browser/rend... content/public/browser/render_process_host.h:185: virtual bool TerminateOnUnresponsiveAllowed() const = 0; On 2012/03/02 00:21:10, John Abd-El-Malek wrote: > i dont think this new method is necessary? can't the caller just check > ListenersIterator() itself? They probably could, but this means that they should be aware of the fact that listeners are equivalent to views using the host (could be apparent, but it wasn't to me). If you believe that we shouldn't be exposing such a function, I can change the code to iterate through the listeners to get a count, but that makes it harder for potential future changes.
LGTM http://codereview.chromium.org/9514016/diff/12004/content/browser/renderer_ho... File content/browser/renderer_host/mock_render_process_host.cc (right): http://codereview.chromium.org/9514016/diff/12004/content/browser/renderer_ho... content/browser/renderer_host/mock_render_process_host.cc:218: return false; On 2012/03/02 18:40:36, nasko wrote: > On 2012/03/02 00:16:43, creis wrote: > > On 2012/03/01 22:50:15, nasko wrote: > > > On 2012/03/01 21:08:17, creis wrote: > > > > Is this necessary for tests? Usually the Mock version just hardcodes a > > > > reasonable answer, like true. > > > > > > Is there a reason to have a hardcoded value, especially with such a simple > > > function body? > > > > In general, it might get out of date if we change the other one. It might > also > > not do what you think, depending on how listeners_ is updated in the Mock. > > If the function is now renamed to IsSharedByMultipleViews, does it make sense to > hard code the value, especially when the mock host does proper accounting for > listeners? No, we can leave it as is. http://codereview.chromium.org/9514016/diff/18001/content/browser/renderer_ho... File content/browser/renderer_host/render_view_host.cc (right): http://codereview.chromium.org/9514016/diff/18001/content/browser/renderer_ho... content/browser/renderer_host/render_view_host.cc:412: // don't kill the process. This allows a corner case where a navigation On 2012/03/02 18:40:36, nasko wrote: > On 2012/03/02 00:16:43, creis wrote: > > I think the problem I'm having is that this feels out of order. We should > > probably describe the common case first-- only kill the process if TabContents > > sets SuddenTerminationAllowed, since that indicates the timer expired. > > > > Then explain the corner case (data/blank URLs that don't hit that logic) and > why > > we chose not to deal with it. > > Rewrote the comment, hopefully it is better. Much better, thanks! http://codereview.chromium.org/9514016/diff/18001/content/public/browser/rend... File content/public/browser/render_process_host.h (right): http://codereview.chromium.org/9514016/diff/18001/content/public/browser/rend... content/public/browser/render_process_host.h:185: virtual bool TerminateOnUnresponsiveAllowed() const = 0; On 2012/03/02 18:40:36, nasko wrote: > On 2012/03/02 00:21:10, John Abd-El-Malek wrote: > > i dont think this new method is necessary? can't the caller just check > > ListenersIterator() itself? > > They probably could, but this means that they should be aware of the fact that > listeners are equivalent to views using the host (could be apparent, but it > wasn't to me). > If you believe that we shouldn't be exposing such a function, I can change the > code to iterate through the listeners to get a count, but that makes it harder > for potential future changes. I agree with Nasko. It's not clear to users of this class that listeners_ happens to be equivalent to the set of views being displayed by the process. We might attach other types of IPC::Channel::Listeners in the future, and callers of this method should be shielded from that.
John, can you take a look at this one today? Thanks.
http://codereview.chromium.org/9514016/diff/18001/content/public/browser/rend... File content/public/browser/render_process_host.h (right): http://codereview.chromium.org/9514016/diff/18001/content/public/browser/rend... content/public/browser/render_process_host.h:185: virtual bool TerminateOnUnresponsiveAllowed() const = 0; On 2012/03/02 19:17:02, creis wrote: > On 2012/03/02 18:40:36, nasko wrote: > > On 2012/03/02 00:21:10, John Abd-El-Malek wrote: > > > i dont think this new method is necessary? can't the caller just check > > > ListenersIterator() itself? > > > > They probably could, but this means that they should be aware of the fact that > > listeners are equivalent to views using the host (could be apparent, but it > > wasn't to me). > > If you believe that we shouldn't be exposing such a function, I can change the > > code to iterate through the listeners to get a count, but that makes it harder > > for potential future changes. > > I agree with Nasko. It's not clear to users of this class that listeners_ > happens to be equivalent to the set of views being displayed by the process. We > might attach other types of IPC::Channel::Listeners in the future, and callers > of this method should be shielded from that. There's a cost every time we add a method to a public API in terms of adding to the list of methods that someone not familiar with content has to figure out. We've tried really hard to keep the public API to a minimum. In this particular case, chrome already has a number of places that make this assumption and even iterate through them through ListenersIterator() and cast to RenderWidgetHost.
Removed the public API. http://codereview.chromium.org/9514016/diff/18001/content/public/browser/rend... File content/public/browser/render_process_host.h (right): http://codereview.chromium.org/9514016/diff/18001/content/public/browser/rend... content/public/browser/render_process_host.h:185: virtual bool TerminateOnUnresponsiveAllowed() const = 0; On 2012/03/06 17:25:26, John Abd-El-Malek wrote: > On 2012/03/02 19:17:02, creis wrote: > > On 2012/03/02 18:40:36, nasko wrote: > > > On 2012/03/02 00:21:10, John Abd-El-Malek wrote: > > > > i dont think this new method is necessary? can't the caller just check > > > > ListenersIterator() itself? > > > > > > They probably could, but this means that they should be aware of the fact > that > > > listeners are equivalent to views using the host (could be apparent, but it > > > wasn't to me). > > > If you believe that we shouldn't be exposing such a function, I can change > the > > > code to iterate through the listeners to get a count, but that makes it > harder > > > for potential future changes. > > > > I agree with Nasko. It's not clear to users of this class that listeners_ > > happens to be equivalent to the set of views being displayed by the process. > We > > might attach other types of IPC::Channel::Listeners in the future, and callers > > of this method should be shielded from that. > > There's a cost every time we add a method to a public API in terms of adding to > the list of methods that someone not familiar with content has to figure out. > We've tried really hard to keep the public API to a minimum. > > In this particular case, chrome already has a number of places that make this > assumption and even iterate through them through ListenersIterator() and cast to > RenderWidgetHost. The reason I picked the function is that the only real code I could see using the iterators was the memory details code, which has big warning about the usage. I've changed it to use the iterator.
lgtm
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/nasko@chromium.org/9514016/33001
Can't apply patch for file content/browser/renderer_host/render_view_host.cc. While running patch -p1 --forward --force; patching file content/browser/renderer_host/render_view_host.cc Hunk #1 FAILED at 41. Hunk #2 FAILED at 380. Hunk #3 FAILED at 388. 3 out of 3 hunks FAILED -- saving rejects to file content/browser/renderer_host/render_view_host.cc.rej
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/nasko@chromium.org/9514016/36001
Can't apply patch for file content/browser/renderer_host/render_view_host.cc. While running patch -p1 --forward --force; patching file content/browser/renderer_host/render_view_host.cc Hunk #1 FAILED at 41. Hunk #2 FAILED at 415. Hunk #3 FAILED at 423. 3 out of 3 hunks FAILED -- saving rejects to file content/browser/renderer_host/render_view_host.cc.rej
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/nasko@chromium.org/9514016/42001
Try job failure for 9514016-42001 (retry) on linux_rel for step "browser_tests". It's a second try, previously, step "browser_tests" failed. http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_rel&...
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/nasko@chromium.org/9514016/44003
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/nasko@chromium.org/9514016/44003
Try job failure for 9514016-44003 (retry) on win_rel for steps "unit_tests, content_unittests, browser_tests" (clobber build). It's a second try, previously, step "compile" failed. http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...
Still LGTM. I think the change to MockRenderProcessHost might help with the windows failures. I was seeing related issues on another CL of mine that tried to use the ListenersIterator. I'll give it another try.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/nasko@chromium.org/9514016/61001
Try job failure for 9514016-61001 (retry) (retry) on win_rel for step "browser_tests" (clobber build). It's a second try, previously, step "compile" failed. http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...
Since the commit queue was having trouble with the Windows bot (looks like http://crbug.com/113031), I've committed this manually: http://src.chromium.org/viewvc/chrome?view=rev&revision=126151 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
