Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2782603002:
Enable content shell crash integration test on Windows (Closed)
|
Created:
3 years, 8 months ago by jochen (gone - plz use gerrit) Modified:
3 years, 8 months ago CC:
chromium-reviews, darin-cc_chromium.org, jam, jochen+watch_chromium.org, mlamouri+watch-content_chromium.org, Peter Beverloo Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
DescriptionEnable content shell crash integration test on Windows
BUG=688737, 706744
R=dpranke@chromium.org
Review-Url: https://codereview.chromium.org/2782603002
Cr-Commit-Position: refs/heads/master@{#461175}
Committed: https://chromium.googlesource.com/chromium/src/+/2511b73c11a55d8bb8a4c730d0cb308fbc821780
Patch Set 1 #Patch Set 2 : updates #Patch Set 3 : typo #
Total comments: 9
Patch Set 4 : updates #Patch Set 5 : updates #
Total comments: 2
Messages
Total messages: 33 (17 generated)
The CQ bit was checked by jochen@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...
ptal
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
jochen@chromium.org changed reviewers: + scottmg@chromium.org
meh, the stack cdb extracts from the minidump is wrong.. on https://chromium-swarm.appspot.com/task?id=35322cdcd35e4010&refresh=10&show_r... the reported stack is 002adc14 0320fbb4 content_shell!mojo::internal::InterfacePtrState<media::mojom::RemoterFactory>::ConfigureProxyIfNecessary+0x2ad 002adce0 03215da2 content_shell!content::RenderFrameImpl::MaybeEnableMojoBindings+0x2d3 002addcc 0320ffb0 content_shell!content::RenderFrameImpl::PrepareRenderViewForNavigation+0x60 002adfdc 03213d09 content_shell!content::RenderFrameImpl::NavigateInternal+0xab 002ae0c8 03209703 content_shell!content::RenderFrameImpl::OnNavigate+0xe9 ... However, PrepareRenderViewForNavigation invokes MaybeHandleDebugURL and not MaybeEnableMojoBindings what do?
On 2017/03/29 09:39:23, jochen wrote: > meh, the stack cdb extracts from the minidump is wrong.. > > on > https://chromium-swarm.appspot.com/task?id=35322cdcd35e4010&refresh=10&show_r... > the reported stack is > > 002adc14 0320fbb4 > content_shell!mojo::internal::InterfacePtrState<media::mojom::RemoterFactory>::ConfigureProxyIfNecessary+0x2ad > 002adce0 03215da2 > content_shell!content::RenderFrameImpl::MaybeEnableMojoBindings+0x2d3 > 002addcc 0320ffb0 > content_shell!content::RenderFrameImpl::PrepareRenderViewForNavigation+0x60 > 002adfdc 03213d09 content_shell!content::RenderFrameImpl::NavigateInternal+0xab > 002ae0c8 03209703 content_shell!content::RenderFrameImpl::OnNavigate+0xe9 > ... > > However, PrepareRenderViewForNavigation invokes MaybeHandleDebugURL and not > MaybeEnableMojoBindings > > what do? I'm not sure what's wrong. I notice that despite saying ".lines" to cdb you're not getting any line number information. This makes me suspect something like a missing pdb, or, I think more likely, that the wrong (old/OS-suppplied) version of dbghelp.dll is getting loaded. I'd try getting the .dmp and looking at it locally instead if possible to rule that out.
https://codereview.chromium.org/2782603002/diff/40001/build/vs_toolchain.py File build/vs_toolchain.py (left): https://codereview.chromium.org/2782603002/diff/40001/build/vs_toolchain.py#o... build/vs_toolchain.py:396: def SetEnvironmentAndGetSDKDir(): This is used from tools/win/setenv.py, can you update that script as well? https://codereview.chromium.org/2782603002/diff/40001/content/shell/tools/bre... File content/shell/tools/breakpad_integration_test.py (right): https://codereview.chromium.org/2782603002/diff/40001/content/shell/tools/bre... content/shell/tools/breakpad_integration_test.py:130: cdb_exe = os.path.join(options.build_dir, 'cdb', 'cdb.exe') you might also try chdiring to the $build_dir/cdb before running to "encourage" the right dll to load if that's easier than getting the dmp file out of a swarming bot.
On 2017/03/29 23:17:39, scottmg wrote: > https://codereview.chromium.org/2782603002/diff/40001/build/vs_toolchain.py > File build/vs_toolchain.py (left): > > https://codereview.chromium.org/2782603002/diff/40001/build/vs_toolchain.py#o... > build/vs_toolchain.py:396: def SetEnvironmentAndGetSDKDir(): > This is used from tools/win/setenv.py, can you update that script as well? > > https://codereview.chromium.org/2782603002/diff/40001/content/shell/tools/bre... > File content/shell/tools/breakpad_integration_test.py (right): > > https://codereview.chromium.org/2782603002/diff/40001/content/shell/tools/bre... > content/shell/tools/breakpad_integration_test.py:130: cdb_exe = > os.path.join(options.build_dir, 'cdb', 'cdb.exe') > you might also try chdiring to the $build_dir/cdb before running to "encourage" > the right dll to load if that's easier than getting the dmp file out of a > swarming bot. (I tried that here https://codereview.chromium.org/2786823002 but I didn't wait to see if it helped yet.)
On 2017/03/30 00:37:29, scottmg wrote: > On 2017/03/29 23:17:39, scottmg wrote: > > https://codereview.chromium.org/2782603002/diff/40001/build/vs_toolchain.py > > File build/vs_toolchain.py (left): > > > > > https://codereview.chromium.org/2782603002/diff/40001/build/vs_toolchain.py#o... > > build/vs_toolchain.py:396: def SetEnvironmentAndGetSDKDir(): > > This is used from tools/win/setenv.py, can you update that script as well? > > > > > https://codereview.chromium.org/2782603002/diff/40001/content/shell/tools/bre... > > File content/shell/tools/breakpad_integration_test.py (right): > > > > > https://codereview.chromium.org/2782603002/diff/40001/content/shell/tools/bre... > > content/shell/tools/breakpad_integration_test.py:130: cdb_exe = > > os.path.join(options.build_dir, 'cdb', 'cdb.exe') > > you might also try chdiring to the $build_dir/cdb before running to > "encourage" > > the right dll to load if that's easier than getting the dmp file out of a > > swarming bot. > > (I tried that here https://codereview.chromium.org/2786823002 but I didn't wait > to see if it helped yet.) Didn't seem to make any difference. Hmm...
On 2017/03/30 01:34:22, scottmg wrote: > On 2017/03/30 00:37:29, scottmg wrote: > > On 2017/03/29 23:17:39, scottmg wrote: > > > https://codereview.chromium.org/2782603002/diff/40001/build/vs_toolchain.py > > > File build/vs_toolchain.py (left): > > > > > > > > > https://codereview.chromium.org/2782603002/diff/40001/build/vs_toolchain.py#o... > > > build/vs_toolchain.py:396: def SetEnvironmentAndGetSDKDir(): > > > This is used from tools/win/setenv.py, can you update that script as well? > > > > > > > > > https://codereview.chromium.org/2782603002/diff/40001/content/shell/tools/bre... > > > File content/shell/tools/breakpad_integration_test.py (right): > > > > > > > > > https://codereview.chromium.org/2782603002/diff/40001/content/shell/tools/bre... > > > content/shell/tools/breakpad_integration_test.py:130: cdb_exe = > > > os.path.join(options.build_dir, 'cdb', 'cdb.exe') > > > you might also try chdiring to the $build_dir/cdb before running to > > "encourage" > > > the right dll to load if that's easier than getting the dmp file out of a > > > swarming bot. > > > > (I tried that here https://codereview.chromium.org/2786823002 but I didn't > wait > > to see if it helped yet.) > > Didn't seem to make any difference. Hmm... Huh, I get the same thing if I run locally on a Win7 VM. Yuck.
On 2017/03/30 at 01:42:33, scottmg wrote: > On 2017/03/30 01:34:22, scottmg wrote: > > On 2017/03/30 00:37:29, scottmg wrote: > > > On 2017/03/29 23:17:39, scottmg wrote: > > > > https://codereview.chromium.org/2782603002/diff/40001/build/vs_toolchain.py > > > > File build/vs_toolchain.py (left): > > > > > > > > > > > > > https://codereview.chromium.org/2782603002/diff/40001/build/vs_toolchain.py#o... > > > > build/vs_toolchain.py:396: def SetEnvironmentAndGetSDKDir(): > > > > This is used from tools/win/setenv.py, can you update that script as well? > > > > > > > > > > > > > https://codereview.chromium.org/2782603002/diff/40001/content/shell/tools/bre... > > > > File content/shell/tools/breakpad_integration_test.py (right): > > > > > > > > > > > > > https://codereview.chromium.org/2782603002/diff/40001/content/shell/tools/bre... > > > > content/shell/tools/breakpad_integration_test.py:130: cdb_exe = > > > > os.path.join(options.build_dir, 'cdb', 'cdb.exe') > > > > you might also try chdiring to the $build_dir/cdb before running to > > > "encourage" > > > > the right dll to load if that's easier than getting the dmp file out of a > > > > swarming bot. > > > > > > (I tried that here https://codereview.chromium.org/2786823002 but I didn't > > wait > > > to see if it helped yet.) > > > > Didn't seem to make any difference. Hmm... > > Huh, I get the same thing if I run locally on a Win7 VM. Yuck. filed a bug to track this: https://bugs.chromium.org/p/chromium/issues/detail?id=706744
https://codereview.chromium.org/2782603002/diff/40001/build/win/copy_cdb_to_o... File build/win/copy_cdb_to_output/BUILD.gn (right): https://codereview.chromium.org/2782603002/diff/40001/build/win/copy_cdb_to_o... build/win/copy_cdb_to_output/BUILD.gn:1: # Copyright 2017 The Chromium Authors. All rights reserved. Why create a whole new directory for this? Why not just put this target in //build/win:copy_cdb_to_output? https://codereview.chromium.org/2782603002/diff/40001/content/shell/BUILD.gn File content/shell/BUILD.gn (right): https://codereview.chromium.org/2782603002/diff/40001/content/shell/BUILD.gn#... content/shell/BUILD.gn:798: "//build/win/copy_cdb_to_output:copy_cdb_to_output", Nit: GN style would be just "//build/win/copy_cdb_to_output" since the target matches the directory. https://codereview.chromium.org/2782603002/diff/40001/content/shell/BUILD.gn#... content/shell/BUILD.gn:801: # TODO(GYP): These should be provided automatically through data_deps. Are you sure these aren't automatically provided now? https://codereview.chromium.org/2782603002/diff/40001/tools/perf/chrome_telem... File tools/perf/chrome_telemetry_build/BUILD.gn (right): https://codereview.chromium.org/2782603002/diff/40001/tools/perf/chrome_telem... tools/perf/chrome_telemetry_build/BUILD.gn:47: "//build/win/copy_cdb_to_output:copy_cdb_to_output", same nit.
The CQ bit was checked by jochen@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-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)
The CQ bit was checked by jochen@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.
all done, and the test passes now ptal https://codereview.chromium.org/2782603002/diff/40001/build/vs_toolchain.py File build/vs_toolchain.py (left): https://codereview.chromium.org/2782603002/diff/40001/build/vs_toolchain.py#o... build/vs_toolchain.py:396: def SetEnvironmentAndGetSDKDir(): On 2017/03/29 at 23:17:39, scottmg wrote: > This is used from tools/win/setenv.py, can you update that script as well? I left this method in instead https://codereview.chromium.org/2782603002/diff/40001/build/win/copy_cdb_to_o... File build/win/copy_cdb_to_output/BUILD.gn (right): https://codereview.chromium.org/2782603002/diff/40001/build/win/copy_cdb_to_o... build/win/copy_cdb_to_output/BUILD.gn:1: # Copyright 2017 The Chromium Authors. All rights reserved. On 2017/03/30 at 18:28:23, Dirk Pranke wrote: > Why create a whole new directory for this? Why not just put this target in //build/win:copy_cdb_to_output? done https://codereview.chromium.org/2782603002/diff/40001/content/shell/BUILD.gn File content/shell/BUILD.gn (right): https://codereview.chromium.org/2782603002/diff/40001/content/shell/BUILD.gn#... content/shell/BUILD.gn:801: # TODO(GYP): These should be provided automatically through data_deps. On 2017/03/30 at 18:28:23, Dirk Pranke wrote: > Are you sure these aren't automatically provided now? I just copied this from the telemetry BUILD.gn file, let's try without
lgtm https://codereview.chromium.org/2782603002/diff/80001/build/win/BUILD.gn File build/win/BUILD.gn (right): https://codereview.chromium.org/2782603002/diff/80001/build/win/BUILD.gn#newc... build/win/BUILD.gn:18: if (is_win) { Nit: it seems like the `if (is_win)` should be redundant here, and maybe we should have an `assert(is_win)` at the top of the file instead.
https://codereview.chromium.org/2782603002/diff/80001/build/win/BUILD.gn File build/win/BUILD.gn (right): https://codereview.chromium.org/2782603002/diff/80001/build/win/BUILD.gn#newc... build/win/BUILD.gn:18: if (is_win) { On 2017/03/31 at 17:46:16, Dirk Pranke wrote: > Nit: it seems like the `if (is_win)` should be redundant here, and maybe we should have an `assert(is_win)` at the top of the file instead. See patchset 4 - the all target appears to execute this action on all platforms :/
The CQ bit was checked by jochen@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
lgtm
Description was changed from ========== Enable content shell crash integration test on Windows BUG=688737 R=dpranke@chromium.org ========== to ========== Enable content shell crash integration test on Windows BUG=688737,706744 R=dpranke@chromium.org ==========
CQ is committing da patch.
Bot data: {"patchset_id": 80001, "attempt_start_ts": 1490984223887210,
"parent_rev": "9ae73f4ee60f60cbe72e79d2684b2295571eec53", "commit_rev":
"2511b73c11a55d8bb8a4c730d0cb308fbc821780"}
Message was sent while issue was closed.
Description was changed from ========== Enable content shell crash integration test on Windows BUG=688737,706744 R=dpranke@chromium.org ========== to ========== Enable content shell crash integration test on Windows BUG=688737,706744 R=dpranke@chromium.org Review-Url: https://codereview.chromium.org/2782603002 Cr-Commit-Position: refs/heads/master@{#461175} Committed: https://chromium.googlesource.com/chromium/src/+/2511b73c11a55d8bb8a4c730d0cb... ==========
Message was sent while issue was closed.
Committed patchset #5 (id:80001) as https://chromium.googlesource.com/chromium/src/+/2511b73c11a55d8bb8a4c730d0cb...
Message was sent while issue was closed.
On 2017/03/31 17:58:41, jochen (slow until Apr 4) wrote: > https://codereview.chromium.org/2782603002/diff/80001/build/win/BUILD.gn > File build/win/BUILD.gn (right): > > https://codereview.chromium.org/2782603002/diff/80001/build/win/BUILD.gn#newc... > build/win/BUILD.gn:18: if (is_win) { > On 2017/03/31 at 17:46:16, Dirk Pranke wrote: > > Nit: it seems like the `if (is_win)` should be redundant here, and maybe we > should have an `assert(is_win)` at the top of the file instead. > > See patchset 4 - the all target appears to execute this action on all platforms > :/ Filed https://bugs.chromium.org/p/chromium/issues/detail?id=707331 for this. |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
