Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2810763002:
gpu: Do not relaunch the process during teardown. (Closed)
Descriptiongpu: Do not relaunch the process during teardown.
If the browser has started shutting down, then do not attempt to relaunch
the gpu process.
BUG=709890
Review-Url: https://codereview.chromium.org/2810763002
Cr-Commit-Position: refs/heads/master@{#463395}
Committed: https://chromium.googlesource.com/chromium/src/+/02efd05d378211f550761fd26e23be5f71644a08
Patch Set 1 #
Total comments: 3
Patch Set 2 : AtomicFlag #
Messages
Total messages: 21 (13 generated)
The CQ bit was checked by sadrul@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...
sadrul@chromium.org changed reviewers: + jam@chromium.org
jam@ Would you mind taking a look to see if this looks reasonable? piman@ FYI (but would be happy to know if you have feedback) Thanks! https://codereview.chromium.org/2810763002/diff/1/content/browser/gpu/gpu_pro... File content/browser/gpu/gpu_process_host.cc (right): https://codereview.chromium.org/2810763002/diff/1/content/browser/gpu/gpu_pro... content/browser/gpu/gpu_process_host.cc:374: return nullptr; Note that this is running on the IO thread. The flag does get set [1] before the thread-teardown starts [2]. But I am not entirely sure if this is OK to do in all cases. [1] https://cs.chromium.org/chromium/src/content/browser/browser_main_runner.cc?t... [2] https://cs.chromium.org/chromium/src/content/browser/browser_main_runner.cc?t...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
piman@chromium.org changed reviewers: + piman@chromium.org
https://codereview.chromium.org/2810763002/diff/1/content/browser/gpu/gpu_pro... File content/browser/gpu/gpu_process_host.cc (right): https://codereview.chromium.org/2810763002/diff/1/content/browser/gpu/gpu_pro... content/browser/gpu/gpu_process_host.cc:374: return nullptr; On 2017/04/10 15:18:00, sadrul wrote: > Note that this is running on the IO thread. The flag does get set [1] before the > thread-teardown starts [2]. But I am not entirely sure if this is OK to do in > all cases. > > [1] > https://cs.chromium.org/chromium/src/content/browser/browser_main_runner.cc?t... > [2] > https://cs.chromium.org/chromium/src/content/browser/browser_main_runner.cc?t... Isn't this a data race, anyway?
https://codereview.chromium.org/2810763002/diff/1/content/browser/gpu/gpu_pro... File content/browser/gpu/gpu_process_host.cc (right): https://codereview.chromium.org/2810763002/diff/1/content/browser/gpu/gpu_pro... content/browser/gpu/gpu_process_host.cc:374: return nullptr; On 2017/04/10 17:14:32, piman wrote: > On 2017/04/10 15:18:00, sadrul wrote: > > Note that this is running on the IO thread. The flag does get set [1] before > the > > thread-teardown starts [2]. But I am not entirely sure if this is OK to do in > > all cases. > > > > [1] > > > https://cs.chromium.org/chromium/src/content/browser/browser_main_runner.cc?t... > > [2] > > > https://cs.chromium.org/chromium/src/content/browser/browser_main_runner.cc?t... > > Isn't this a data race, anyway? In the case when it's not terminating (i.e. in most general case), it is, yep. (In the case when it is terminating, it's safe I think, since the main-thread is blocked on the io thread shutting down.) Would introducing a lock in BrowserMainRunner::ExitedMainMessageLoop() work?
On 2017/04/10 17:51:50, sadrul wrote: > https://codereview.chromium.org/2810763002/diff/1/content/browser/gpu/gpu_pro... > File content/browser/gpu/gpu_process_host.cc (right): > > https://codereview.chromium.org/2810763002/diff/1/content/browser/gpu/gpu_pro... > content/browser/gpu/gpu_process_host.cc:374: return nullptr; > On 2017/04/10 17:14:32, piman wrote: > > On 2017/04/10 15:18:00, sadrul wrote: > > > Note that this is running on the IO thread. The flag does get set [1] before > > the > > > thread-teardown starts [2]. But I am not entirely sure if this is OK to do > in > > > all cases. > > > > > > [1] > > > > > > https://cs.chromium.org/chromium/src/content/browser/browser_main_runner.cc?t... > > > [2] > > > > > > https://cs.chromium.org/chromium/src/content/browser/browser_main_runner.cc?t... > > > > Isn't this a data race, anyway? > > In the case when it's not terminating (i.e. in most general case), it is, yep. > (In the case when it is terminating, it's safe I think, since the main-thread is > blocked on the io thread shutting down.) > > Would introducing a lock in BrowserMainRunner::ExitedMainMessageLoop() work? Provided it is also taken while writing to g_exited_main_message_loop, yes. Or, you could use AtomicFlag which is safe for this pattern (always writing from the same thread, reading from any).
The CQ bit was checked by sadrul@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...
On 2017/04/10 17:58:01, piman wrote: > On 2017/04/10 17:51:50, sadrul wrote: > > > https://codereview.chromium.org/2810763002/diff/1/content/browser/gpu/gpu_pro... > > File content/browser/gpu/gpu_process_host.cc (right): > > > > > https://codereview.chromium.org/2810763002/diff/1/content/browser/gpu/gpu_pro... > > content/browser/gpu/gpu_process_host.cc:374: return nullptr; > > On 2017/04/10 17:14:32, piman wrote: > > > On 2017/04/10 15:18:00, sadrul wrote: > > > > Note that this is running on the IO thread. The flag does get set [1] > before > > > the > > > > thread-teardown starts [2]. But I am not entirely sure if this is OK to do > > in > > > > all cases. > > > > > > > > [1] > > > > > > > > > > https://cs.chromium.org/chromium/src/content/browser/browser_main_runner.cc?t... > > > > [2] > > > > > > > > > > https://cs.chromium.org/chromium/src/content/browser/browser_main_runner.cc?t... > > > > > > Isn't this a data race, anyway? > > > > In the case when it's not terminating (i.e. in most general case), it is, yep. > > (In the case when it is terminating, it's safe I think, since the main-thread > is > > blocked on the io thread shutting down.) > > > > Would introducing a lock in BrowserMainRunner::ExitedMainMessageLoop() work? > > Provided it is also taken while writing to g_exited_main_message_loop, yes. > Or, you could use AtomicFlag which is safe for this pattern (always writing from > the same thread, reading from any). Cool! Done. PTAL
lgtm
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by sadrul@chromium.org
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": 20001, "attempt_start_ts": 1491858116804100,
"parent_rev": "f56ccf3f087fe868c7188ce23a162886744211f7", "commit_rev":
"02efd05d378211f550761fd26e23be5f71644a08"}
Message was sent while issue was closed.
Description was changed from ========== gpu: Do not relaunch the process during teardown. If the browser has started shutting down, then do not attempt to relaunch the gpu process. BUG=709890 ========== to ========== gpu: Do not relaunch the process during teardown. If the browser has started shutting down, then do not attempt to relaunch the gpu process. BUG=709890 Review-Url: https://codereview.chromium.org/2810763002 Cr-Commit-Position: refs/heads/master@{#463395} Committed: https://chromium.googlesource.com/chromium/src/+/02efd05d378211f550761fd26e23... ==========
Message was sent while issue was closed.
Committed patchset #2 (id:20001) as https://chromium.googlesource.com/chromium/src/+/02efd05d378211f550761fd26e23... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
