Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1863393006:
MacViews: Fix MenuRunnerTests which fail with asan enabled. (Closed)
|
Created:
4 years, 8 months ago by karandeepb Modified:
4 years, 8 months ago CC:
chromium-reviews, tfarina, chrome-apps-syd-reviews_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionMacViews: Fix MenuRunnerTests which fail with asan enabled.
This CL fixes the following tests which fail due to use-after-free errors when
asan is enabled -
-MenuRunnerTest.AsynchronousKeyEventHandling
-MenuRunnerTest.LatinMnemonic
-MenuRunnerTest.NonLatinMnemonic
These fail on the events dispatched by ui::test::EventGenerator. The reason for
the failures is that the call to TerminateNestedMessageLoopIfNecessary() in
MenuController::OnWillDispatchKeyEvent is unsafe, since the MenuController might
by deleted by the calls to SelectByChar(..) or OnKeyDown(..). These failures are
not experienced on platforms other than Mac, due to the differences in event
processing pipelines.
This CL fixes the tests by checking that the current MenuController instance is
alive before calling TerminateNestedMessageLoopIfNecessary().
BUG=601315
Committed: https://crrev.com/aa336ae9f9b653825af2c70bdf59593fd58e886a
Cr-Commit-Position: refs/heads/master@{#386323}
Patch Set 1 #
Messages
Total messages: 14 (8 generated)
Description was changed from ========== ---- ========== to ========== MacViews: Fix MenuRunnerTests which fail with asan enabled. This CL fixes the following tests which fail when asan is enabled - -MenuRunnerTest.AsynchronousKeyEventHandling -MenuRunnerTest.LatinMnemonic -MenuRunnerTest.NonLatinMnemonic These fail since the call to TerminateNestedMessageLoopIfNecessary() in MenuController::OnWillDispatchKeyEvent is unsafe, since the MenuController might by deleted by the calls to SelectByChar(..) or OnKeyDown(..). These failures are not experienced on platforms other than Mac, due to the differences in event processing pipelines. This CL fixes the tests by checking that the current MenuController instance is alive before calling TerminateNestedMessageLoopIfNecessary(). BUG=601315 ==========
Description was changed from ========== MacViews: Fix MenuRunnerTests which fail with asan enabled. This CL fixes the following tests which fail when asan is enabled - -MenuRunnerTest.AsynchronousKeyEventHandling -MenuRunnerTest.LatinMnemonic -MenuRunnerTest.NonLatinMnemonic These fail since the call to TerminateNestedMessageLoopIfNecessary() in MenuController::OnWillDispatchKeyEvent is unsafe, since the MenuController might by deleted by the calls to SelectByChar(..) or OnKeyDown(..). These failures are not experienced on platforms other than Mac, due to the differences in event processing pipelines. This CL fixes the tests by checking that the current MenuController instance is alive before calling TerminateNestedMessageLoopIfNecessary(). BUG=601315 ========== to ========== MacViews: Fix MenuRunnerTests which fail with asan enabled. This CL fixes the following tests which fail when asan is enabled - -MenuRunnerTest.AsynchronousKeyEventHandling -MenuRunnerTest.LatinMnemonic -MenuRunnerTest.NonLatinMnemonic These fail since the call to TerminateNestedMessageLoopIfNecessary() in MenuController::OnWillDispatchKeyEvent is unsafe, since the MenuController might by deleted by the calls to SelectByChar(..) or OnKeyDown(..). These failures are not experienced on platforms other than Mac, due to the differences in event processing pipelines. This CL fixes the tests by checking that the current MenuController instance is alive before calling TerminateNestedMessageLoopIfNecessary(). BUG=601315 ==========
Description was changed from ========== MacViews: Fix MenuRunnerTests which fail with asan enabled. This CL fixes the following tests which fail when asan is enabled - -MenuRunnerTest.AsynchronousKeyEventHandling -MenuRunnerTest.LatinMnemonic -MenuRunnerTest.NonLatinMnemonic These fail since the call to TerminateNestedMessageLoopIfNecessary() in MenuController::OnWillDispatchKeyEvent is unsafe, since the MenuController might by deleted by the calls to SelectByChar(..) or OnKeyDown(..). These failures are not experienced on platforms other than Mac, due to the differences in event processing pipelines. This CL fixes the tests by checking that the current MenuController instance is alive before calling TerminateNestedMessageLoopIfNecessary(). BUG=601315 ========== to ========== MacViews: Fix MenuRunnerTests which fail with asan enabled. This CL fixes the following tests which fail due to use-after-free errors when asan is enabled - -MenuRunnerTest.AsynchronousKeyEventHandling -MenuRunnerTest.LatinMnemonic -MenuRunnerTest.NonLatinMnemonic These fail on the events dispatched by ui::test::EventGenerator. The reason for the failures is that the call to TerminateNestedMessageLoopIfNecessary() in MenuController::OnWillDispatchKeyEvent is unsafe, since the MenuController might by deleted by the calls to SelectByChar(..) or OnKeyDown(..). These failures are not experienced on platforms other than Mac, due to the differences in event processing pipelines. This CL fixes the tests by checking that the current MenuController instance is alive before calling TerminateNestedMessageLoopIfNecessary(). BUG=601315 ==========
karandeepb@chromium.org changed reviewers: + tapted@chromium.org
PTAL Trent. As I said in the CL description, these tests don't fail on other platforms due to difference in event processing pipelines. BridgedContentView passes the event to MenuController::OnWillDispatchKeyEvent on Mac, while on Linux it is passed to MenuKeyEventHandler::OnKeyEvent by EventHandler::OnEvent. Can you confirm whether this is ok?
karandeepb@chromium.org changed reviewers: + sky@chromium.org
PTAL sky@. I think that checking GetActiveInstance() before accessing member variables is safe only when the instance on which the method is invoked is the active instance currently. I am not sure if this is always the case. Should I add a DCHECK(GetActiveInstance() == this) at the start of the methods using this strategy?
LGTM
The CQ bit was checked by karandeepb@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1863393006/1 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1863393006/1
Message was sent while issue was closed.
Description was changed from ========== MacViews: Fix MenuRunnerTests which fail with asan enabled. This CL fixes the following tests which fail due to use-after-free errors when asan is enabled - -MenuRunnerTest.AsynchronousKeyEventHandling -MenuRunnerTest.LatinMnemonic -MenuRunnerTest.NonLatinMnemonic These fail on the events dispatched by ui::test::EventGenerator. The reason for the failures is that the call to TerminateNestedMessageLoopIfNecessary() in MenuController::OnWillDispatchKeyEvent is unsafe, since the MenuController might by deleted by the calls to SelectByChar(..) or OnKeyDown(..). These failures are not experienced on platforms other than Mac, due to the differences in event processing pipelines. This CL fixes the tests by checking that the current MenuController instance is alive before calling TerminateNestedMessageLoopIfNecessary(). BUG=601315 ========== to ========== MacViews: Fix MenuRunnerTests which fail with asan enabled. This CL fixes the following tests which fail due to use-after-free errors when asan is enabled - -MenuRunnerTest.AsynchronousKeyEventHandling -MenuRunnerTest.LatinMnemonic -MenuRunnerTest.NonLatinMnemonic These fail on the events dispatched by ui::test::EventGenerator. The reason for the failures is that the call to TerminateNestedMessageLoopIfNecessary() in MenuController::OnWillDispatchKeyEvent is unsafe, since the MenuController might by deleted by the calls to SelectByChar(..) or OnKeyDown(..). These failures are not experienced on platforms other than Mac, due to the differences in event processing pipelines. This CL fixes the tests by checking that the current MenuController instance is alive before calling TerminateNestedMessageLoopIfNecessary(). BUG=601315 ==========
Message was sent while issue was closed.
Committed patchset #1 (id:1)
Message was sent while issue was closed.
Description was changed from ========== MacViews: Fix MenuRunnerTests which fail with asan enabled. This CL fixes the following tests which fail due to use-after-free errors when asan is enabled - -MenuRunnerTest.AsynchronousKeyEventHandling -MenuRunnerTest.LatinMnemonic -MenuRunnerTest.NonLatinMnemonic These fail on the events dispatched by ui::test::EventGenerator. The reason for the failures is that the call to TerminateNestedMessageLoopIfNecessary() in MenuController::OnWillDispatchKeyEvent is unsafe, since the MenuController might by deleted by the calls to SelectByChar(..) or OnKeyDown(..). These failures are not experienced on platforms other than Mac, due to the differences in event processing pipelines. This CL fixes the tests by checking that the current MenuController instance is alive before calling TerminateNestedMessageLoopIfNecessary(). BUG=601315 ========== to ========== MacViews: Fix MenuRunnerTests which fail with asan enabled. This CL fixes the following tests which fail due to use-after-free errors when asan is enabled - -MenuRunnerTest.AsynchronousKeyEventHandling -MenuRunnerTest.LatinMnemonic -MenuRunnerTest.NonLatinMnemonic These fail on the events dispatched by ui::test::EventGenerator. The reason for the failures is that the call to TerminateNestedMessageLoopIfNecessary() in MenuController::OnWillDispatchKeyEvent is unsafe, since the MenuController might by deleted by the calls to SelectByChar(..) or OnKeyDown(..). These failures are not experienced on platforms other than Mac, due to the differences in event processing pipelines. This CL fixes the tests by checking that the current MenuController instance is alive before calling TerminateNestedMessageLoopIfNecessary(). BUG=601315 Committed: https://crrev.com/aa336ae9f9b653825af2c70bdf59593fd58e886a Cr-Commit-Position: refs/heads/master@{#386323} ==========
Message was sent while issue was closed.
Patchset 1 (id:??) landed as https://crrev.com/aa336ae9f9b653825af2c70bdf59593fd58e886a Cr-Commit-Position: refs/heads/master@{#386323} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
