Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2559633002:
[DXVAVDA] Add a vectored exception handler so a dump is reported on driver error. (Closed)
Description[DXVAVDA] Add a vectored exception handler so a dump is reported on driver error.
The MS VP9 MFT swallows driver exceptions and later hangs because it gets
into a weird state. Instead the vectored exception handler can upload a
crash dump so we can get more actionable feedback.
BUG=636158
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel
Committed: https://crrev.com/3a43b1fab59f70110154fd062ba908605638188a
Cr-Commit-Position: refs/heads/master@{#437667}
Patch Set 1 #Patch Set 2 : make sure crashpad runs #Patch Set 3 : fix build #
Total comments: 4
Patch Set 4 : don't crash, use tls #
Total comments: 1
Patch Set 5 : change to 1 #Messages
Total messages: 44 (29 generated)
Description was changed from ========== [DXVAVDA] Add a vectored exception handler so Chrome crashes on driver error. The MS VP9 MFT swallows driver exceptions and later hangs because it gets into a weird state. Instead the vectored exception handler can cause a crash, which makes more useful stack traces and is faster to recover from. BUG=636158 ========== to ========== [DXVAVDA] Add a vectored exception handler so Chrome crashes on driver error. The MS VP9 MFT swallows driver exceptions and later hangs because it gets into a weird state. Instead the vectored exception handler can cause a crash, which makes more useful stack traces and is faster to recover from. BUG=636158 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel ==========
The CQ bit was checked by jbauman@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: win_optional_gpu_tests_rel on master.tryserver.chromium.win (JOB_TIMED_OUT, no build URL)
The CQ bit was checked by jbauman@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: win_chromium_compile_dbg_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)
The CQ bit was checked by jbauman@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...
jbauman@chromium.org changed reviewers: + brucedawson@chromium.org, scottmg@chromium.org
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_optional_gpu_tests_rel on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_optional_gpu_...)
https://codereview.chromium.org/2559633002/diff/40001/media/gpu/dxva_video_de... File media/gpu/dxva_video_decode_accelerator_win.cc (right): https://codereview.chromium.org/2559633002/diff/40001/media/gpu/dxva_video_de... media/gpu/dxva_video_decode_accelerator_win.cc:27: #include "base/debug/dump_without_crashing.h" Instead of this, #include "third_party/crashpad/crashpad/client/crashpad_client.h" https://codereview.chromium.org/2559633002/diff/40001/media/gpu/dxva_video_de... media/gpu/dxva_video_decode_accelerator_win.cc:188: // Try to get crashpad to handle this. Then, replace 188-190 with CrashpadClient::DumpAndCrash(exception_pointers); https://codereview.chromium.org/2559633002/diff/40001/media/gpu/dxva_video_de... media/gpu/dxva_video_decode_accelerator_win.cc:199: ScopedExceptionCrasher(bool handle_exception) { explicit https://codereview.chromium.org/2559633002/diff/40001/media/gpu/dxva_video_de... media/gpu/dxva_video_decode_accelerator_win.cc:210: void* handler_ = nullptr; DISALLOW...
I was wondering about adding a global VEH in addition to the UEF that we have now to handle certain |ExceptionCode|s. But I haven't investigated it too much yet, I figured we probably have too many things causing handled exceptions.
On 2016/12/08 17:21:46, scottmg wrote: > I was wondering about adding a global VEH in addition to the UEF that we have > now to handle certain |ExceptionCode|s. But I haven't investigated it too much > yet, I figured we probably have too many things causing handled exceptions. Have you been able to reproduce the bad behavior? I would be concerned that there was some code, somewhere, that relied on raising exceptions for correct behavior. Not AV type exceptions, but C++ exceptions, exceptions used for naming threads, etc. Do you know how this code interacts with such code? Preventing the swallowing of exceptions seems perilous, even though I agree with the desire.
On 2016/12/08 21:27:10, brucedawson wrote: > On 2016/12/08 17:21:46, scottmg wrote: > > I was wondering about adding a global VEH in addition to the UEF that we have > > now to handle certain |ExceptionCode|s. But I haven't investigated it too much > > yet, I figured we probably have too many things causing handled exceptions. > > Have you been able to reproduce the bad behavior? > > I would be concerned that there was some code, somewhere, that relied on raising > exceptions for correct behavior. Not AV type exceptions, but C++ exceptions, > exceptions used for naming threads, etc. Do you know how this code interacts > with such code? Preventing the swallowing of exceptions seems perilous, even > though I agree with the desire. No, haven't really tried. I was intending to only crash on a whitelist of specific ExceptionCodes (i.e. =0xc0000005), but I could certainly imagine v8 using probing/AV guards to allocate jit pages or something. I have no idea. The main drawback of the UEF is that it's not very complete on Windows, and there's a lot of cases that escape detection because Windows decides it knows best and sends the crash straight to watson. e.g. recently https://bugs.chromium.org/p/chromium/issues/detail?id=670466. (WER's hooks are also weaksauce https://bugs.chromium.org/p/crashpad/issues/detail?id=133#c3 ) Anyway, sort of off-topic here. The way John added a localized VEH here seems OK, even though it's possible the the input/output processing could trigger an unintended crash.
> Anyway, sort of off-topic here. The way John added a localized VEH here seems Except that there is no such thing as a localized VEH. Well, it is temporaly localized, but if another thread triggers an exception while John's exception is active then that exception will be caught. https://msdn.microsoft.com/en-us/library/windows/desktop/ms681420(v=vs.85).aspx
On 2016/12/08 21:52:09, brucedawson wrote: > > Anyway, sort of off-topic here. The way John added a localized VEH here seems > > Except that there is no such thing as a localized VEH. Well, it is temporaly > localized, but if another thread triggers an exception while John's exception is > active then that exception will be caught. > > https://msdn.microsoft.com/en-us/library/windows/desktop/ms681420(v=vs.85).aspx Yeah, good point. I don't know how to evaluate what else is potentially going on the in the GPU process. Maybe it's a bad idea though. We could also try a continue handler that does a DumpWithoutCrashing() instead? It's going to take a ~couple seconds to dump, which might amount to a similar "downtime" as crashing the gpu anyway.
The CQ bit was checked by jbauman@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 ========== [DXVAVDA] Add a vectored exception handler so Chrome crashes on driver error. The MS VP9 MFT swallows driver exceptions and later hangs because it gets into a weird state. Instead the vectored exception handler can cause a crash, which makes more useful stack traces and is faster to recover from. BUG=636158 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel ========== to ========== [DXVAVDA] Add a vectored exception handler so a dump is reported on driver error. The MS VP9 MFT swallows driver exceptions and later hangs because it gets into a weird state. Instead the vectored exception handler can upload a crash dump so we can get more actionable feedback. BUG=636158 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel ==========
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_...)
PTAL. I've made it only dump instead of crash, so we can get more information on if this would affect anything unexpected. If we later want to expand this to actually crashing we can probably restrict it to access violation exceptions, as those hopefully aren't intentional. Also it's now using TLS to ensure the handler only does something when it's running on the expected thread.
lgtm https://codereview.chromium.org/2559633002/diff/60001/media/gpu/dxva_video_de... File media/gpu/dxva_video_decode_accelerator_win.cc (right): https://codereview.chromium.org/2559633002/diff/60001/media/gpu/dxva_video_de... media/gpu/dxva_video_decode_accelerator_win.cc:213: handler_ = AddVectoredExceptionHandler(TRUE, &VectoredCrashHandler); 1 instead of TRUE since it's documented as ULONG.
The CQ bit was checked by jbauman@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...
jbauman@chromium.org changed reviewers: + sandersd@chromium.org
Also sandersd@ as media/ OWNERS.
lgtm
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 jbauman@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from scottmg@chromium.org Link to the patchset: https://codereview.chromium.org/2559633002/#ps80001 (title: "change to 1")
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": 80001, "attempt_start_ts": 1481320284729940,
"parent_rev": "62b9c0da94d887bc7c672a00c35f3163d0a05bb2", "commit_rev":
"a2de9ad36ccce80d294db2c2ce8a0f28633c295e"}
Message was sent while issue was closed.
Description was changed from ========== [DXVAVDA] Add a vectored exception handler so a dump is reported on driver error. The MS VP9 MFT swallows driver exceptions and later hangs because it gets into a weird state. Instead the vectored exception handler can upload a crash dump so we can get more actionable feedback. BUG=636158 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel ========== to ========== [DXVAVDA] Add a vectored exception handler so a dump is reported on driver error. The MS VP9 MFT swallows driver exceptions and later hangs because it gets into a weird state. Instead the vectored exception handler can upload a crash dump so we can get more actionable feedback. BUG=636158 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel Review-Url: https://codereview.chromium.org/2559633002 ==========
Message was sent while issue was closed.
Committed patchset #5 (id:80001)
Message was sent while issue was closed.
Description was changed from ========== [DXVAVDA] Add a vectored exception handler so a dump is reported on driver error. The MS VP9 MFT swallows driver exceptions and later hangs because it gets into a weird state. Instead the vectored exception handler can upload a crash dump so we can get more actionable feedback. BUG=636158 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel Review-Url: https://codereview.chromium.org/2559633002 ========== to ========== [DXVAVDA] Add a vectored exception handler so a dump is reported on driver error. The MS VP9 MFT swallows driver exceptions and later hangs because it gets into a weird state. Instead the vectored exception handler can upload a crash dump so we can get more actionable feedback. BUG=636158 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel Committed: https://crrev.com/3a43b1fab59f70110154fd062ba908605638188a Cr-Commit-Position: refs/heads/master@{#437667} ==========
Message was sent while issue was closed.
Patchset 5 (id:??) landed as https://crrev.com/3a43b1fab59f70110154fd062ba908605638188a Cr-Commit-Position: refs/heads/master@{#437667} |
