Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2857263003:
Task Manager should listen to WebContentsObserver::RenderFrameCreated. (Closed)
|
Created:
3 years, 7 months ago by Łukasz Anforowicz Modified:
3 years, 7 months ago Reviewers:
ncarter (slow) CC:
chromium-reviews, jam, darin-cc_chromium.org Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
DescriptionTask Manager should listen to WebContentsObserver::RenderFrameCreated.
Listening to WebContentsObserver::RenderFrameCreated is the only way to
realize that a RenderFrameHost got recreated after a render process
crash - Task Manager needs to listen to this callback to re-show frames
that were re-navigated after a render process crash.
A slight problem with this approach is that the PID of the render
process is not necessarily known when RenderFrameCreated is called - to
work around that Task Manager postpones processing of the new frame via
PostTaskWhenProcessIsReady (an instance method added to RenderProcessHost
in this CL).
BUG=642958
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
Review-Url: https://codereview.chromium.org/2857263003
Cr-Commit-Position: refs/heads/master@{#472325}
Committed: https://chromium.googlesource.com/chromium/src/+/f2c4f1f94d10b019363f2d8c45defe00881cb5cb
Patch Set 1 #
Total comments: 3
Patch Set 2 : . #
Total comments: 5
Patch Set 3 : Make PostTaskWhenProcessIsReady an instance method of RPH. #
Total comments: 14
Patch Set 4 : Addressing the easy part of CR feedback from nick@. #Patch Set 5 : Avoid calling CreateTaskForFrame twice. #Patch Set 6 : Don't listen to RenderViewReady, ignore !IsRenderFrameLive, ignore non-current RFHs. #Patch Set 7 : RenderFrameHost::IsCurrent #
Total comments: 4
Patch Set 8 : Tweaked the comments as suggested in the CR feedback #Messages
Total messages: 44 (34 generated)
The CQ bit was checked by lukasza@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by lukasza@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
lukasza@chromium.org changed reviewers: + nick@chromium.org
nick@, could you PTAL? https://codereview.chromium.org/2857263003/diff/1/chrome/browser/task_manager... File chrome/browser/task_manager/task_manager_browsertest.cc (right): https://codereview.chromium.org/2857263003/diff/1/chrome/browser/task_manager... chrome/browser/task_manager/task_manager_browsertest.cc:865: "document.getElementById('frame1').src = '" + b_url.spec() + "';")); Initially I couldn't get the test to repro the problem, because I was doing 'f.src = f.src' which would go through a process swap. https://codereview.chromium.org/2857263003/diff/1/content/public/browser/rend... File content/public/browser/render_process_host_utils.h (right): https://codereview.chromium.org/2857263003/diff/1/content/public/browser/rend... content/public/browser/render_process_host_utils.h:20: CONTENT_EXPORT void PostTaskWhenRenderProcessHostIsReady( I really liked and recommend reading http://www.gotw.ca/gotw/084.htm - standalone functions FTW! :-) But, please let me know if you think I should either 1) move the function declaration to render_frame_host.h or 2) you'd prefer to see this as a static or instance method of RenderProcessHost. https://codereview.chromium.org/2857263003/diff/1/content/public/browser/rend... content/public/browser/render_process_host_utils.h:22: base::OnceClosure task); I wondered whether this should be a base::OnceCallback<void(RenderProcessHost*)>, but YAGNI + it is safe to have the OnceClosure close over base::Unretained pointer to RenderProcessHost. https://codereview.chromium.org/2857263003/diff/20001/chrome/browser/task_man... File chrome/browser/task_manager/providers/web_contents/web_contents_task_provider.cc (left): https://codereview.chromium.org/2857263003/diff/20001/chrome/browser/task_man... chrome/browser/task_manager/providers/web_contents/web_contents_task_provider.cc:52: RenderFrameHost* new_host) override; Initially I've thought that I can remove RenderFrameHostChanged (thinking that RenderFrameCreated would always cover the |new_host| from RenderFrameHostChanged), but that turned out to not be true as shown by failures in TaskManagerBrowserTest.NoticeHostedAppTabChanges and/or TaskManagerOOPIFBrowserTest.NavigateToSiteWithSubframeToOriginalSite. https://codereview.chromium.org/2857263003/diff/20001/chrome/browser/task_man... chrome/browser/task_manager/providers/web_contents/web_contents_task_provider.cc:206: DCHECK(!tasks_by_frames_.count(render_frame_host)); This DCHECK will be violated, because (as I described in another comment) we have to have both RenderFrameCreated and RenderFrameHostChanged (and so - a task for the same RFH will be created twice - once from each of WCO callbacks). Interestingly, if there is an existing entry, it sometimes is incorrect, so we need to clear the old entry and recreate it. Otherwise we get failures in TaskManagerBrowserTest.NoticeHostedAppTabChanges and/or TaskManagerOOPIFBrowserTest.NavigateToSiteWithSubframeToOriginalSite. To be honest, I don't really understand why the tasks might be stale and need to be recreated and somewhat blindly tried a few things until all the tests started passing :-/ https://codereview.chromium.org/2857263003/diff/20001/chrome/browser/task_man... File chrome/browser/task_manager/task_manager_browsertest.cc (right): https://codereview.chromium.org/2857263003/diff/20001/chrome/browser/task_man... chrome/browser/task_manager/task_manager_browsertest.cc:224: ::testing::Values(true)); Having "DefaultIsolation" or "SitePerProcess" in the test output (or in --gtest_filter) is nicer than trying to figure out what is meant by 0 and what is meant by 1.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
https://codereview.chromium.org/2857263003/diff/20001/content/public/browser/... File content/public/browser/render_process_host_utils.h (right): https://codereview.chromium.org/2857263003/diff/20001/content/public/browser/... content/public/browser/render_process_host_utils.h:22: base::OnceClosure task); Also - please shout if you think I should add a test specifically targeting the newly added function. I was kind of on the fence whether the task manager browser tests are providing sufficient test coverage for the new function (they probably are, but are doing this in a rather indirect way).
The CQ bit was checked by lukasza@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from ========== Task Manager should listen to WebContentsObserver::RenderFrameCreated. Listening to WebContentsObserver::RenderFrameCreated is the only way to realize that a RenderFrameHost got recreated after a render process crash - Task Manager needs to listen to this callback to re-show frames that were re-navigated after a render process crash. A slight problem with this approach is that the PID of the render process is now necessarily known when RenderFrameCreated is called - to work around that Task Manager postpones processing of the new frame via PostTaskWhenRenderProcessHostIsReady (a helper function introduced in this CL). BUG=642958 ========== to ========== Task Manager should listen to WebContentsObserver::RenderFrameCreated. Listening to WebContentsObserver::RenderFrameCreated is the only way to realize that a RenderFrameHost got recreated after a render process crash - Task Manager needs to listen to this callback to re-show frames that were re-navigated after a render process crash. A slight problem with this approach is that the PID of the render process is not necessarily known when RenderFrameCreated is called - to work around that Task Manager postpones processing of the new frame via PostTaskWhenProcessIsReady (an instance method added to RenderProcessHost in this CL). BUG=642958 ==========
nick@ - FYI - I've made PostTaskWhenProcessIsReady an instance method of RenderProcessHost as we've discussed earlier today.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: cast_shell_linux on master.tryserver.chromium.linux (JOB_TIMED_OUT, build has not started yet; builder either lacks capacity or does not exist (misspelled?)) chromeos_amd64-generic_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, build has not started yet; builder either lacks capacity or does not exist (misspelled?)) chromeos_daisy_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, build has not started yet; builder either lacks capacity or does not exist (misspelled?)) chromium_presubmit on master.tryserver.chromium.linux (JOB_TIMED_OUT, build has not started yet; builder either lacks capacity or does not exist (misspelled?)) linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, build has not started yet; builder either lacks capacity or does not exist (misspelled?)) linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, build has not started yet; builder either lacks capacity or does not exist (misspelled?)) linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, build has not started yet; builder either lacks capacity or does not exist (misspelled?)) linux_chromium_compile_dbg_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, build has not started yet; builder either lacks capacity or does not exist (misspelled?)) linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, build has not started yet; builder either lacks capacity or does not exist (misspelled?)) linux_chromium_tsan_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, build has not started yet; builder either lacks capacity or does not exist (misspelled?))
https://codereview.chromium.org/2857263003/diff/20001/chrome/browser/task_man... File chrome/browser/task_manager/task_manager_browsertest.cc (right): https://codereview.chromium.org/2857263003/diff/20001/chrome/browser/task_man... chrome/browser/task_manager/task_manager_browsertest.cc:224: ::testing::Values(true)); On 2017/05/05 22:01:06, Łukasz A. wrote: > Having "DefaultIsolation" or "SitePerProcess" in the test output (or in > --gtest_filter) is nicer than trying to figure out what is meant by 0 and what > is meant by 1. This is great. https://codereview.chromium.org/2857263003/diff/40001/chrome/browser/task_man... File chrome/browser/task_manager/providers/web_contents/web_contents_task_provider.cc (right): https://codereview.chromium.org/2857263003/diff/40001/chrome/browser/task_man... chrome/browser/task_manager/providers/web_contents/web_contents_task_provider.cc:158: render_frame_host->GetProcess()->PostTaskWhenProcessIsReady(base::Bind( When we show the task manager, this means we will post 1 task per RFH, browser-wide. That seems like a lot of tasks, especially considering most RFH's are going to be same-process with their parent. https://codereview.chromium.org/2857263003/diff/40001/chrome/browser/task_man... chrome/browser/task_manager/providers/web_contents/web_contents_task_provider.cc:168: if (render_frame_host) This would need to be: render_frame_host && render_frame_host->IsRenderFrameLive() && /* something to rule out this being a pending delete host */ https://codereview.chromium.org/2857263003/diff/40001/chrome/browser/task_man... chrome/browser/task_manager/providers/web_contents/web_contents_task_provider.cc:238: void WebContentsEntry::CreateTaskForFrame(RenderFrameHost* render_frame_host) { This function is implemented assuming that |render_frame_host|'s ancestors have already been processed (see the subsequent comments for some subtle implications of that. So I'm wondering if we can preserve that invariant by making the enumeration work like this: - Remove the ForEachFrame() in CreateAllTasks, and make CreateTaskForFrame be responsible for adding children (most of the time, |render_frame_host| will not have children at this point). - At the bottom of CreateTaskForFrame, recursively call CreateTaskForFrame on all children. (might require a content/public change, but RenderFrameHost::GetChildren() or ForEachChild() would not be unwelcome, IMO). - At the top of CreateTaskForFrame, do the following "filter" and "defer" operations: if (!render_frame_host_->IsRenderFrameLive()) return; // Sad tab; ignore. if (render_frame_host_->GetParent() not in tasks_by_frames_) { return; // do nothing, we're waiting on an ancestor. if (!render_frame_host_->GetProcess()->IsReady()) { render_frame_host->GetProcess()->PostTaskWhenProcessIsReady(); pending_frames_.push_back(render_frame_host); // maybe return; // Defer this subtree until we get a PID. } - Figure out what, if anything, |pending_frames_| above ought to be. It could potentially be something like a map<RenderFrameHost, WeakPtrFactory<RenderFrameHost>>, if we wanted to support cancelling all outstanding callbacks; but a manually curated set would probably work well too. My gut sense is that we need something to support cancellation when RFH's leave the current slot in their FTN -- especially the ClearTaskForFrame(old_host) that happens in RenderFrameHostChanged. https://codereview.chromium.org/2857263003/diff/40001/chrome/browser/task_man... chrome/browser/task_manager/providers/web_contents/web_contents_task_provider.cc:240: ClearTaskForFrame(render_frame_host); When does this happen? If |render_frame_host| is the main frame, this cleanup is unsafe, because existing subframe tasks hold onto a reference to the task of the main frame https://cs.chromium.org/chromium/src/chrome/browser/task_manager/providers/we... In the edit history of the task manager, we've generally tried to avoid the "deleting and re-add, just to be sure" pattern, since it turns out to not usually be a total no-op. Making this change causes the window to repaint, and can cause UI state (like the row selection) to be lost, and it regenerates the ID, which can cause rows to jump around in the order. So, I'd prefer to keep these DCHECKs if possible. https://codereview.chromium.org/2857263003/diff/40001/chrome/browser/task_man... chrome/browser/task_manager/providers/web_contents/web_contents_task_provider.cc:243: if (!site_instance->GetProcess()->HasConnection()) Should probably be IsReady() https://codereview.chromium.org/2857263003/diff/40001/content/browser/rendere... File content/browser/renderer_host/render_process_host_impl.cc (right): https://codereview.chromium.org/2857263003/diff/40001/content/browser/rendere... content/browser/renderer_host/render_process_host_impl.cc:552: delete this; I'd probably elect to |delete this| unconditionally here, if we get here and the process has died already. Otherwise the Observer could live on until the user reloads the page again, at which point the caller would probably queue up another PostTaskWhenProcessIsReady. https://codereview.chromium.org/2857263003/diff/40001/content/public/browser/... File content/public/browser/render_process_host.h (right): https://codereview.chromium.org/2857263003/diff/40001/content/public/browser/... content/public/browser/render_process_host.h:364: void PostTaskWhenProcessIsReady(base::OnceClosure task); I bet we could implement RenderViewHostImpl::PostRenderViewReady() in terms of this, and delete some code out of RenderViewHostImpl.
The CQ bit was checked by lukasza@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
The CQ bit was checked by lukasza@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
nick@, can you take another look please? The most important change (I think) is that in the most recent patchset I restored the DCHECK that CreateTaskForFrame is only called when the frame doesn't yet have a task - restoring the DCHECK was made possible by a combination of 1) ignoring RenderFrameCreated for main frame (because RenderFrameHostChanged will always cover main frame - there is no way to reload a crashed main frame without hitting it) and 2) checking if a task already exists in RenderFrameHostChanged. This seems okay - the desirable invariant/DCHECK is preserved (and all tests pass). I guess we could also consider an alternative approach - deleting the DCHECK and making CreateTaskForFrame idempotent - when called twice with the same frame it wouldn't delete the old task manager's task, but would instead update it. I don't think we need to venture in this direction though - the approach in the most recent patchset also works fine I think. WDYT? https://codereview.chromium.org/2857263003/diff/40001/chrome/browser/task_man... File chrome/browser/task_manager/providers/web_contents/web_contents_task_provider.cc (right): https://codereview.chromium.org/2857263003/diff/40001/chrome/browser/task_man... chrome/browser/task_manager/providers/web_contents/web_contents_task_provider.cc:158: render_frame_host->GetProcess()->PostTaskWhenProcessIsReady(base::Bind( On 2017/05/12 21:56:02, ncarter wrote: > When we show the task manager, this means we will post 1 task per RFH, > browser-wide. That seems like a lot of tasks, especially considering most RFH's > are going to be same-process with their parent. In the most recent patchset, I am returning early for: - non-subframes (which will always be processed in RenderFrameHostChanged; I've verified that reloading a killed main frame correctly re-shows the entry in task manager) - subframes that are in the same instance as their parent Do you think that should help sufficiently? https://codereview.chromium.org/2857263003/diff/40001/chrome/browser/task_man... chrome/browser/task_manager/providers/web_contents/web_contents_task_provider.cc:168: if (render_frame_host) On 2017/05/12 21:56:02, ncarter wrote: > This would need to be: > > render_frame_host && render_frame_host->IsRenderFrameLive() && > /* something to rule out this being a pending delete host */ RE: IsRenderFrameLive Wouldn't this be redundant with |!site_instance->GetProcess()->HasConnection()| in WebContentsEntry::CreateTaskForFrame? RE: pending delete Is this really necessary, if we delete the task manager's entry upon RenderFrameDeleted? I think it is okay if there are temporarily 2 entries for the given frame during a swap-out. https://codereview.chromium.org/2857263003/diff/40001/chrome/browser/task_man... chrome/browser/task_manager/providers/web_contents/web_contents_task_provider.cc:238: void WebContentsEntry::CreateTaskForFrame(RenderFrameHost* render_frame_host) { On 2017/05/12 21:56:02, ncarter wrote: > This function is implemented assuming that |render_frame_host|'s ancestors have > already been processed (see the subsequent comments for some subtle implications > of that. > > So I'm wondering if we can preserve that invariant by making the enumeration > work like this: > > - Remove the ForEachFrame() in CreateAllTasks, and make CreateTaskForFrame be > responsible for adding children (most of the time, |render_frame_host| will not > have children at this point). > > - At the bottom of CreateTaskForFrame, recursively call CreateTaskForFrame on > all children. (might require a content/public change, but > RenderFrameHost::GetChildren() or ForEachChild() would not be unwelcome, IMO). > > - At the top of CreateTaskForFrame, do the following "filter" and "defer" > operations: > > if (!render_frame_host_->IsRenderFrameLive()) > return; // Sad tab; ignore. > > if (render_frame_host_->GetParent() not in tasks_by_frames_) { > return; // do nothing, we're waiting on an ancestor. > > if (!render_frame_host_->GetProcess()->IsReady()) { > render_frame_host->GetProcess()->PostTaskWhenProcessIsReady(); > pending_frames_.push_back(render_frame_host); // maybe > return; // Defer this subtree until we get a PID. > } > > - Figure out what, if anything, |pending_frames_| above ought to be. > It could potentially be something like a map<RenderFrameHost, > WeakPtrFactory<RenderFrameHost>>, if we wanted to support cancelling all > outstanding callbacks; but a manually curated set would probably work well too. > My gut sense is that we need something to support cancellation when RFH's leave > the current slot in their FTN -- especially the ClearTaskForFrame(old_host) that > happens in RenderFrameHostChanged. I am not sure if this is still needed? https://codereview.chromium.org/2857263003/diff/40001/chrome/browser/task_man... chrome/browser/task_manager/providers/web_contents/web_contents_task_provider.cc:240: ClearTaskForFrame(render_frame_host); On 2017/05/12 21:56:02, ncarter wrote: > When does this happen? This would happen when CreateTaskForFrame would be called from both RenderFrameReady and RenderFrameHostChanged. See also https://codereview.chromium.org/2857263003/diff/20001/chrome/browser/task_man... and https://codereview.chromium.org/2857263003/diff/20001/chrome/browser/task_man.... In the most recent patchset, I've moved the ContainsKey check to RenderFrameHostChanged - this way I can restore the DCHECK here. > If |render_frame_host| is the main frame, this cleanup is unsafe, because > existing subframe tasks hold onto a reference to the task of the main frame > https://cs.chromium.org/chromium/src/chrome/browser/task_manager/providers/we... > > In the edit history of the task manager, we've generally tried to avoid the > "deleting and re-add, just to be sure" pattern, since it turns out to not > usually be a total no-op. Making this change causes the window to repaint, and > can cause UI state (like the row selection) to be lost, and it regenerates the > ID, which can cause rows to jump around in the order. > > So, I'd prefer to keep these DCHECKs if possible. Makes sense. https://codereview.chromium.org/2857263003/diff/40001/chrome/browser/task_man... chrome/browser/task_manager/providers/web_contents/web_contents_task_provider.cc:243: if (!site_instance->GetProcess()->HasConnection()) On 2017/05/12 21:56:02, ncarter wrote: > Should probably be IsReady() Changing this makes PrintPreviewDialogControllerBrowserTest.TaskManagementTest fail, by failing to create tasks for chrome://print contents - in this case I see the following in RenderProcessHostImpl: IsReady -> returning false; HasConnection() -> returning true; channel_connected_ = 0; GetHandle() = 0; is_initialized_ = 1; is_dead_ = 0 https://codereview.chromium.org/2857263003/diff/40001/content/browser/rendere... File content/browser/renderer_host/render_process_host_impl.cc (right): https://codereview.chromium.org/2857263003/diff/40001/content/browser/rendere... content/browser/renderer_host/render_process_host_impl.cc:552: delete this; On 2017/05/12 21:56:02, ncarter wrote: > I'd probably elect to |delete this| unconditionally here, if we get here and the > process has died already. Otherwise the Observer could live on until the user > reloads the page again, at which point the caller would probably queue up > another PostTaskWhenProcessIsReady. Done. I think in the scenario you point out RenderProcessHostDestroyed would be called. OTOH, I see that if a process is not ready in CallTask method, then it will never become ready and there is no point in keeping the observer alive. https://codereview.chromium.org/2857263003/diff/40001/content/public/browser/... File content/public/browser/render_process_host.h (right): https://codereview.chromium.org/2857263003/diff/40001/content/public/browser/... content/public/browser/render_process_host.h:364: void PostTaskWhenProcessIsReady(base::OnceClosure task); On 2017/05/12 21:56:02, ncarter wrote: > I bet we could implement RenderViewHostImpl::PostRenderViewReady() in terms of > this, and delete some code out of RenderViewHostImpl. Good idea. Done.
The CQ bit was checked by lukasza@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from ========== Task Manager should listen to WebContentsObserver::RenderFrameCreated. Listening to WebContentsObserver::RenderFrameCreated is the only way to realize that a RenderFrameHost got recreated after a render process crash - Task Manager needs to listen to this callback to re-show frames that were re-navigated after a render process crash. A slight problem with this approach is that the PID of the render process is not necessarily known when RenderFrameCreated is called - to work around that Task Manager postpones processing of the new frame via PostTaskWhenProcessIsReady (an instance method added to RenderProcessHost in this CL). BUG=642958 ========== to ========== Task Manager should listen to WebContentsObserver::RenderFrameCreated. Listening to WebContentsObserver::RenderFrameCreated is the only way to realize that a RenderFrameHost got recreated after a render process crash - Task Manager needs to listen to this callback to re-show frames that were re-navigated after a render process crash. A slight problem with this approach is that the PID of the render process is not necessarily known when RenderFrameCreated is called - to work around that Task Manager postpones processing of the new frame via PostTaskWhenProcessIsReady (an instance method added to RenderProcessHost in this CL). BUG=642958 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation ==========
The CQ bit was checked by lukasza@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
lgtm I do think the recursive exploration approach I mentioned suggested would fix some other corner case bugs, such as: the TaskManager is shown at the exact moment we have a RFH that's current but not yet visible, or if the IsReady signal is seriously delayed for an OOPIF. However, those cases are orthogonal to the bugfix you're after here, and this change seems to make things strictly better. https://codereview.chromium.org/2857263003/diff/120001/chrome/browser/task_ma... File chrome/browser/task_manager/providers/web_contents/web_contents_task_provider.cc (right): https://codereview.chromium.org/2857263003/diff/120001/chrome/browser/task_ma... chrome/browser/task_manager/providers/web_contents/web_contents_task_provider.cc:156: // Skip hosts that will be processed anyway by RenderFrameHostChanged. // Skip pending/speculative hosts. We'll create tasks for these if the navigation commits, at which point RenderFrameHostChanged() will fire. https://codereview.chromium.org/2857263003/diff/120001/content/public/browser... File content/public/browser/render_frame_host.h (right): https://codereview.chromium.org/2857263003/diff/120001/content/public/browser... content/public/browser/render_frame_host.h:202: // of swapping out another RenderFrameHost, until the swap actually happens. // Returns true if this is the currently-visible RenderFrameHost for our frame tree node. During process transfer, a RenderFrameHost may be created that is not current. After process transfer, the old RenderFrameHost becomes non-current until it is deleted (which may not happen until its unload handler runs). // // Changes to the IsCurrent() state of a RenderFrameHost may be observed via WebContentsObserver::RenderFrameHostChanged().
On 2017/05/16 20:50:57, ncarter (slow) wrote: > lgtm > > I do think the recursive exploration approach I mentioned suggested would fix > some other corner case bugs, such as: the TaskManager is shown at the exact > moment we have a RFH that's current but not yet visible, or if the IsReady > signal is seriously delayed for an OOPIF. > > However, those cases are orthogonal to the bugfix you're after here, and this > change seems to make things strictly better. > > https://codereview.chromium.org/2857263003/diff/120001/chrome/browser/task_ma... > File > chrome/browser/task_manager/providers/web_contents/web_contents_task_provider.cc > (right): > > https://codereview.chromium.org/2857263003/diff/120001/chrome/browser/task_ma... > chrome/browser/task_manager/providers/web_contents/web_contents_task_provider.cc:156: > // Skip hosts that will be processed anyway by RenderFrameHostChanged. > // Skip pending/speculative hosts. We'll create tasks for these if the > navigation commits, at which point RenderFrameHostChanged() will fire. > > https://codereview.chromium.org/2857263003/diff/120001/content/public/browser... > File content/public/browser/render_frame_host.h (right): > > https://codereview.chromium.org/2857263003/diff/120001/content/public/browser... > content/public/browser/render_frame_host.h:202: // of swapping out another > RenderFrameHost, until the swap actually happens. > // Returns true if this is the currently-visible RenderFrameHost for our frame > tree node. During process transfer, a RenderFrameHost may be created that is not > current. After process transfer, the old RenderFrameHost becomes non-current > until it is deleted (which may not happen until its unload handler runs). > // > // Changes to the IsCurrent() state of a RenderFrameHost may be observed via > WebContentsObserver::RenderFrameHostChanged(). "current but not yet visible" in my previous message ought to have read "current but not yet IsReady"
The CQ bit was checked by lukasza@chromium.org to run a CQ dry run
Thanks. I'll try to do the WebContentsObserver_FrameObserverAdapter idea in a follow-up CL (starting with adapting/wrapping a single WebContentsObserver and maybe in another follow-up going down to a single WCO). https://codereview.chromium.org/2857263003/diff/120001/chrome/browser/task_ma... File chrome/browser/task_manager/providers/web_contents/web_contents_task_provider.cc (right): https://codereview.chromium.org/2857263003/diff/120001/chrome/browser/task_ma... chrome/browser/task_manager/providers/web_contents/web_contents_task_provider.cc:156: // Skip hosts that will be processed anyway by RenderFrameHostChanged. On 2017/05/16 20:50:57, ncarter (slow) wrote: > // Skip pending/speculative hosts. We'll create tasks for these if the > navigation commits, at which point RenderFrameHostChanged() will fire. Done. https://codereview.chromium.org/2857263003/diff/120001/content/public/browser... File content/public/browser/render_frame_host.h (right): https://codereview.chromium.org/2857263003/diff/120001/content/public/browser... content/public/browser/render_frame_host.h:202: // of swapping out another RenderFrameHost, until the swap actually happens. On 2017/05/16 20:50:57, ncarter (slow) wrote: > // Returns true if this is the currently-visible RenderFrameHost for our frame > tree node. During process transfer, a RenderFrameHost may be created that is not > current. After process transfer, the old RenderFrameHost becomes non-current > until it is deleted (which may not happen until its unload handler runs). > // > // Changes to the IsCurrent() state of a RenderFrameHost may be observed via > WebContentsObserver::RenderFrameHostChanged(). Done.
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by lukasza@chromium.org
The CQ bit was checked by lukasza@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from nick@chromium.org Link to the patchset: https://codereview.chromium.org/2857263003/#ps140001 (title: "Tweaked the comments as suggested in the CR feedback")
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 140001, "attempt_start_ts": 1494970460755620,
"parent_rev": "afa32648e767d5288c0ea85ca18526ac8810fe0d", "commit_rev":
"f2c4f1f94d10b019363f2d8c45defe00881cb5cb"}
Message was sent while issue was closed.
Description was changed from ========== Task Manager should listen to WebContentsObserver::RenderFrameCreated. Listening to WebContentsObserver::RenderFrameCreated is the only way to realize that a RenderFrameHost got recreated after a render process crash - Task Manager needs to listen to this callback to re-show frames that were re-navigated after a render process crash. A slight problem with this approach is that the PID of the render process is not necessarily known when RenderFrameCreated is called - to work around that Task Manager postpones processing of the new frame via PostTaskWhenProcessIsReady (an instance method added to RenderProcessHost in this CL). BUG=642958 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation ========== to ========== Task Manager should listen to WebContentsObserver::RenderFrameCreated. Listening to WebContentsObserver::RenderFrameCreated is the only way to realize that a RenderFrameHost got recreated after a render process crash - Task Manager needs to listen to this callback to re-show frames that were re-navigated after a render process crash. A slight problem with this approach is that the PID of the render process is not necessarily known when RenderFrameCreated is called - to work around that Task Manager postpones processing of the new frame via PostTaskWhenProcessIsReady (an instance method added to RenderProcessHost in this CL). BUG=642958 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation Review-Url: https://codereview.chromium.org/2857263003 Cr-Commit-Position: refs/heads/master@{#472325} Committed: https://chromium.googlesource.com/chromium/src/+/f2c4f1f94d10b019363f2d8c45de... ==========
Message was sent while issue was closed.
Committed patchset #8 (id:140001) as https://chromium.googlesource.com/chromium/src/+/f2c4f1f94d10b019363f2d8c45de... |
