Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
661943002:
Content Shell: Fix broken test controller lifetime. (Closed)
DescriptionContent Shell: Fix broken test controller lifetime.
BUG=424216, 420994
NOTRY=true
Committed: https://crrev.com/7f0bd61a5ff90139ecda25f7788349d66534ae8d
Cr-Commit-Position: refs/heads/master@{#300213}
Patch Set 1 #
Total comments: 3
Patch Set 2 : Fixing nits. #
Total comments: 1
Messages
Total messages: 17 (6 generated)
mkwst@chromium.org changed reviewers: + jochen@chromium.org
Mind taking a look, Jochen? I've CCd you on the clusterfuzz bug. https://codereview.chromium.org/661943002/diff/1/content/shell/browser/layout... File content/shell/browser/layout_test/layout_test_browser_main.cc (right): https://codereview.chromium.org/661943002/diff/1/content/shell/browser/layout... content/shell/browser/layout_test/layout_test_browser_main.cc:153: Oops. https://codereview.chromium.org/661943002/diff/1/content/shell/browser/layout... content/shell/browser/layout_test/layout_test_browser_main.cc:171: << "BrowserMainRunner::Initialize failed in ShellBrowserMain"; Oops. https://codereview.chromium.org/661943002/diff/1/content/shell/browser/layout... content/shell/browser/layout_test/layout_test_browser_main.cc:189: { Might make sense to move this to a separate function... Just testing whether it solves the problem on the bots.
mkwst@chromium.org changed reviewers: + peter@chromium.org
Peter, you might be interested in this too.
(It does appear to make the ASAN bot happy. Yay!)
Patchset #2 (id:20001) has been deleted
lgtm. Might want to check whether this breaks Android or not (we don't run layout tests anywhere on try-bots). I can do so on Monday, too. https://codereview.chromium.org/661943002/diff/40001/content/shell/browser/la... File content/shell/browser/layout_test/layout_test_browser_main.cc (right): https://codereview.chromium.org/661943002/diff/40001/content/shell/browser/la... content/shell/browser/layout_test/layout_test_browser_main.cc:222: exit_code = RunTests(main_runner); Hmm. RunTests never returns anything that's non-zero, but it's common for RunTests()-like functions to return an int so that's OK for consistency.
On 2014/10/18 09:17:58, Peter Beverloo wrote: > lgtm. > > Might want to check whether this breaks Android or not (we don't run layout > tests anywhere on try-bots). I can do so on Monday, too. I'm going to land this now to clean up the various Use-After-Free bugs this caused, and to prevent DCHECKs in various bits of //content after running content_shell. Let's sync up on Monday to see if this fix works as expected for the android bits of the system. > https://codereview.chromium.org/661943002/diff/40001/content/shell/browser/la... > File content/shell/browser/layout_test/layout_test_browser_main.cc (right): > > https://codereview.chromium.org/661943002/diff/40001/content/shell/browser/la... > content/shell/browser/layout_test/layout_test_browser_main.cc:222: exit_code = > RunTests(main_runner); > Hmm. RunTests never returns anything that's non-zero, but it's common for > RunTests()-like functions to return an int so that's OK for consistency. That was my thought. It looked weird to simply assign 0 to exit_code after running RunTests, so I made RunTests do that for me. *shrug*
The CQ bit was checked by mkwst@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/661943002/40001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: chromium_presubmit on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
I'm going to NOTRY this; this patch doesn't introduce ScopedAllowIO, it simply
moves it from one bad place to another bad place. Killing it entirely will have
to wait.
** Presubmit ERRORS **
Banned functions were used.
content/shell/browser/layout_test/layout_test_browser_main.cc:154:
New code should not use ScopedAllowIO. Post a task to the blocking
pool or the FILE thread instead.
The CQ bit was checked by mkwst@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/661943002/40001
Message was sent while issue was closed.
Committed patchset #2 (id:40001)
Message was sent while issue was closed.
Patchset 2 (id:??) landed as https://crrev.com/7f0bd61a5ff90139ecda25f7788349d66534ae8d Cr-Commit-Position: refs/heads/master@{#300213} |
