|
|
Descriptiongpu: Reduce direct use of BrowserGpuMemoryBufferManager.
. BrowserGpuMemoryBufferManager register itself as a dump-provider to
MemoryDumpManager upon creation.
. Instead of getting the list of supported native gpu memory buffer
configurations from BrowserGpuMemoryBufferManager, generate the list
directly in GpuInternalUI. This is called only when the chrome:gpu page is
loaded. So this should happen rarely enough that this should not introduce
any latency, or any other kind of regression.
BUG=733482
Review-Url: https://codereview.chromium.org/2971083002
Cr-Commit-Position: refs/heads/master@{#485480}
Committed: https://chromium.googlesource.com/chromium/src/+/d30c3532f0cead481b8ef4c7290e0eb15d8fb6d0
Patch Set 1 #Patch Set 2 : . #Patch Set 3 : . #Patch Set 4 : . #Patch Set 5 : . #Patch Set 6 : . #
Total comments: 2
Patch Set 7 : . #
Total comments: 4
Patch Set 8 : . #Patch Set 9 : tot merge #Patch Set 10 : . #Patch Set 11 : . #
Messages
Total messages: 58 (44 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...
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...
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...
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 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: This issue passed the CQ dry run.
Description was changed from ========== [exp] Change some things about how BrowserGpuChannelHostFactory is torn down. BUG= ========== to ========== gpu: Reduce direct use of BrowserGpuMemoryBufferManager. . BrowserGpuMemoryBufferManager register itself as a dump-provider to MemoryDumpManager upon creation. Since it outlives the IO thread, it no longer explicitly unregisters. . Instead of getting the list of supported native gpu memory buffer configurations from BrowserGpuMemoryBufferManager, generate the list directly in GpuInternalUI. This is called only when the chrome:gpu page is loaded. So this should happen rarely enough that this should not introduce any latency, or any other kind of regression. . BrowserGpuChannelHostFactory::CloseChannel() does not actually seem to be needed. So remove it. BUG=733482 ==========
sadrul@chromium.org changed reviewers: + jbauman@chromium.org, sammc@chromium.org
sammc@ Could you please look at the change related to BrowserGpuChannelHostFactory::CloseChannel()? From what I can tell, it was introduced in https://codereview.chromium.org/2633053002, but seem unrelated to the rest of that CL. Do you remember why it was needed at the time? It does not seem to be needed any longer (at least, things seem to be working fine locally and on the trybots) jbauman@ For all content/browser changes. Thank you!
https://codereview.chromium.org/2971083002/diff/100001/content/browser/gpu/br... File content/browser/gpu/browser_gpu_memory_buffer_manager.cc (right): https://codereview.chromium.org/2971083002/diff/100001/content/browser/gpu/br... content/browser/gpu/browser_gpu_memory_buffer_manager.cc:78: // BrowserProcessSubThread::IOThreadPreCleanUp). This comment about unregistration is incorrect now. Also, shouldn't we still unregister it? I can imagine us wanting to do a memory dump on shutdown in the future.
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...
https://codereview.chromium.org/2971083002/diff/100001/content/browser/gpu/br... File content/browser/gpu/browser_gpu_memory_buffer_manager.cc (right): https://codereview.chromium.org/2971083002/diff/100001/content/browser/gpu/br... content/browser/gpu/browser_gpu_memory_buffer_manager.cc:78: // BrowserProcessSubThread::IOThreadPreCleanUp). On 2017/07/06 21:39:18, jbauman wrote: > This comment about unregistration is incorrect now. Also, shouldn't we still > unregister it? I can imagine us wanting to do a memory dump on shutdown in the > future. Hm, you are right. The MemoryDumpManager does not trigger the provider if the provider is not running on the specified task-runner, and since the IO thread has been torn down at this point, it would never run for BrowserGpuMemoryBufferManager. However, if we attempt to do a memory dump during tear-down, it will still try to poke the now-destroyed provider to look at its task-runner, causing uaf. I have restored the unregistration in BrowserProcessSubThread.
lgtm
On 2017/07/06 07:34:49, sadrul wrote: > sammc@ Could you please look at the change related to > BrowserGpuChannelHostFactory::CloseChannel()? From what I can tell, it was > introduced in https://codereview.chromium.org/2633053002, but seem unrelated to > the rest of that CL. Do you remember why it was needed at the time? It does not > seem to be needed any longer (at least, things seem to be working fine locally > and on the trybots) > At the time, with the rest of that CL without that change some tests were failing with no output on the Windows bots. I don't know why it was happening and I never managed to repro it locally but shutting down the channel stopped it from happening.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)
sadrul@chromium.org changed reviewers: + piman@chromium.org
+piman@ for browser_main_loop.cc owner On 2017/07/07 01:12:08, Sam McNally wrote: > On 2017/07/06 07:34:49, sadrul wrote: > > sammc@ Could you please look at the change related to > > BrowserGpuChannelHostFactory::CloseChannel()? From what I can tell, it was > > introduced in https://codereview.chromium.org/2633053002, but seem unrelated > to > > the rest of that CL. Do you remember why it was needed at the time? It does > not > > seem to be needed any longer (at least, things seem to be working fine locally > > and on the trybots) > > > At the time, with the rest of that CL without that change some tests were > failing with no output on the Windows bots. I don't know why it was happening > and I never managed to repro it locally but shutting down the channel stopped it > from happening. Thanks for the details! Looks like the bots with this CL have been happy with not calling it (the failures in the most recent patchset seem related to the infra issue that affected the bots). I will keep an eye on the bots for failures.
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...
Description was changed from ========== gpu: Reduce direct use of BrowserGpuMemoryBufferManager. . BrowserGpuMemoryBufferManager register itself as a dump-provider to MemoryDumpManager upon creation. Since it outlives the IO thread, it no longer explicitly unregisters. . Instead of getting the list of supported native gpu memory buffer configurations from BrowserGpuMemoryBufferManager, generate the list directly in GpuInternalUI. This is called only when the chrome:gpu page is loaded. So this should happen rarely enough that this should not introduce any latency, or any other kind of regression. . BrowserGpuChannelHostFactory::CloseChannel() does not actually seem to be needed. So remove it. BUG=733482 ========== to ========== gpu: Reduce direct use of BrowserGpuMemoryBufferManager. . BrowserGpuMemoryBufferManager register itself as a dump-provider to MemoryDumpManager upon creation. . Instead of getting the list of supported native gpu memory buffer configurations from BrowserGpuMemoryBufferManager, generate the list directly in GpuInternalUI. This is called only when the chrome:gpu page is loaded. So this should happen rarely enough that this should not introduce any latency, or any other kind of regression. . BrowserGpuChannelHostFactory::CloseChannel() does not actually seem to be needed. So remove it. BUG=733482 ==========
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2971083002/diff/120001/content/browser/browse... File content/browser/browser_main_loop.cc (left): https://codereview.chromium.org/2971083002/diff/120001/content/browser/browse... content/browser/browser_main_loop.cc:1233: BrowserGpuChannelHostFactory::instance()->CloseChannel(); I'm surprised we don't need this. The GpuChannelHost needs the IO thread to post tasks, otherwise I don't think it can clean up correctly... https://codereview.chromium.org/2971083002/diff/120001/content/browser/gpu/br... File content/browser/gpu/browser_gpu_memory_buffer_manager.cc (right): https://codereview.chromium.org/2971083002/diff/120001/content/browser/gpu/br... content/browser/gpu/browser_gpu_memory_buffer_manager.cc:81: BrowserThread::GetTaskRunnerForThread(BrowserThread::IO)); I don't think the IO thread exists yet at this point
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...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
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...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2971083002/diff/120001/content/browser/browse... File content/browser/browser_main_loop.cc (left): https://codereview.chromium.org/2971083002/diff/120001/content/browser/browse... content/browser/browser_main_loop.cc:1233: BrowserGpuChannelHostFactory::instance()->CloseChannel(); On 2017/07/07 03:57:10, piman wrote: > I'm surprised we don't need this. The GpuChannelHost needs the IO thread to post > tasks, otherwise I don't think it can clean up correctly... What kind of failures do you think we might have? The task-runners for the threads are retained when the threads are shut down, so code that attempts to post-task on the IO thread will not crash. But those tasks will never run either, of course. I am not sure what kind of clean up works happen in those tasks though. I notice the ipc support is torn down before the IO thread is destroyed (see below in line ~1238), so if any of the posted tasks did any IPC, they won't be processed anyway. Are there other issues I should watch for? (memory leaks, perhaps?) (fwiw, the trybots have been fairly OK with the change) https://codereview.chromium.org/2971083002/diff/120001/content/browser/gpu/br... File content/browser/gpu/browser_gpu_memory_buffer_manager.cc (right): https://codereview.chromium.org/2971083002/diff/120001/content/browser/gpu/br... content/browser/gpu/browser_gpu_memory_buffer_manager.cc:81: BrowserThread::GetTaskRunnerForThread(BrowserThread::IO)); On 2017/07/07 03:57:10, piman wrote: > I don't think the IO thread exists yet at this point BrowserGpuMemoryBufferManager is created from BrowserMainLoop::BrowserThreadsStarted() (through BrowserGpuChannelHostFactory) [1], which happens after all the threads have been created [2]. I have added a DCHECK() here to make sure that continues to be the case. [1] https://cs.chromium.org/chromium/src/content/browser/browser_main_loop.cc?l=1450 [2] https://cs.chromium.org/chromium/src/content/browser/browser_main_loop.cc?l=906
On Fri, Jul 7, 2017 at 8:19 PM, <sadrul@chromium.org> wrote: > > https://codereview.chromium.org/2971083002/diff/120001/ > content/browser/browser_main_loop.cc > File content/browser/browser_main_loop.cc (left): > > https://codereview.chromium.org/2971083002/diff/120001/ > content/browser/browser_main_loop.cc#oldcode1233 > content/browser/browser_main_loop.cc:1233: > BrowserGpuChannelHostFactory::instance()->CloseChannel(); > On 2017/07/07 03:57:10, piman wrote: > > I'm surprised we don't need this. The GpuChannelHost needs the IO > thread to post > > tasks, otherwise I don't think it can clean up correctly... > > What kind of failures do you think we might have? The task-runners for > the threads are retained when the threads are shut down, so code that > attempts to post-task on the IO thread will not crash. But those tasks > will never run either, of course. I am not sure what kind of clean up > works happen in those tasks though. I notice the ipc support is torn > down before the IO thread is destroyed (see below in line ~1238), so if > any of the posted tasks did any IPC, they won't be processed anyway. Are > there other issues I should watch for? (memory leaks, perhaps?) > For example, sync IPCs post a task to the IO thread and block waiting for either the reply or the notification that we're terminating (via BrowserGpuChannelHostFactory's shutdown_event_). That means that threads could try to send sync IPCs and block forever, therefore blocking termination if we try to join those. GpuChannelHost::DestroyChannel ensures we fail all messages before we get to that point. I suppose if we know there's no more thread that's accessing the GpuChannel at that point, it would be safe. If so we should move the BrowserGpuChannelHostFactory::Terminate to before we join the IO thread. (fwiw, the trybots have been fairly OK with the change) > > https://codereview.chromium.org/2971083002/diff/120001/ > content/browser/gpu/browser_gpu_memory_buffer_manager.cc > File content/browser/gpu/browser_gpu_memory_buffer_manager.cc (right): > > https://codereview.chromium.org/2971083002/diff/120001/ > content/browser/gpu/browser_gpu_memory_buffer_manager.cc#newcode81 > content/browser/gpu/browser_gpu_memory_buffer_manager.cc:81: > BrowserThread::GetTaskRunnerForThread(BrowserThread::IO)); > On 2017/07/07 03:57:10, piman wrote: > > I don't think the IO thread exists yet at this point > > BrowserGpuMemoryBufferManager is created from > BrowserMainLoop::BrowserThreadsStarted() (through > BrowserGpuChannelHostFactory) [1], which happens after all the threads > have been created [2]. > > I have added a DCHECK() here to make sure that continues to be the case. > Ok, thanks! > > [1] > https://cs.chromium.org/chromium/src/content/browser/ > browser_main_loop.cc?l=1450 > [2] > https://cs.chromium.org/chromium/src/content/browser/ > browser_main_loop.cc?l=906 > > https://codereview.chromium.org/2971083002/ > -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
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/07/10 20:58:08, chromium-reviews wrote: > On Fri, Jul 7, 2017 at 8:19 PM, <mailto:sadrul@chromium.org> wrote: > > > > > https://codereview.chromium.org/2971083002/diff/120001/ > > content/browser/browser_main_loop.cc > > File content/browser/browser_main_loop.cc (left): > > > > https://codereview.chromium.org/2971083002/diff/120001/ > > content/browser/browser_main_loop.cc#oldcode1233 > > content/browser/browser_main_loop.cc:1233: > > BrowserGpuChannelHostFactory::instance()->CloseChannel(); > > On 2017/07/07 03:57:10, piman wrote: > > > I'm surprised we don't need this. The GpuChannelHost needs the IO > > thread to post > > > tasks, otherwise I don't think it can clean up correctly... > > > > What kind of failures do you think we might have? The task-runners for > > the threads are retained when the threads are shut down, so code that > > attempts to post-task on the IO thread will not crash. But those tasks > > will never run either, of course. I am not sure what kind of clean up > > works happen in those tasks though. I notice the ipc support is torn > > down before the IO thread is destroyed (see below in line ~1238), so if > > any of the posted tasks did any IPC, they won't be processed anyway. Are > > there other issues I should watch for? (memory leaks, perhaps?) > > > > For example, sync IPCs post a task to the IO thread and block waiting for > either the reply or the notification that we're terminating > (via BrowserGpuChannelHostFactory's shutdown_event_). That means that > threads could try to send sync IPCs and block forever, therefore blocking > termination if we try to join those. GpuChannelHost::DestroyChannel ensures > we fail all messages before we get to that point. > I suppose if we know there's no more thread that's accessing the GpuChannel > at that point, it would be safe. If so we should move > the BrowserGpuChannelHostFactory::Terminate to before we join the IO thread. I tried this, and it looks like there are some failures on the trybots (the DCHECK()s in GpuClient trip, which expect to find BrowserGpuMemoryBufferManager, which no longer exists since BrowserGpuChannelHostFactory takes that down too). I have restored CloseChannel() in this CL. I will look to see if we can actually remove it in a separate CL. PTAL. Thanks!
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...
Description was changed from ========== gpu: Reduce direct use of BrowserGpuMemoryBufferManager. . BrowserGpuMemoryBufferManager register itself as a dump-provider to MemoryDumpManager upon creation. . Instead of getting the list of supported native gpu memory buffer configurations from BrowserGpuMemoryBufferManager, generate the list directly in GpuInternalUI. This is called only when the chrome:gpu page is loaded. So this should happen rarely enough that this should not introduce any latency, or any other kind of regression. . BrowserGpuChannelHostFactory::CloseChannel() does not actually seem to be needed. So remove it. BUG=733482 ========== to ========== gpu: Reduce direct use of BrowserGpuMemoryBufferManager. . BrowserGpuMemoryBufferManager register itself as a dump-provider to MemoryDumpManager upon creation. . Instead of getting the list of supported native gpu memory buffer configurations from BrowserGpuMemoryBufferManager, generate the list directly in GpuInternalUI. This is called only when the chrome:gpu page is loaded. So this should happen rarely enough that this should not introduce any latency, or any other kind of regression. BUG=733482 ==========
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
The patchset sent to the CQ was uploaded after l-g-t-m from jbauman@chromium.org Link to the patchset: https://codereview.chromium.org/2971083002/#ps200001 (title: ".")
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": 200001, "attempt_start_ts": 1499735407024110, "parent_rev": "c307e2cda54a47cbf6ef0c1dc6155297c87bf7d1", "commit_rev": "1695732bd9af8efc8e925883324c6978d80bc33f"}
CQ is committing da patch. Bot data: {"patchset_id": 200001, "attempt_start_ts": 1499735407024110, "parent_rev": "6d80d5eec932db8b7b42322029a9420f5cbe4961", "commit_rev": "d30c3532f0cead481b8ef4c7290e0eb15d8fb6d0"}
Message was sent while issue was closed.
Description was changed from ========== gpu: Reduce direct use of BrowserGpuMemoryBufferManager. . BrowserGpuMemoryBufferManager register itself as a dump-provider to MemoryDumpManager upon creation. . Instead of getting the list of supported native gpu memory buffer configurations from BrowserGpuMemoryBufferManager, generate the list directly in GpuInternalUI. This is called only when the chrome:gpu page is loaded. So this should happen rarely enough that this should not introduce any latency, or any other kind of regression. BUG=733482 ========== to ========== gpu: Reduce direct use of BrowserGpuMemoryBufferManager. . BrowserGpuMemoryBufferManager register itself as a dump-provider to MemoryDumpManager upon creation. . Instead of getting the list of supported native gpu memory buffer configurations from BrowserGpuMemoryBufferManager, generate the list directly in GpuInternalUI. This is called only when the chrome:gpu page is loaded. So this should happen rarely enough that this should not introduce any latency, or any other kind of regression. BUG=733482 Review-Url: https://codereview.chromium.org/2971083002 Cr-Commit-Position: refs/heads/master@{#485480} Committed: https://chromium.googlesource.com/chromium/src/+/d30c3532f0cead481b8ef4c7290e... ==========
Message was sent while issue was closed.
Committed patchset #11 (id:200001) as https://chromium.googlesource.com/chromium/src/+/d30c3532f0cead481b8ef4c7290e... |