Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2961423002:
[TaskManager] Allow a Task to mutate its PID after creation (Closed)
|
Created:
3 years, 5 months ago by ncarter (slow) Modified:
3 years, 4 months ago CC:
chromium-reviews, jam, nasko+codewatch_chromium.org, darin-cc_chromium.org, creis+watch_chromium.org Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
Description[TaskManager] Allow a Task to mutate its PID after creation
Prior to this change, WebContentsTaskProvider would defer task creation if the
PID was not known at RenderFrameCreated time. However, for a newly-cloned
WebContents, no event ever occurred to re-create the Task once the process
finished launching. This was bug 738169.
Fix the problem by never deferring task_manager::Task creation; instead, create
Tasks for all live, current RenderFrameHosts. If the PID is not known immediately,
allow it to be set on the task at a later time (when RenderProcessHost::IsReady()
becomes true).
Because the TaskManagerImpl was written with the expectation that Tasks have
a constant PID, PID mutation is implemented in terms of temporarily removing the
task, mutating the PID, and re-adding it.
In WebContentsTaskProvider, we no longer need to implement RenderProcessGone.
In TaskGroup, don't waste time collecting stats for a null process ID.
Tests are added exercising this bug, and its subframe equivalent. Test coverage
is added for the PID==0 case, and we ensure that the "Memory" column is
populated with something nonzero.
BUG=738169
Review-Url: https://codereview.chromium.org/2961423002
Cr-Commit-Position: refs/heads/master@{#491180}
Committed: https://chromium.googlesource.com/chromium/src/+/6c760df2f6a9f5d1ce7051be6acfeebc26919774
Patch Set 1 #Patch Set 2 : Totally change approach; just let Tasks modify their PIDs. #
Total comments: 20
Patch Set 3 : afakhry's fixes #
Total comments: 2
Patch Set 4 : Rework comment. #Messages
Total messages: 42 (33 generated)
Description was changed from ========== TaskManager: Defer task creation until PID is known, even for main frames. BUG=738169 ========== to ========== TaskManager: Defer task creation until PID is known, even for main frames. BUG=738169 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation ==========
Description was changed from ========== TaskManager: Defer task creation until PID is known, even for main frames. BUG=738169 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation ========== to ========== TaskManager: Always defer task creation until PID is known. Prior to this change, we would defer task creation if the PID was not known at RenderFrameCreated time, but for a newly-created WebContents, such deferrals didn't happen. Bug 738169 describes a case where this matters. In this fix, we defer task creation until all the following conditions are met: - Tasks creation is complete for the frame's parent. - The frame is the current RFH of its FrameTreeNode (IsCurrent()) - The frame is live (IsRenderFrameLive()) - The process ID is known (RenderProcessHost::IsReady()). When the first three conditions are met, the frame is considered 'tracked' by the WebContentsTaskProvider; but if the process ID is not yet known, it goes into a deferral queue |tracked_frames_without_tasks_|. Tests are added exercising this bug, and its subframe equivalent. Test coverage is added for the PID==0 case, and we ensure that the "Memory" column is populated with something nonzero. BUG=738169 ==========
Description was changed from ========== TaskManager: Always defer task creation until PID is known. Prior to this change, we would defer task creation if the PID was not known at RenderFrameCreated time, but for a newly-created WebContents, such deferrals didn't happen. Bug 738169 describes a case where this matters. In this fix, we defer task creation until all the following conditions are met: - Tasks creation is complete for the frame's parent. - The frame is the current RFH of its FrameTreeNode (IsCurrent()) - The frame is live (IsRenderFrameLive()) - The process ID is known (RenderProcessHost::IsReady()). When the first three conditions are met, the frame is considered 'tracked' by the WebContentsTaskProvider; but if the process ID is not yet known, it goes into a deferral queue |tracked_frames_without_tasks_|. Tests are added exercising this bug, and its subframe equivalent. Test coverage is added for the PID==0 case, and we ensure that the "Memory" column is populated with something nonzero. BUG=738169 ========== to ========== TaskManager: Always defer task creation until PID is known. Prior to this change, we would defer task creation if the PID was not known at RenderFrameCreated time, but for a newly-created WebContents, such deferrals didn't happen. Bug 738169 describes a case where this matters. In this fix, we defer task creation until all the following conditions are met: - Task creation is complete for the frame's parent. - The frame is the current RFH of its FrameTreeNode (IsCurrent()) - The frame is live (IsRenderFrameLive()) - The process ID is known (RenderProcessHost::IsReady()). When the first three conditions are met, the frame is considered 'tracked' by the WebContentsTaskProvider; but if the process ID is not yet known, it goes into a deferral queue |tracked_frames_without_tasks_|. Tests are added exercising this bug, and its subframe equivalent. Test coverage is added for the PID==0 case, and we ensure that the "Memory" column is populated with something nonzero. BUG=738169 ==========
Description was changed from ========== TaskManager: Always defer task creation until PID is known. Prior to this change, we would defer task creation if the PID was not known at RenderFrameCreated time, but for a newly-created WebContents, such deferrals didn't happen. Bug 738169 describes a case where this matters. In this fix, we defer task creation until all the following conditions are met: - Task creation is complete for the frame's parent. - The frame is the current RFH of its FrameTreeNode (IsCurrent()) - The frame is live (IsRenderFrameLive()) - The process ID is known (RenderProcessHost::IsReady()). When the first three conditions are met, the frame is considered 'tracked' by the WebContentsTaskProvider; but if the process ID is not yet known, it goes into a deferral queue |tracked_frames_without_tasks_|. Tests are added exercising this bug, and its subframe equivalent. Test coverage is added for the PID==0 case, and we ensure that the "Memory" column is populated with something nonzero. BUG=738169 ========== to ========== TaskManager: Always defer task creation until PID is known for self & ancestors Prior to this change, we would defer task creation if the PID was not known at RenderFrameCreated time, but for a newly-created WebContents, such deferrals didn't happen. Bug 738169 describes a case where this matters. In this fix, we defer task creation until all the following conditions are met: - Task creation is complete for the frame's parent. - The frame is the current RFH of its FrameTreeNode (IsCurrent()) - The frame is live (IsRenderFrameLive()) - The process ID is known (RenderProcessHost::IsReady()). When the first three conditions are met, the frame is considered 'tracked' by the WebContentsTaskProvider; but if the process ID is not yet known, it goes into a deferral queue |tracked_frames_without_tasks_|. This approach needs to gradually walk the frame tree from top to bottom; so expose RenderFrameHost::GetChildAt() to content/public. Generally, we rely on WebContentsObserver's tree-order semantics (children are RenderFrameDeleted before their parents are) for correctness here, but the RenderProcessReady signal is theoretically uncorrelated with all of that. Tests are added exercising this bug, and its subframe equivalent. Test coverage is added for the PID==0 case, and we ensure that the "Memory" column is populated with something nonzero. BUG=738169 ==========
Description was changed from ========== TaskManager: Always defer task creation until PID is known for self & ancestors Prior to this change, we would defer task creation if the PID was not known at RenderFrameCreated time, but for a newly-created WebContents, such deferrals didn't happen. Bug 738169 describes a case where this matters. In this fix, we defer task creation until all the following conditions are met: - Task creation is complete for the frame's parent. - The frame is the current RFH of its FrameTreeNode (IsCurrent()) - The frame is live (IsRenderFrameLive()) - The process ID is known (RenderProcessHost::IsReady()). When the first three conditions are met, the frame is considered 'tracked' by the WebContentsTaskProvider; but if the process ID is not yet known, it goes into a deferral queue |tracked_frames_without_tasks_|. This approach needs to gradually walk the frame tree from top to bottom; so expose RenderFrameHost::GetChildAt() to content/public. Generally, we rely on WebContentsObserver's tree-order semantics (children are RenderFrameDeleted before their parents are) for correctness here, but the RenderProcessReady signal is theoretically uncorrelated with all of that. Tests are added exercising this bug, and its subframe equivalent. Test coverage is added for the PID==0 case, and we ensure that the "Memory" column is populated with something nonzero. BUG=738169 ========== to ========== TaskManager: Always defer task creation until PID is known Prior to this change, we would defer task creation if the PID was not known at RenderFrameCreated time, but for a newly-created WebContents, such deferrals didn't happen. Bug 738169 describes a case where this matters. In this fix, we defer task creation until all the following conditions are met: - Task creation is complete for the frame's parent. - The frame is the current RFH of its FrameTreeNode (IsCurrent()) - The frame is live (IsRenderFrameLive()) - The process ID is known (RenderProcessHost::IsReady()). When the first three conditions are met, the frame is considered 'tracked' by the WebContentsTaskProvider; but if the process ID is not yet known, it goes into a deferral queue |tracked_frames_without_tasks_|. This approach needs to gradually walk the frame tree from top to bottom; so expose RenderFrameHost::GetChildAt() to content/public. Generally, we rely on WebContentsObserver's tree-order semantics (children are RenderFrameDeleted before their parents are) for correctness here, but the RenderProcessReady signal is theoretically uncorrelated with all of that. Tests are added exercising this bug, and its subframe equivalent. Test coverage is added for the PID==0 case, and we ensure that the "Memory" column is populated with something nonzero. BUG=738169 ==========
Description was changed from ========== TaskManager: Always defer task creation until PID is known Prior to this change, we would defer task creation if the PID was not known at RenderFrameCreated time, but for a newly-created WebContents, such deferrals didn't happen. Bug 738169 describes a case where this matters. In this fix, we defer task creation until all the following conditions are met: - Task creation is complete for the frame's parent. - The frame is the current RFH of its FrameTreeNode (IsCurrent()) - The frame is live (IsRenderFrameLive()) - The process ID is known (RenderProcessHost::IsReady()). When the first three conditions are met, the frame is considered 'tracked' by the WebContentsTaskProvider; but if the process ID is not yet known, it goes into a deferral queue |tracked_frames_without_tasks_|. This approach needs to gradually walk the frame tree from top to bottom; so expose RenderFrameHost::GetChildAt() to content/public. Generally, we rely on WebContentsObserver's tree-order semantics (children are RenderFrameDeleted before their parents are) for correctness here, but the RenderProcessReady signal is theoretically uncorrelated with all of that. Tests are added exercising this bug, and its subframe equivalent. Test coverage is added for the PID==0 case, and we ensure that the "Memory" column is populated with something nonzero. BUG=738169 ========== to ========== TaskManager: Always defer task creation until PID is known Prior to this change, we would defer task creation if the PID was not known at RenderFrameCreated time, but for a newly-cloned WebContents, such deferrals didn't happen. Bug 738169 describes a case where this matters. In this fix, we defer task creation until all the following conditions are met: - Task creation is complete for the frame's parent. - The frame is the current RFH of its FrameTreeNode (IsCurrent()) - The frame is live (IsRenderFrameLive()) - The process ID is known (RenderProcessHost::IsReady()). When the first three conditions are met, the frame is considered 'tracked' by the WebContentsTaskProvider; but if the process ID is not yet known, it goes into a deferral queue |tracked_frames_without_tasks_|. This approach needs to gradually walk the frame tree from top to bottom; so expose RenderFrameHost::GetChildAt() to content/public. Generally, we rely on WebContentsObserver's tree-order semantics (children are RenderFrameDeleted before their parents are) for correctness here, but the RenderProcessReady signal is theoretically uncorrelated with all of that. Tests are added exercising this bug, and its subframe equivalent. Test coverage is added for the PID==0 case, and we ensure that the "Memory" column is populated with something nonzero. BUG=738169 ==========
Description was changed from ========== TaskManager: Always defer task creation until PID is known Prior to this change, we would defer task creation if the PID was not known at RenderFrameCreated time, but for a newly-cloned WebContents, such deferrals didn't happen. Bug 738169 describes a case where this matters. In this fix, we defer task creation until all the following conditions are met: - Task creation is complete for the frame's parent. - The frame is the current RFH of its FrameTreeNode (IsCurrent()) - The frame is live (IsRenderFrameLive()) - The process ID is known (RenderProcessHost::IsReady()). When the first three conditions are met, the frame is considered 'tracked' by the WebContentsTaskProvider; but if the process ID is not yet known, it goes into a deferral queue |tracked_frames_without_tasks_|. This approach needs to gradually walk the frame tree from top to bottom; so expose RenderFrameHost::GetChildAt() to content/public. Generally, we rely on WebContentsObserver's tree-order semantics (children are RenderFrameDeleted before their parents are) for correctness here, but the RenderProcessReady signal is theoretically uncorrelated with all of that. Tests are added exercising this bug, and its subframe equivalent. Test coverage is added for the PID==0 case, and we ensure that the "Memory" column is populated with something nonzero. BUG=738169 ========== to ========== TaskManager: Always defer task creation until PID is known Prior to this change, we would defer task creation if the PID was not known at RenderFrameCreated time, but for a newly-cloned WebContents, such deferrals didn't happen. Bug 738169 describes a case where this matters. In this fix, we defer task creation until all the following conditions are met: - Task creation is complete for the frame's parent. - The frame is the current RFH of its FrameTreeNode (IsCurrent()) - The frame is live (IsRenderFrameLive()) - The process ID is known (RenderProcessHost::IsReady()). When the first three conditions are met, the frame is considered 'tracked' by the WebContentsTaskProvider; but if the process ID is not yet known, it goes into a deferral queue |tracked_frames_without_tasks_|. This approach needs to gradually walk the frame tree from top to bottom; so expose RenderFrameHost::GetChildAt() to content/public. Generally, we rely on WebContentsObserver's tree-order semantics (children are RenderFrameDeleted before their parents are) for correctness here, but the RenderProcessReady signal is independent of all of that. Tests are added exercising this bug, and its subframe equivalent. Test coverage is added for the PID==0 case, and we ensure that the "Memory" column is populated with something nonzero. BUG=738169 ==========
The CQ bit was checked by nick@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 nick@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 ========== TaskManager: Always defer task creation until PID is known Prior to this change, we would defer task creation if the PID was not known at RenderFrameCreated time, but for a newly-cloned WebContents, such deferrals didn't happen. Bug 738169 describes a case where this matters. In this fix, we defer task creation until all the following conditions are met: - Task creation is complete for the frame's parent. - The frame is the current RFH of its FrameTreeNode (IsCurrent()) - The frame is live (IsRenderFrameLive()) - The process ID is known (RenderProcessHost::IsReady()). When the first three conditions are met, the frame is considered 'tracked' by the WebContentsTaskProvider; but if the process ID is not yet known, it goes into a deferral queue |tracked_frames_without_tasks_|. This approach needs to gradually walk the frame tree from top to bottom; so expose RenderFrameHost::GetChildAt() to content/public. Generally, we rely on WebContentsObserver's tree-order semantics (children are RenderFrameDeleted before their parents are) for correctness here, but the RenderProcessReady signal is independent of all of that. Tests are added exercising this bug, and its subframe equivalent. Test coverage is added for the PID==0 case, and we ensure that the "Memory" column is populated with something nonzero. BUG=738169 ========== to ========== TaskManager: Support creating Tasks before their process ID is known Prior to this change, we would defer task creation if the PID was not known at RenderFrameCreated time. However, for a newly-cloned WebContents, no event ever occurred to re-create the row once the process finished launching. Bug 738169 describes repro steps where this mattered. Fix bug 738169 by never deferring task_manager::Task creation; instead, create Tasks for all live, current RenderFrameHosts, and allow a Task's process_id to be updated at a later time. Because the TaskManager expects Tasks to have a constant process_id, we implement this in terms of temporarily removing the task, mutating its process id, and re-adding it. We no longer need to special-case RenderProcessGone. Tests are added exercising this bug, and its subframe equivalent. Test coverage is added for the PID==0 case, and we ensure that the "Memory" column is populated with something nonzero. BUG=738169 ==========
Description was changed from ========== TaskManager: Support creating Tasks before their process ID is known Prior to this change, we would defer task creation if the PID was not known at RenderFrameCreated time. However, for a newly-cloned WebContents, no event ever occurred to re-create the row once the process finished launching. Bug 738169 describes repro steps where this mattered. Fix bug 738169 by never deferring task_manager::Task creation; instead, create Tasks for all live, current RenderFrameHosts, and allow a Task's process_id to be updated at a later time. Because the TaskManager expects Tasks to have a constant process_id, we implement this in terms of temporarily removing the task, mutating its process id, and re-adding it. We no longer need to special-case RenderProcessGone. Tests are added exercising this bug, and its subframe equivalent. Test coverage is added for the PID==0 case, and we ensure that the "Memory" column is populated with something nonzero. BUG=738169 ========== to ========== TaskManager: Support creating Tasks before PID is known Prior to this change, we would defer task creation if the PID was not known at RenderFrameCreated time. However, for a newly-cloned WebContents, no event ever occurred to re-create the row once the process finished launching. Bug 738169 describes repro steps where this mattered. Fix bug 738169 by never deferring task_manager::Task creation; instead, create Tasks for all live, current RenderFrameHosts, and allow a Task's process_id to be updated at a later time. Because the TaskManager expects Tasks to have a constant process_id, we implement this in terms of temporarily removing the task, mutating its process id, and re-adding it. We no longer need to special-case RenderProcessGone. Tests are added exercising this bug, and its subframe equivalent. Test coverage is added for the PID==0 case, and we ensure that the "Memory" column is populated with something nonzero. BUG=738169 ==========
Description was changed from ========== TaskManager: Support creating Tasks before PID is known Prior to this change, we would defer task creation if the PID was not known at RenderFrameCreated time. However, for a newly-cloned WebContents, no event ever occurred to re-create the row once the process finished launching. Bug 738169 describes repro steps where this mattered. Fix bug 738169 by never deferring task_manager::Task creation; instead, create Tasks for all live, current RenderFrameHosts, and allow a Task's process_id to be updated at a later time. Because the TaskManager expects Tasks to have a constant process_id, we implement this in terms of temporarily removing the task, mutating its process id, and re-adding it. We no longer need to special-case RenderProcessGone. Tests are added exercising this bug, and its subframe equivalent. Test coverage is added for the PID==0 case, and we ensure that the "Memory" column is populated with something nonzero. BUG=738169 ========== to ========== TaskManager: Support creating Tasks before PID is known Prior to this change, we would defer task creation if the PID was not known at RenderFrameCreated time. However, for a newly-cloned WebContents, no event ever occurred to re-create the row once the process finished launching. Bug 738169 describes repro steps where this mattered. Fix bug 738169 by never deferring task_manager::Task creation; instead, create Tasks for all live, current RenderFrameHosts, and allow a Task's process_id to be updated at a later time. Because the TaskManager expects Tasks to have a constant process_id, we implement this in terms of temporarily removing the task, mutating its process id, and re-adding it. In WebContentsTaskProvider, we no longer need to special-case RenderProcessGone. In TaskGroup, don't waste time collecting stats for a null process ID. Tests are added exercising this bug, and its subframe equivalent. Test coverage is added for the PID==0 case, and we ensure that the "Memory" column is populated with something nonzero. BUG=738169 ==========
Description was changed from ========== TaskManager: Support creating Tasks before PID is known Prior to this change, we would defer task creation if the PID was not known at RenderFrameCreated time. However, for a newly-cloned WebContents, no event ever occurred to re-create the row once the process finished launching. Bug 738169 describes repro steps where this mattered. Fix bug 738169 by never deferring task_manager::Task creation; instead, create Tasks for all live, current RenderFrameHosts, and allow a Task's process_id to be updated at a later time. Because the TaskManager expects Tasks to have a constant process_id, we implement this in terms of temporarily removing the task, mutating its process id, and re-adding it. In WebContentsTaskProvider, we no longer need to special-case RenderProcessGone. In TaskGroup, don't waste time collecting stats for a null process ID. Tests are added exercising this bug, and its subframe equivalent. Test coverage is added for the PID==0 case, and we ensure that the "Memory" column is populated with something nonzero. BUG=738169 ========== to ========== TaskManager: Support creating Tasks before PID is known Prior to this change, we would defer task creation if the PID was not known at RenderFrameCreated time. However, for a newly-cloned WebContents, no event ever occurred to re-create the Task once the process finished launching. This was bug 738169. Fix the problem by never deferring task_manager::Task creation; instead, create Tasks for all live, current RenderFrameHosts, and allow a Task's process_id to be updated at a later time. Because the TaskManager expects Tasks to have a constant process_id, we implement this in terms of temporarily removing the task, mutating its process id, and re-adding it. In WebContentsTaskProvider, we no longer need to special-case RenderProcessGone. In TaskGroup, don't waste time collecting stats for a null process ID. Tests are added exercising this bug, and its subframe equivalent. Test coverage is added for the PID==0 case, and we ensure that the "Memory" column is populated with something nonzero. BUG=738169 ==========
Description was changed from ========== TaskManager: Support creating Tasks before PID is known Prior to this change, we would defer task creation if the PID was not known at RenderFrameCreated time. However, for a newly-cloned WebContents, no event ever occurred to re-create the Task once the process finished launching. This was bug 738169. Fix the problem by never deferring task_manager::Task creation; instead, create Tasks for all live, current RenderFrameHosts, and allow a Task's process_id to be updated at a later time. Because the TaskManager expects Tasks to have a constant process_id, we implement this in terms of temporarily removing the task, mutating its process id, and re-adding it. In WebContentsTaskProvider, we no longer need to special-case RenderProcessGone. In TaskGroup, don't waste time collecting stats for a null process ID. Tests are added exercising this bug, and its subframe equivalent. Test coverage is added for the PID==0 case, and we ensure that the "Memory" column is populated with something nonzero. BUG=738169 ========== to ========== TaskManager: Support creating Tasks before PID is known Prior to this change, we would defer task creation if the PID was not known at RenderFrameCreated time. However, for a newly-cloned WebContents, no event ever occurred to re-create the Task once the process finished launching. This was bug 738169. Fix the problem by never deferring task_manager::Task creation; instead, create Tasks for all live, current RenderFrameHosts, and allow a Task's process_id to be updated at a later time. Because the TaskManager expects Tasks to have a constant process_id, we implement this in terms of temporarily removing the task, mutating its process id, and re-adding it. In WebContentsTaskProvider, we no longer need to special-case RenderProcessGone. In TaskGroup, don't waste time collecting stats for a null process ID. Tests are added exercising this bug, and its subframe equivalent. Test coverage is added for the PID==0 case, and we ensure that the "Memory" column is populated with something nonzero. BUG=738169 ==========
Description was changed from ========== TaskManager: Support creating Tasks before PID is known Prior to this change, we would defer task creation if the PID was not known at RenderFrameCreated time. However, for a newly-cloned WebContents, no event ever occurred to re-create the Task once the process finished launching. This was bug 738169. Fix the problem by never deferring task_manager::Task creation; instead, create Tasks for all live, current RenderFrameHosts, and allow a Task's process_id to be updated at a later time. Because the TaskManager expects Tasks to have a constant process_id, we implement this in terms of temporarily removing the task, mutating its process id, and re-adding it. In WebContentsTaskProvider, we no longer need to special-case RenderProcessGone. In TaskGroup, don't waste time collecting stats for a null process ID. Tests are added exercising this bug, and its subframe equivalent. Test coverage is added for the PID==0 case, and we ensure that the "Memory" column is populated with something nonzero. BUG=738169 ========== to ========== [TaskManager] Allow a Task to mutate its PID after creation Prior to this change, we would defer task creation if the PID was not known at RenderFrameCreated time. However, for a newly-cloned WebContents, no event ever occurred to re-create the Task once the process finished launching. This was bug 738169. Fix the problem by never deferring task_manager::Task creation; instead, create Tasks for all live, current RenderFrameHosts, and allow a Task's process_id to be updated at a later time. Because the TaskManager expects Tasks to have a constant process_id, we implement this in terms of temporarily removing the task, mutating its process id, and re-adding it. In WebContentsTaskProvider, we no longer need to special-case RenderProcessGone. In TaskGroup, don't waste time collecting stats for a null process ID. Tests are added exercising this bug, and its subframe equivalent. Test coverage is added for the PID==0 case, and we ensure that the "Memory" column is populated with something nonzero. BUG=738169 ==========
Description was changed from ========== [TaskManager] Allow a Task to mutate its PID after creation Prior to this change, we would defer task creation if the PID was not known at RenderFrameCreated time. However, for a newly-cloned WebContents, no event ever occurred to re-create the Task once the process finished launching. This was bug 738169. Fix the problem by never deferring task_manager::Task creation; instead, create Tasks for all live, current RenderFrameHosts, and allow a Task's process_id to be updated at a later time. Because the TaskManager expects Tasks to have a constant process_id, we implement this in terms of temporarily removing the task, mutating its process id, and re-adding it. In WebContentsTaskProvider, we no longer need to special-case RenderProcessGone. In TaskGroup, don't waste time collecting stats for a null process ID. Tests are added exercising this bug, and its subframe equivalent. Test coverage is added for the PID==0 case, and we ensure that the "Memory" column is populated with something nonzero. BUG=738169 ========== to ========== [TaskManager] Allow a Task to mutate its PID after creation Prior to this change, WebContentsTaskProvider would defer task creation if the PID was not known at RenderFrameCreated time. However, for a newly-cloned WebContents, no event ever occurred to re-create the Task once the process finished launching. This was bug 738169. Fix the problem by never deferring task_manager::Task creation; instead, create Tasks for all live, current RenderFrameHosts. If the PID is not known immediately, allow it to be set on the task at a later time (when RenderProcessHost::IsReady() becomes true). Because the TaskManagerImpl was written with the expectation that Tasks have a constant PID, PID mutation is implemented in terms of temporarily removing the task, mutating the PID, and re-adding it. In WebContentsTaskProvider, we no longer need to implement RenderProcessGone. In TaskGroup, don't waste time collecting stats for a null process ID. Tests are added exercising this bug, and its subframe equivalent. Test coverage is added for the PID==0 case, and we ensure that the "Memory" column is populated with something nonzero. BUG=738169 ==========
nick@chromium.org changed reviewers: + afakhry@chromium.org, cburn@google.com
Chuck, Ahmed -- let me know what you think
nick@chromium.org changed reviewers: + lukasza@chromium.org
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Just a few nits. Thank you for fixing this issue! :) https://codereview.chromium.org/2961423002/diff/20001/chrome/browser/task_man... File chrome/browser/task_manager/providers/task.cc (right): https://codereview.chromium.org/2961423002/diff/20001/chrome/browser/task_man... chrome/browser/task_manager/providers/task.cc:101: // TaskManagerImpl and TaskGroup implementations assume that a process id is Nit: process id => ditto. https://codereview.chromium.org/2961423002/diff/20001/chrome/browser/task_man... File chrome/browser/task_manager/providers/task.h (right): https://codereview.chromium.org/2961423002/diff/20001/chrome/browser/task_man... chrome/browser/task_manager/providers/task.h:88: // Temporarily remove a task, change its process id, and then re-add it. Nit: For consistency, process id => process ID. https://codereview.chromium.org/2961423002/diff/20001/chrome/browser/task_man... File chrome/browser/task_manager/providers/web_contents/web_contents_task_provider.cc (right): https://codereview.chromium.org/2961423002/diff/20001/chrome/browser/task_man... chrome/browser/task_manager/providers/web_contents/web_contents_task_provider.cc:186: // IsReady state. This might mean we know its the process ID. Nit: This might mean we know its (-the) process ID? https://codereview.chromium.org/2961423002/diff/20001/chrome/browser/task_man... chrome/browser/task_manager/providers/web_contents/web_contents_task_provider.cc:192: if (task) { Nit: Much indented nested blocks? if (!render_frame_host) return; Task* task = GetTaskForFrame(render_frame_host); if (!task) return; https://codereview.chromium.org/2961423002/diff/20001/chrome/browser/task_man... chrome/browser/task_manager/providers/web_contents/web_contents_task_provider.cc:193: base::ProcessId determine_pid_from_handle = base::kNullProcessId; Nit: Can you make this a const? https://codereview.chromium.org/2961423002/diff/20001/chrome/browser/task_man... chrome/browser/task_manager/providers/web_contents/web_contents_task_provider.cc:273: site_instance == render_frame_host->GetParent()->GetSiteInstance()) Nit: braces here. https://codereview.chromium.org/2961423002/diff/20001/chrome/browser/task_man... chrome/browser/task_manager/providers/web_contents/web_contents_task_provider.cc:329: if (new_task->process_id() == base::kNullProcessId) { Nit: Indented nested blocks here too? if (new_task->process_id() != base::kNullProcessId) return; // <Your above comment here> // PostTaskWhenProcessIsReady(...); https://codereview.chromium.org/2961423002/diff/20001/chrome/browser/task_man... File chrome/browser/task_manager/sampling/task_group.cc (right): https://codereview.chromium.org/2961423002/diff/20001/chrome/browser/task_man... chrome/browser/task_manager/sampling/task_group.cc:136: weak_ptr_factory_.GetWeakPtr())); It seems we can also exclude the shared_sampler_ callback registeration too? We just return if it's the null process: https://cs.chromium.org/chromium/src/chrome/browser/task_manager/sampling/sha... https://codereview.chromium.org/2961423002/diff/20001/chrome/browser/task_man... File chrome/browser/task_manager/task_manager_browsertest.cc (right): https://codereview.chromium.org/2961423002/diff/20001/chrome/browser/task_man... chrome/browser/task_manager/task_manager_browsertest.cc:832: "a.com", "/cross_site_iframe_factory.html?a(b(b(c)))")); I have no idea what that URL means.
Hi, do you plan to continue working on this one?
On 2017/07/18 01:25:25, afakhry wrote: > Hi, do you plan to continue working on this one? What happened to this change? :)
The CQ bit was checked by nick@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...
Finally got back to this one. Thanks for the pings. https://codereview.chromium.org/2961423002/diff/20001/chrome/browser/task_man... File chrome/browser/task_manager/providers/task.cc (right): https://codereview.chromium.org/2961423002/diff/20001/chrome/browser/task_man... chrome/browser/task_manager/providers/task.cc:101: // TaskManagerImpl and TaskGroup implementations assume that a process id is On 2017/07/10 17:27:53, afakhry wrote: > Nit: process id => ditto. Done. https://codereview.chromium.org/2961423002/diff/20001/chrome/browser/task_man... File chrome/browser/task_manager/providers/task.h (right): https://codereview.chromium.org/2961423002/diff/20001/chrome/browser/task_man... chrome/browser/task_manager/providers/task.h:88: // Temporarily remove a task, change its process id, and then re-add it. On 2017/07/10 17:27:53, afakhry wrote: > Nit: For consistency, process id => process ID. Done. https://codereview.chromium.org/2961423002/diff/20001/chrome/browser/task_man... File chrome/browser/task_manager/providers/web_contents/web_contents_task_provider.cc (right): https://codereview.chromium.org/2961423002/diff/20001/chrome/browser/task_man... chrome/browser/task_manager/providers/web_contents/web_contents_task_provider.cc:186: // IsReady state. This might mean we know its the process ID. On 2017/07/10 17:27:53, afakhry wrote: > Nit: This might mean we know its (-the) process ID? Done. https://codereview.chromium.org/2961423002/diff/20001/chrome/browser/task_man... chrome/browser/task_manager/providers/web_contents/web_contents_task_provider.cc:192: if (task) { On 2017/07/10 17:27:53, afakhry wrote: > Nit: Much indented nested blocks? > if (!render_frame_host) > return; > > Task* task = GetTaskForFrame(render_frame_host); > if (!task) > return; Done. https://codereview.chromium.org/2961423002/diff/20001/chrome/browser/task_man... chrome/browser/task_manager/providers/web_contents/web_contents_task_provider.cc:193: base::ProcessId determine_pid_from_handle = base::kNullProcessId; On 2017/07/10 17:27:53, afakhry wrote: > Nit: Can you make this a const? Done. https://codereview.chromium.org/2961423002/diff/20001/chrome/browser/task_man... chrome/browser/task_manager/providers/web_contents/web_contents_task_provider.cc:273: site_instance == render_frame_host->GetParent()->GetSiteInstance()) On 2017/07/10 17:27:53, afakhry wrote: > Nit: braces here. Done. https://codereview.chromium.org/2961423002/diff/20001/chrome/browser/task_man... chrome/browser/task_manager/providers/web_contents/web_contents_task_provider.cc:329: if (new_task->process_id() == base::kNullProcessId) { On 2017/07/10 17:27:53, afakhry wrote: > Nit: Indented nested blocks here too? > > if (new_task->process_id() != base::kNullProcessId) > return; > > // <Your above comment here> > // PostTaskWhenProcessIsReady(...); I opted not to do this one; in my mind early-return is for cases where we know we're done with the whole operation (we hit an error or a null value, say). Whereas this just happens to be the last if() in the function. https://codereview.chromium.org/2961423002/diff/20001/chrome/browser/task_man... File chrome/browser/task_manager/sampling/task_group.cc (right): https://codereview.chromium.org/2961423002/diff/20001/chrome/browser/task_man... chrome/browser/task_manager/sampling/task_group.cc:136: weak_ptr_factory_.GetWeakPtr())); On 2017/07/10 17:27:54, afakhry wrote: > It seems we can also exclude the shared_sampler_ callback registeration too? > > We just return if it's the null process: > https://cs.chromium.org/chromium/src/chrome/browser/task_manager/sampling/sha... Done. https://codereview.chromium.org/2961423002/diff/20001/chrome/browser/task_man... File chrome/browser/task_manager/task_manager_browsertest.cc (right): https://codereview.chromium.org/2961423002/diff/20001/chrome/browser/task_man... chrome/browser/task_manager/task_manager_browsertest.cc:832: "a.com", "/cross_site_iframe_factory.html?a(b(b(c)))")); On 2017/07/10 17:27:54, afakhry wrote: > I have no idea what that URL means. Done.
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_...)
LGTM with nit. Thanks! https://codereview.chromium.org/2961423002/diff/20001/chrome/browser/task_man... File chrome/browser/task_manager/providers/web_contents/web_contents_task_provider.cc (right): https://codereview.chromium.org/2961423002/diff/20001/chrome/browser/task_man... chrome/browser/task_manager/providers/web_contents/web_contents_task_provider.cc:329: if (new_task->process_id() == base::kNullProcessId) { On 2017/07/31 22:30:29, ncarter (slow) wrote: > On 2017/07/10 17:27:53, afakhry wrote: > > Nit: Indented nested blocks here too? > > > > if (new_task->process_id() != base::kNullProcessId) > > return; > > > > // <Your above comment here> > > // PostTaskWhenProcessIsReady(...); > > I opted not to do this one; in my mind early-return is for cases where we know > we're done with the whole operation (we hit an error or a null value, say). > Whereas this just happens to be the last if() in the function. Acknowledged. https://codereview.chromium.org/2961423002/diff/20001/chrome/browser/task_man... File chrome/browser/task_manager/task_manager_browsertest.cc (right): https://codereview.chromium.org/2961423002/diff/20001/chrome/browser/task_man... chrome/browser/task_manager/task_manager_browsertest.cc:832: "a.com", "/cross_site_iframe_factory.html?a(b(b(c)))")); On 2017/07/31 22:30:29, ncarter (slow) wrote: > On 2017/07/10 17:27:54, afakhry wrote: > > I have no idea what that URL means. > > Done. Thanks! Much clearer now! :) https://codereview.chromium.org/2961423002/diff/40001/chrome/browser/task_man... File chrome/browser/task_manager/providers/task.h (right): https://codereview.chromium.org/2961423002/diff/40001/chrome/browser/task_man... chrome/browser/task_manager/providers/task.h:88: // Temporarily remove a task, change its process ID, and then re-add it. Nit: Looking at this comment again, I think it would be better to change it. For example: - ("this" task) instead of ("a" task). - remove and re-add where? To |observer| for instance. I'll leave the wording for you since you're better than me at this! :)
https://codereview.chromium.org/2961423002/diff/40001/chrome/browser/task_man... File chrome/browser/task_manager/providers/task.h (right): https://codereview.chromium.org/2961423002/diff/40001/chrome/browser/task_man... chrome/browser/task_manager/providers/task.h:88: // Temporarily remove a task, change its process ID, and then re-add it. On 2017/08/01 17:25:35, afakhry wrote: > Nit: Looking at this comment again, I think it would be better to change it. For > example: > > - ("this" task) instead of ("a" task). > - remove and re-add where? To |observer| for instance. > > I'll leave the wording for you since you're better than me at this! :) Done.
The CQ bit was checked by nick@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from afakhry@chromium.org Link to the patchset: https://codereview.chromium.org/2961423002/#ps60001 (title: "Rework comment.")
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": 60001, "attempt_start_ts": 1501628867120850,
"parent_rev": "4297bf0f3236ca84516f22ecf17bd5ebd732d346", "commit_rev":
"f3c7679c2bd39c5f3bf966808058eb78d366c9ef"}
CQ is committing da patch.
Bot data: {"patchset_id": 60001, "attempt_start_ts": 1501628867120850,
"parent_rev": "f0bae7e53ba808999edffb7402558a5b7be721f9", "commit_rev":
"6c760df2f6a9f5d1ce7051be6acfeebc26919774"}
Message was sent while issue was closed.
Description was changed from ========== [TaskManager] Allow a Task to mutate its PID after creation Prior to this change, WebContentsTaskProvider would defer task creation if the PID was not known at RenderFrameCreated time. However, for a newly-cloned WebContents, no event ever occurred to re-create the Task once the process finished launching. This was bug 738169. Fix the problem by never deferring task_manager::Task creation; instead, create Tasks for all live, current RenderFrameHosts. If the PID is not known immediately, allow it to be set on the task at a later time (when RenderProcessHost::IsReady() becomes true). Because the TaskManagerImpl was written with the expectation that Tasks have a constant PID, PID mutation is implemented in terms of temporarily removing the task, mutating the PID, and re-adding it. In WebContentsTaskProvider, we no longer need to implement RenderProcessGone. In TaskGroup, don't waste time collecting stats for a null process ID. Tests are added exercising this bug, and its subframe equivalent. Test coverage is added for the PID==0 case, and we ensure that the "Memory" column is populated with something nonzero. BUG=738169 ========== to ========== [TaskManager] Allow a Task to mutate its PID after creation Prior to this change, WebContentsTaskProvider would defer task creation if the PID was not known at RenderFrameCreated time. However, for a newly-cloned WebContents, no event ever occurred to re-create the Task once the process finished launching. This was bug 738169. Fix the problem by never deferring task_manager::Task creation; instead, create Tasks for all live, current RenderFrameHosts. If the PID is not known immediately, allow it to be set on the task at a later time (when RenderProcessHost::IsReady() becomes true). Because the TaskManagerImpl was written with the expectation that Tasks have a constant PID, PID mutation is implemented in terms of temporarily removing the task, mutating the PID, and re-adding it. In WebContentsTaskProvider, we no longer need to implement RenderProcessGone. In TaskGroup, don't waste time collecting stats for a null process ID. Tests are added exercising this bug, and its subframe equivalent. Test coverage is added for the PID==0 case, and we ensure that the "Memory" column is populated with something nonzero. BUG=738169 Review-Url: https://codereview.chromium.org/2961423002 Cr-Commit-Position: refs/heads/master@{#491180} Committed: https://chromium.googlesource.com/chromium/src/+/6c760df2f6a9f5d1ce7051be6acf... ==========
Message was sent while issue was closed.
Committed patchset #4 (id:60001) as https://chromium.googlesource.com/chromium/src/+/6c760df2f6a9f5d1ce7051be6acf... |
