Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1438273002:
cc: Don't crash if GMB allocation fails (Closed)
Descriptioncc: Don't crash if GMB allocation fails
BUG=554541
Committed: https://crrev.com/25f3c9987d5cba8a5350a3caf4fcba4d650c0b31
Cr-Commit-Position: refs/heads/master@{#363813}
Patch Set 1 #Patch Set 2 : Remove asserts and rebase #Patch Set 3 : Rebase #Messages
Total messages: 26 (10 generated)
ccameron@chromium.org changed reviewers: + reveman@chromium.org
This is the #1 crasher -- this avoids the crash (tested), but we should dig in more.
is this really better than crashing? I'm worried that allowing draw to fail will hide other drawing issues.
On 2015/11/12 19:44:06, reveman wrote: > is this really better than crashing? I'm worried that allowing draw to fail will > hide other drawing issues. In that case we should have AllocateGpuMemoryBuffer crash when it will return nullptr -- if we require that the allocation succeeds, we should crash as early as possible. I tested this locally, and it just produces GL errors and black quads (better than crashing IMO), but, that said, I'm actually going to disable kEnableGpuMemoryBufferCompositorResources for M48, cause we won't get the benefit yet.
Message was sent while issue was closed.
On 2015/11/12 20:13:15, ccameron wrote: > On 2015/11/12 19:44:06, reveman wrote: > > is this really better than crashing? I'm worried that allowing draw to fail > will > > hide other drawing issues. > > In that case we should have AllocateGpuMemoryBuffer crash when it will return > nullptr -- if we require that the allocation succeeds, we should crash as early > as possible. > > I tested this locally, and it just produces GL errors and black quads (better > than crashing IMO), but, that said, I'm actually going to disable > kEnableGpuMemoryBufferCompositorResources for M48, cause we won't get the > benefit yet. Actually, we really do need this. The consequences of AllocateGpuMemoryBuffer failing should be the same as any other GL error. Textures can fail to allocate, and we will just report an error. This does not happen due to requesting invalid GMB formats (if that happens then we get a shared memory GMB which just produces GL errors). It seems most likely that this is something in the IOSurface allocation code failing.
Message was sent while issue was closed.
Description was changed from ========== cc: Don't crash if GMB allocation fails BUG=554541 CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel ========== to ========== cc: Don't crash if GMB allocation fails BUG=554541 CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel ==========
On 2015/11/27 at 23:49:12, ccameron wrote: > On 2015/11/12 20:13:15, ccameron wrote: > > On 2015/11/12 19:44:06, reveman wrote: > > > is this really better than crashing? I'm worried that allowing draw to fail > > will > > > hide other drawing issues. > > > > In that case we should have AllocateGpuMemoryBuffer crash when it will return > > nullptr -- if we require that the allocation succeeds, we should crash as early > > as possible. > > > > I tested this locally, and it just produces GL errors and black quads (better > > than crashing IMO), but, that said, I'm actually going to disable > > kEnableGpuMemoryBufferCompositorResources for M48, cause we won't get the > > benefit yet. > > Actually, we really do need this. The consequences of AllocateGpuMemoryBuffer failing should be the same as any other GL error. Textures can fail to allocate, and we will just report an error. > > This does not happen due to requesting invalid GMB formats (if that happens then we get a shared memory GMB which just produces GL errors). It seems most likely that this is something in the IOSurface allocation code failing. GMBs are not tied to a context as textures and allocation should never fail except as a result of an OOM error. Do you know in what situation this is failing?
On 2015/11/30 16:49:16, reveman wrote: > > GMBs are not tied to a context as textures and allocation should never fail > except as a result of an OOM error. Do you know in what situation this is > failing? The type of OOM that might cause an IOSurface to fail allocation is not the same as would cause malloc() to fail, so I don't agree completely with that characterization. In that case, we should never return nullptr from https://code.google.com/p/chromium/codesearch#chromium/src/content/child/chil... but rather, always CHECK. I still need to track down why the IOSurface allocation is failing. I suspect we'll have to do that with histograms, since the failures only appear in the field.
On 2015/11/30 at 20:30:56, ccameron wrote: > On 2015/11/30 16:49:16, reveman wrote: > > > > GMBs are not tied to a context as textures and allocation should never fail > > except as a result of an OOM error. Do you know in what situation this is > > failing? > > The type of OOM that might cause an IOSurface to fail allocation is not the same as would cause malloc() to fail, so I don't agree completely with that characterization. I agree that the reasons for failure is not necessarily the same as malloc/mmap but like malloc there's no valid usage that can result in a failure that we need to be able to handle. That's the idea at least. > > In that case, we should never return nullptr from > https://code.google.com/p/chromium/codesearch#chromium/src/content/child/chil... > but rather, always CHECK. Yes, we should change that to CHECKs. FYI, this used to be allowed to fail back when the mechanism for allocating these buffers didn't have the ability to re-launch the gpu process but since that changed CHECKs here would be more appropriate. > > I still need to track down why the IOSurface allocation is failing. I suspect we'll have to do that with histograms, since the failures only appear in the field.
Ptal again. We tried adding CHECKs to the allocation path, and it was responsible for 65% of the renderer crashes on Canary. That level of destabilization of Canary is not acceptable. While we investigate this, we should either 1. Make cc::ResourceProvider robust to AllocateGpuMemoryBuffer failing 2. Remove all callers to AllocateGpuMemoryBuffer until all of these allocation failures are understood. [1] is what this patch does, and [2] is not a realistic option.
Description was changed from ========== cc: Don't crash if GMB allocation fails BUG=554541 CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel ========== to ========== cc: Don't crash if GMB allocation fails TBR=reveman,avi,vmiura BUG=554541 CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel ==========
The CQ bit was checked by ccameron@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1438273002/20001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1438273002/20001
On 2015/12/08 00:26:04, commit-bot: I haz the power wrote: > CQ is trying da patch. Follow status at > https://chromium-cq-status.appspot.com/patch-status/1438273002/20001 > View timeline at > https://chromium-cq-status.appspot.com/patch-timeline/1438273002/20001 TBRing this to fix tonight's dev.
The CQ bit was unchecked by ccameron@chromium.org
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: cast_shell_linux on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...) linux_chromium_chromeos_ozone_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_compile_dbg_32_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
lgtm
Thanks!
The CQ bit was checked by ccameron@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1438273002/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1438273002/40001
Description was changed from ========== cc: Don't crash if GMB allocation fails TBR=reveman,avi,vmiura BUG=554541 CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel ========== to ========== cc: Don't crash if GMB allocation fails BUG=554541 ==========
Message was sent while issue was closed.
Description was changed from ========== cc: Don't crash if GMB allocation fails BUG=554541 ========== to ========== cc: Don't crash if GMB allocation fails BUG=554541 ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001)
Message was sent while issue was closed.
Description was changed from ========== cc: Don't crash if GMB allocation fails BUG=554541 ========== to ========== cc: Don't crash if GMB allocation fails BUG=554541 Committed: https://crrev.com/25f3c9987d5cba8a5350a3caf4fcba4d650c0b31 Cr-Commit-Position: refs/heads/master@{#363813} ==========
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/25f3c9987d5cba8a5350a3caf4fcba4d650c0b31 Cr-Commit-Position: refs/heads/master@{#363813} |
