Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
12328030:
Magnifier: add validation of root window. (Closed)
|
Created:
7 years, 10 months ago by yoshiki Modified:
7 years, 9 months ago Reviewers:
oshima CC:
chromium-reviews, sadrul, ben+watch_chromium.org Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionMagnifier: add validation of root window.
Changes:
- Adds null check where root_window_ is used because |root_window_| became nullable.
- Adds validation if the root window is valid on public methods. If the root window is invalid, uses the primary root window. If by chance there are no root window, the method returns immediately.
- Fixes a bug in OnMouseEvent(), changing the order of assigning to |point_of_interest_|.
BUG=176335
TEST=none
Patch Set 1 #Patch Set 2 : review fix #
Total comments: 10
Patch Set 3 : review fix #Patch Set 4 : fix crash #Patch Set 5 : add comment. #
Total comments: 1
Patch Set 6 : review fix #Patch Set 7 : review fix #
Total comments: 8
Messages
Total messages: 13 (0 generated)
@oshima: Could you take a look? Thanks,
On 2013/02/21 04:20:21, yoshiki wrote: > @oshima: Could you take a look? Thanks, Can you just listen to (root window's) WIndowObserver::OnWindowDestroying and reset the root_window_? Also note that all root windows will be gone at shutdown so make sure the shutdown process is handled properly.
On 2013/02/21 04:32:42, oshima wrote: > On 2013/02/21 04:20:21, yoshiki wrote: > > @oshima: Could you take a look? Thanks, > > Can you just listen to (root window's) WIndowObserver::OnWindowDestroying and > reset the root_window_? > > Also note that all root windows will be gone at shutdown so make > sure the shutdown process is handled properly. Thanks for advice. I have used OnWindowDestroying(). Now |root_window_| becomes nullable, so I added null-check where |root_window_| is used.
https://codereview.chromium.org/12328030/diff/4002/ash/magnifier/magnificatio... File ash/magnifier/magnification_controller.cc (right): https://codereview.chromium.org/12328030/diff/4002/ash/magnifier/magnificatio... ash/magnifier/magnification_controller.cc:53: } Just return std::find(...) != root_windows.end(); https://codereview.chromium.org/12328030/diff/4002/ash/magnifier/magnificatio... ash/magnifier/magnification_controller.cc:458: root_window_ = NULL; Shouldn't you switch the root window here? This way, can't you assume root_window_ is never been null elsewhere? https://codereview.chromium.org/12328030/diff/4002/ash/magnifier/magnificatio... ash/magnifier/magnification_controller.cc:470: root_window_->RemoveObserver(this); root_window_ can be NULL at this point, can't it? (with current flow) https://codereview.chromium.org/12328030/diff/4002/ash/magnifier/magnificatio... ash/magnifier/magnification_controller.cc:474: if (IsRootWindowValid(new_root_window)) { Looking through call flow, this should never be false. https://codereview.chromium.org/12328030/diff/4002/ash/magnifier/magnificatio... ash/magnifier/magnification_controller.cc:483: bool MagnificationControllerImpl::IsRootwindowStale() { Changing a state inside predicate style method like IsXXX is confusing. If you need to keep this method, I'd sugges something like EnsureValidRootWindow or something like that. My recommendation is to switch the root in OnWindowDestorying (or Destroyed if it works), and eliminate this method if possible.
@oshima: PTAL https://codereview.chromium.org/12328030/diff/4002/ash/magnifier/magnificatio... File ash/magnifier/magnification_controller.cc (right): https://codereview.chromium.org/12328030/diff/4002/ash/magnifier/magnificatio... ash/magnifier/magnification_controller.cc:53: } On 2013/02/21 16:45:06, oshima wrote: > Just > > return std::find(...) != root_windows.end(); Done. https://codereview.chromium.org/12328030/diff/4002/ash/magnifier/magnificatio... ash/magnifier/magnification_controller.cc:458: root_window_ = NULL; On 2013/02/21 16:45:06, oshima wrote: > Shouldn't you switch the root window here? > > This way, can't you assume root_window_ is never been null elsewhere? Done. Switching root window here. https://codereview.chromium.org/12328030/diff/4002/ash/magnifier/magnificatio... ash/magnifier/magnification_controller.cc:470: root_window_->RemoveObserver(this); On 2013/02/21 16:45:06, oshima wrote: > root_window_ can be NULL at this point, can't it? (with current flow) Done. I've added the check. https://codereview.chromium.org/12328030/diff/4002/ash/magnifier/magnificatio... ash/magnifier/magnification_controller.cc:474: if (IsRootWindowValid(new_root_window)) { On 2013/02/21 16:45:06, oshima wrote: > Looking through call flow, this should never be false. Done. https://codereview.chromium.org/12328030/diff/4002/ash/magnifier/magnificatio... ash/magnifier/magnification_controller.cc:483: bool MagnificationControllerImpl::IsRootwindowStale() { On 2013/02/21 16:45:06, oshima wrote: > Changing a state inside predicate style method like IsXXX > is confusing. If you need to keep this method, I'd sugges > something like > EnsureValidRootWindow or something like that. My recommendation > is to switch the root in OnWindowDestorying (or Destroyed if it works), and > eliminate this method if possible. Removed.
On 2013/02/25 12:32:44, yoshiki wrote: > @oshima: PTAL > > https://codereview.chromium.org/12328030/diff/4002/ash/magnifier/magnificatio... > File ash/magnifier/magnification_controller.cc (right): > > https://codereview.chromium.org/12328030/diff/4002/ash/magnifier/magnificatio... > ash/magnifier/magnification_controller.cc:53: } > On 2013/02/21 16:45:06, oshima wrote: > > Just > > > > return std::find(...) != root_windows.end(); > > Done. > > https://codereview.chromium.org/12328030/diff/4002/ash/magnifier/magnificatio... > ash/magnifier/magnification_controller.cc:458: root_window_ = NULL; > On 2013/02/21 16:45:06, oshima wrote: > > Shouldn't you switch the root window here? > > > > This way, can't you assume root_window_ is never been null elsewhere? > > Done. Switching root window here. > > https://codereview.chromium.org/12328030/diff/4002/ash/magnifier/magnificatio... > ash/magnifier/magnification_controller.cc:470: > root_window_->RemoveObserver(this); > On 2013/02/21 16:45:06, oshima wrote: > > root_window_ can be NULL at this point, can't it? (with current flow) > > Done. I've added the check. > > https://codereview.chromium.org/12328030/diff/4002/ash/magnifier/magnificatio... > ash/magnifier/magnification_controller.cc:474: if > (IsRootWindowValid(new_root_window)) { > On 2013/02/21 16:45:06, oshima wrote: > > Looking through call flow, this should never be false. > > Done. > > https://codereview.chromium.org/12328030/diff/4002/ash/magnifier/magnificatio... > ash/magnifier/magnification_controller.cc:483: bool > MagnificationControllerImpl::IsRootwindowStale() { > On 2013/02/21 16:45:06, oshima wrote: > > Changing a state inside predicate style method like IsXXX > > is confusing. If you need to keep this method, I'd sugges > > something like > > EnsureValidRootWindow or something like that. My recommendation > > is to switch the root in OnWindowDestorying (or Destroyed if it works), and > > eliminate this method if possible. > > Removed. This is probably related from a glace (not 100% sure). But after apply this patch locally, I get a crash with the following stacktrace when running my test in interactive_ui_tests [0x7fdc2d9d6c50] base::debug::StackTrace::StackTrace() [0x7fdc2d9d6557] base::debug::(anonymous namespace)::StackDumpSignalHandler() [0x7fdc212bdcb0] <unknown> [0x7fdc26939263] aura::Window::SetPropertyInternal() [0x7fdc2c569634] aura::Window::SetProperty<>() [0x7fdc2c5b43cd] ash::internal::RootWindowController::CloseChildWindows() [0x7fdc2c5b3713] ash::internal::RootWindowController::Shutdown() [0x7fdc2c5b35aa] ash::internal::RootWindowController::~RootWindowController() [0x7fdc2c572f8d] ash::DisplayController::Shutdown() [0x7fdc2c5bb7bf] ash::Shell::~Shell() [0x7fdc2c5bbe32] ash::Shell::~Shell() [0x7fdc2c5bc026] ash::Shell::DeleteInstance() [0x000001dce60d] chrome::CloseAsh() [0x000001bf5435] ChromeBrowserMainExtraPartsAsh::PostMainMessageLoopRun() [0x0000026363e7] ChromeBrowserMainParts::PostMainMessageLoopRun() [0x000002638ce1] ChromeBrowserMainPartsLinux::PostMainMessageLoopRun() [0x000001793d76] chromeos::ChromeBrowserMainPartsChromeos::PostMainMessageLoopRun() [0x7fdc23ad2162] content::BrowserMainLoop::ShutdownThreadsAndCleanUp() [0x7fdc23ad607c] content::BrowserMainRunnerImpl::Shutdown() [0x7fdc23ad038a] content::BrowserMain() [0x0000027633f0] content::BrowserTestBase::SetUp() [0x0000017277b7] InProcessBrowserTest::SetUp() [0x000001b42ef3] testing::internal::HandleSehExceptionsInMethodIfSupported<>() [0x000001b40311] testing::internal::HandleExceptionsInMethodIfSupported<>() [0x000001b35fef] testing::Test::Run() [0x000001b3661e] testing::TestInfo::Run() [0x000001b36b2c] testing::TestCase::Run() [0x000001b3b12c] testing::internal::UnitTestImpl::RunAllTests() [0x000001b43d10] testing::internal::HandleSehExceptionsInMethodIfSupported<>() [0x000001b40b29] testing::internal::HandleExceptionsInMethodIfSupported<>() [0x000001b3a277] testing::UnitTest::Run() [0x00000177bc9a] base::TestSuite::Run() [0x0000008d49e9] ChromeTestLauncherDelegate::RunTestSuite() [0x000001361aba] content::LaunchTests() [0x0000008d489a] main
On 2013/02/25 18:46:54, bshe wrote: > On 2013/02/25 12:32:44, yoshiki wrote: > > @oshima: PTAL > > > > > https://codereview.chromium.org/12328030/diff/4002/ash/magnifier/magnificatio... > > File ash/magnifier/magnification_controller.cc (right): > > > > > https://codereview.chromium.org/12328030/diff/4002/ash/magnifier/magnificatio... > > ash/magnifier/magnification_controller.cc:53: } > > On 2013/02/21 16:45:06, oshima wrote: > > > Just > > > > > > return std::find(...) != root_windows.end(); > > > > Done. > > > > > https://codereview.chromium.org/12328030/diff/4002/ash/magnifier/magnificatio... > > ash/magnifier/magnification_controller.cc:458: root_window_ = NULL; > > On 2013/02/21 16:45:06, oshima wrote: > > > Shouldn't you switch the root window here? > > > > > > This way, can't you assume root_window_ is never been null elsewhere? > > > > Done. Switching root window here. > > > > > https://codereview.chromium.org/12328030/diff/4002/ash/magnifier/magnificatio... > > ash/magnifier/magnification_controller.cc:470: > > root_window_->RemoveObserver(this); > > On 2013/02/21 16:45:06, oshima wrote: > > > root_window_ can be NULL at this point, can't it? (with current flow) > > > > Done. I've added the check. > > > > > https://codereview.chromium.org/12328030/diff/4002/ash/magnifier/magnificatio... > > ash/magnifier/magnification_controller.cc:474: if > > (IsRootWindowValid(new_root_window)) { > > On 2013/02/21 16:45:06, oshima wrote: > > > Looking through call flow, this should never be false. > > > > Done. > > > > > https://codereview.chromium.org/12328030/diff/4002/ash/magnifier/magnificatio... > > ash/magnifier/magnification_controller.cc:483: bool > > MagnificationControllerImpl::IsRootwindowStale() { > > On 2013/02/21 16:45:06, oshima wrote: > > > Changing a state inside predicate style method like IsXXX > > > is confusing. If you need to keep this method, I'd sugges > > > something like > > > EnsureValidRootWindow or something like that. My recommendation > > > is to switch the root in OnWindowDestorying (or Destroyed if it works), and > > > eliminate this method if possible. > > > > Removed. > > This is probably related from a glace (not 100% sure). But after apply this > patch locally, I get a crash with the following stacktrace when running my test > in interactive_ui_tests > > [0x7fdc2d9d6c50] base::debug::StackTrace::StackTrace() > [0x7fdc2d9d6557] base::debug::(anonymous namespace)::StackDumpSignalHandler() > [0x7fdc212bdcb0] <unknown> > [0x7fdc26939263] aura::Window::SetPropertyInternal() > [0x7fdc2c569634] aura::Window::SetProperty<>() > [0x7fdc2c5b43cd] ash::internal::RootWindowController::CloseChildWindows() > [0x7fdc2c5b3713] ash::internal::RootWindowController::Shutdown() > [0x7fdc2c5b35aa] ash::internal::RootWindowController::~RootWindowController() > [0x7fdc2c572f8d] ash::DisplayController::Shutdown() > [0x7fdc2c5bb7bf] ash::Shell::~Shell() > [0x7fdc2c5bbe32] ash::Shell::~Shell() > [0x7fdc2c5bc026] ash::Shell::DeleteInstance() > [0x000001dce60d] chrome::CloseAsh() > [0x000001bf5435] ChromeBrowserMainExtraPartsAsh::PostMainMessageLoopRun() > [0x0000026363e7] ChromeBrowserMainParts::PostMainMessageLoopRun() > [0x000002638ce1] ChromeBrowserMainPartsLinux::PostMainMessageLoopRun() > [0x000001793d76] > chromeos::ChromeBrowserMainPartsChromeos::PostMainMessageLoopRun() > [0x7fdc23ad2162] content::BrowserMainLoop::ShutdownThreadsAndCleanUp() > [0x7fdc23ad607c] content::BrowserMainRunnerImpl::Shutdown() > [0x7fdc23ad038a] content::BrowserMain() > [0x0000027633f0] content::BrowserTestBase::SetUp() > [0x0000017277b7] InProcessBrowserTest::SetUp() > [0x000001b42ef3] testing::internal::HandleSehExceptionsInMethodIfSupported<>() > [0x000001b40311] testing::internal::HandleExceptionsInMethodIfSupported<>() > [0x000001b35fef] testing::Test::Run() > [0x000001b3661e] testing::TestInfo::Run() > [0x000001b36b2c] testing::TestCase::Run() > [0x000001b3b12c] testing::internal::UnitTestImpl::RunAllTests() > [0x000001b43d10] testing::internal::HandleSehExceptionsInMethodIfSupported<>() > [0x000001b40b29] testing::internal::HandleExceptionsInMethodIfSupported<>() > [0x000001b3a277] testing::UnitTest::Run() > [0x00000177bc9a] base::TestSuite::Run() > [0x0000008d49e9] ChromeTestLauncherDelegate::RunTestSuite() > [0x000001361aba] content::LaunchTests() > [0x0000008d489a] main Thanks for the report. I think the crash is fixed on the patch set #4. Waiting for trybot.
https://codereview.chromium.org/12328030/diff/13006/ash/magnifier/magnificati... File ash/magnifier/magnification_controller.cc (right): https://codereview.chromium.org/12328030/diff/13006/ash/magnifier/magnificati... ash/magnifier/magnification_controller.cc:141: // at shutdown. can't you destroy this object before all root windows get destroyed, so that you can assume root_window_ never be null, nor invalid?
On 2013/02/26 18:57:38, oshima wrote: > https://codereview.chromium.org/12328030/diff/13006/ash/magnifier/magnificati... > File ash/magnifier/magnification_controller.cc (right): > > https://codereview.chromium.org/12328030/diff/13006/ash/magnifier/magnificati... > ash/magnifier/magnification_controller.cc:141: // at shutdown. > can't you destroy this object before all root windows get destroyed, so that you > can assume root_window_ never > be null, nor invalid? I confirmed that MagnificationController is initialized after the first root window is created and MagnificationController is destroyed after the last root window is destroyed. And I rewrote the code and I now assume root_window_ never be null.
Sorry if my comment wasn't clear. You should delete this *before* all root windows are deleted so that you can safely remove it without worrying about if root window is null. https://codereview.chromium.org/12328030/diff/3008/ash/magnifier/magnificatio... File ash/magnifier/magnification_controller.cc (right): https://codereview.chromium.org/12328030/diff/3008/ash/magnifier/magnificatio... ash/magnifier/magnification_controller.cc:449: CHECK(active_root_window); what happens when the last root window gets detroyed?
I looked at the code and this instance is indeed deleted *before* root windows are deleted, so I assume you just made typo. https://codereview.chromium.org/12328030/diff/3008/ash/magnifier/magnificatio... File ash/magnifier/magnification_controller.cc (right): https://codereview.chromium.org/12328030/diff/3008/ash/magnifier/magnificatio... ash/magnifier/magnification_controller.cc:167: CHECK(root_window_); CHECK/DCHECK is useful if you want to fail early, but this case it'll fail right away, so you don't need it. https://codereview.chromium.org/12328030/diff/3008/ash/magnifier/magnificatio... ash/magnifier/magnification_controller.cc:173: DCHECK(root_window_); ditto https://codereview.chromium.org/12328030/diff/3008/ash/magnifier/magnificatio... ash/magnifier/magnification_controller.cc:181: DCHECK(root_window_); ditto https://codereview.chromium.org/12328030/diff/3008/ash/magnifier/magnificatio... ash/magnifier/magnification_controller.cc:202: DCHECK(root_window_); ditto https://codereview.chromium.org/12328030/diff/3008/ash/magnifier/magnificatio... ash/magnifier/magnification_controller.cc:272: DCHECK(root_window_); ditto https://codereview.chromium.org/12328030/diff/3008/ash/magnifier/magnificatio... ash/magnifier/magnification_controller.cc:570: if (IsMagnified() && root_window_ && event->type() == ui::ET_MOUSE_MOVED) you don't need to check root_window_ anymore, do you? https://codereview.chromium.org/12328030/diff/3008/ash/magnifier/magnificatio... ash/magnifier/magnification_controller.cc:597: if (current_root && current_root == root_window_) { when can |current_root| be NULL?
On 2013/02/27 19:07:04, oshima wrote: > I looked at the code and this instance is indeed deleted *before* root windows > are deleted, so I assume you just > made typo. > > https://codereview.chromium.org/12328030/diff/3008/ash/magnifier/magnificatio... > File ash/magnifier/magnification_controller.cc (right): > > https://codereview.chromium.org/12328030/diff/3008/ash/magnifier/magnificatio... > ash/magnifier/magnification_controller.cc:167: CHECK(root_window_); > CHECK/DCHECK is useful if you want to fail early, but > this case it'll fail right away, so you don't need it. > > https://codereview.chromium.org/12328030/diff/3008/ash/magnifier/magnificatio... > ash/magnifier/magnification_controller.cc:173: DCHECK(root_window_); > ditto > > https://codereview.chromium.org/12328030/diff/3008/ash/magnifier/magnificatio... > ash/magnifier/magnification_controller.cc:181: DCHECK(root_window_); > ditto > > https://codereview.chromium.org/12328030/diff/3008/ash/magnifier/magnificatio... > ash/magnifier/magnification_controller.cc:202: DCHECK(root_window_); > ditto > > https://codereview.chromium.org/12328030/diff/3008/ash/magnifier/magnificatio... > ash/magnifier/magnification_controller.cc:272: DCHECK(root_window_); > ditto > > https://codereview.chromium.org/12328030/diff/3008/ash/magnifier/magnificatio... > ash/magnifier/magnification_controller.cc:570: if (IsMagnified() && root_window_ > && event->type() == ui::ET_MOUSE_MOVED) > you don't need to check root_window_ anymore, do you? > > https://codereview.chromium.org/12328030/diff/3008/ash/magnifier/magnificatio... > ash/magnifier/magnification_controller.cc:597: if (current_root && current_root > == root_window_) { > when can |current_root| be NULL? I need this fix to land my CL, so I'm taking this over.
Ok, closing this. |
