New issue
Advanced search Search tips

Issue 734939 link

Starred by 1 user
Status: Fixed
Owner:
tapted@chromium.org
Closed: Jul 2017
Cc:
mac-bugs-priority@chromium.org
Components:
EstimatedDays: ----
NextAction: ----
OS: Mac
Pri: 1
Type: Bug
Team-Accessibility

Blocking:
issue 462133



Sign in to add a comment

MacViews: a11y lifetime problems

Project Member Reported by tapted@chromium.org, Jun 20 2017 
Chrome Version       : 61.0.3124.4
OS Version: OS X 10.12.5

What steps will reproduce the problem?
1. Open a11y inspector, open a bubble, e.g. PageInfo
2. Press Alt+Space to toggle the inspection pointer
3. Click on a control with an action (it goes green)
4. Click "perform" on a11y inspector

What is the expected result?

No Crash.

What happens instead of that?

DCHECK, or a Crash if the DCHECK is skipped.

The problem is that AXPlatformNodeCocoa doesn't have enough checks for its |node_| being detached.

Fix: https://codereview.chromium.org/2944103005


frame #3: 0x000000012e10d087 libaccessibility.dylib`::-[AXPlatformNodeCocoa accessibilityPerformAction:](self=0x0000000168e5ca50, _cmd="accessibilityPerformAction:", action=0x7373657250584175) + 279 at ax_platform_node_mac.mm:343
   340 	}
   341
   342 	- (void)accessibilityPerformAction:(NSString*)action {
-> 343 	  DCHECK([[self accessibilityActionNames] containsObject:action]);
   344 	  ui::AXActionData data;
   345 	  for (const ActionMap::value_type& entry : GetActionMap()) {
   346 	    if ([action isEqualToString:entry.second]) {
Project Member

Comment 1 by bugdroid1@chromium.org, Jul 4 2017 

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/bb756436f55c01c2066c17d5eec5aaeb1e52a120

commit bb756436f55c01c2066c17d5eec5aaeb1e52a120
Author: tapted <tapted@chromium.org>
Date: Tue Jul 04 03:31:09 2017

Fix lifetime problems in AXPlatformNodeCocoa.

AXPlatformNodeCocoa needs to check in more places for its |node_| being
detached. Fix and add test coverage.

BUG= 734939 

Review-Url: https://codereview.chromium.org/2944103005
Cr-Commit-Position: refs/heads/master@{#484048}

[modify] https://crrev.com/bb756436f55c01c2066c17d5eec5aaeb1e52a120/ui/accessibility/platform/ax_platform_node_mac.mm
[modify] https://crrev.com/bb756436f55c01c2066c17d5eec5aaeb1e52a120/ui/views/widget/native_widget_mac_accessibility_unittest.mm

Comment 2 by tapted@chromium.org, Jul 4 2017

Status: Fixed (was: Started)

Comment 3 by a...@chromium.org, Jul 5 2017

Status: Assigned (was: Fixed)
Had to revert the fix.

It fails to compile with a modern SDK:

../../ui/views/widget/native_widget_mac_accessibility_unittest.mm:176:34: error: null passed to a callee that requires a non-null argument [-Werror,-Wnonnull]
  EXPECT_EQ(NSNotFound, [ax_node accessibilityIndexOfChild:nil]);
                                 ^                         ~~~
../../third_party/googletest/src/googletest/include/gtest/gtest.h:1925:29: note: expanded from macro 'EXPECT_EQ'
                      val1, val2)
                            ^~~~
../../third_party/googletest/src/googletest/include/gtest/gtest_pred_impl.h:162:40: note: expanded from macro 'EXPECT_PRED_FORMAT2'
  GTEST_PRED_FORMAT2_(pred_format, v1, v2, GTEST_NONFATAL_FAILURE_)
                                       ^~
../../third_party/googletest/src/googletest/include/gtest/gtest_pred_impl.h:147:43: note: expanded from macro 'GTEST_PRED_FORMAT2_'
  GTEST_ASSERT_(pred_format(#v1, #v2, v1, v2), \
                                          ^~
../../third_party/googletest/src/googletest/include/gtest/gtest_pred_impl.h:77:52: note: expanded from macro 'GTEST_ASSERT_'
  if (const ::testing::AssertionResult gtest_ar = (expression)) \
                                                   ^~~~~~~~~~
../../ui/views/widget/native_widget_mac_accessibility_unittest.mm:217:34: error: null passed to a callee that requires a non-null argument [-Werror,-Wnonnull]
  EXPECT_EQ(NSNotFound, [ax_node accessibilityIndexOfChild:nil]);
                                 ^                         ~~~
../../third_party/googletest/src/googletest/include/gtest/gtest.h:1925:29: note: expanded from macro 'EXPECT_EQ'
                      val1, val2)
                            ^~~~
../../third_party/googletest/src/googletest/include/gtest/gtest_pred_impl.h:162:40: note: expanded from macro 'EXPECT_PRED_FORMAT2'
  GTEST_PRED_FORMAT2_(pred_format, v1, v2, GTEST_NONFATAL_FAILURE_)
                                       ^~
../../third_party/googletest/src/googletest/include/gtest/gtest_pred_impl.h:147:43: note: expanded from macro 'GTEST_PRED_FORMAT2_'
  GTEST_ASSERT_(pred_format(#v1, #v2, v1, v2), \
                                          ^~
../../third_party/googletest/src/googletest/include/gtest/gtest_pred_impl.h:77:52: note: expanded from macro 'GTEST_ASSERT_'
  if (const ::testing::AssertionResult gtest_ar = (expression)) \
                                                   ^~~~~~~~~~
In file included from ../../ui/views/widget/native_widget_mac_accessibility_unittest.mm:14:
In file included from ../../testing/gtest_mac.h:8:
In file included from ../../testing/gtest/include/gtest/gtest.h:10:
../../third_party/googletest/src/googletest/include/gtest/gtest.h:1392:11: error: comparison of integers of different signs: 'const long' and 'const unsigned long' [-Werror,-Wsign-compare]
  if (lhs == rhs) {
      ~~~ ^  ~~~
../../third_party/googletest/src/googletest/include/gtest/gtest.h:1421:12: note: in instantiation of function template specialization 'testing::internal::CmpHelperEQ<long, unsigned long>' requested here
    return CmpHelperEQ(lhs_expression, rhs_expression, lhs, rhs);
           ^
../../ui/views/widget/native_widget_mac_accessibility_unittest.mm:176:3: note: in instantiation of function template specialization 'testing::internal::EqHelper<false>::Compare<long, unsigned long>' requested here
  EXPECT_EQ(NSNotFound, [ax_node accessibilityIndexOfChild:nil]);
  ^
../../third_party/googletest/src/googletest/include/gtest/gtest.h:1924:63: note: expanded from macro 'EXPECT_EQ'
                      EqHelper<GTEST_IS_NULL_LITERAL_(val1)>::Compare, \
                                                              ^
3 errors generated.
Project Member

Comment 4 by bugdroid1@chromium.org, Jul 5 2017 

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/da803e376ba46699f1800c6d0f3953ab61e78c9b

commit da803e376ba46699f1800c6d0f3953ab61e78c9b
Author: avi <avi@chromium.org>
Date: Wed Jul 05 19:43:47 2017

Revert of Fix lifetime problems in AXPlatformNodeCocoa. (patchset #6 id:100001 of https://codereview.chromium.org/2944103005/ )

Reason for revert:
Compile issues with modern SDK:

../../ui/views/widget/native_widget_mac_accessibility_unittest.mm:176:34: error: null passed to a callee that requires a non-null argument [-Werror,-Wnonnull]
  EXPECT_EQ(NSNotFound, [ax_node accessibilityIndexOfChild:nil]);
                                 ^                         ~~~
../../third_party/googletest/src/googletest/include/gtest/gtest.h:1925:29: note: expanded from macro 'EXPECT_EQ'
                      val1, val2)
                            ^~~~
../../third_party/googletest/src/googletest/include/gtest/gtest_pred_impl.h:162:40: note: expanded from macro 'EXPECT_PRED_FORMAT2'
  GTEST_PRED_FORMAT2_(pred_format, v1, v2, GTEST_NONFATAL_FAILURE_)
                                       ^~
../../third_party/googletest/src/googletest/include/gtest/gtest_pred_impl.h:147:43: note: expanded from macro 'GTEST_PRED_FORMAT2_'
  GTEST_ASSERT_(pred_format(#v1, #v2, v1, v2), \
                                          ^~
../../third_party/googletest/src/googletest/include/gtest/gtest_pred_impl.h:77:52: note: expanded from macro 'GTEST_ASSERT_'
  if (const ::testing::AssertionResult gtest_ar = (expression)) \
                                                   ^~~~~~~~~~
../../ui/views/widget/native_widget_mac_accessibility_unittest.mm:217:34: error: null passed to a callee that requires a non-null argument [-Werror,-Wnonnull]
  EXPECT_EQ(NSNotFound, [ax_node accessibilityIndexOfChild:nil]);
                                 ^                         ~~~
../../third_party/googletest/src/googletest/include/gtest/gtest.h:1925:29: note: expanded from macro 'EXPECT_EQ'
                      val1, val2)
                            ^~~~
../../third_party/googletest/src/googletest/include/gtest/gtest_pred_impl.h:162:40: note: expanded from macro 'EXPECT_PRED_FORMAT2'
  GTEST_PRED_FORMAT2_(pred_format, v1, v2, GTEST_NONFATAL_FAILURE_)
                                       ^~
../../third_party/googletest/src/googletest/include/gtest/gtest_pred_impl.h:147:43: note: expanded from macro 'GTEST_PRED_FORMAT2_'
  GTEST_ASSERT_(pred_format(#v1, #v2, v1, v2), \
                                          ^~
../../third_party/googletest/src/googletest/include/gtest/gtest_pred_impl.h:77:52: note: expanded from macro 'GTEST_ASSERT_'
  if (const ::testing::AssertionResult gtest_ar = (expression)) \
                                                   ^~~~~~~~~~
In file included from ../../ui/views/widget/native_widget_mac_accessibility_unittest.mm:14:
In file included from ../../testing/gtest_mac.h:8:
In file included from ../../testing/gtest/include/gtest/gtest.h:10:
../../third_party/googletest/src/googletest/include/gtest/gtest.h:1392:11: error: comparison of integers of different signs: 'const long' and 'const unsigned long' [-Werror,-Wsign-compare]
  if (lhs == rhs) {
      ~~~ ^  ~~~
../../third_party/googletest/src/googletest/include/gtest/gtest.h:1421:12: note: in instantiation of function template specialization 'testing::internal::CmpHelperEQ<long, unsigned long>' requested here
    return CmpHelperEQ(lhs_expression, rhs_expression, lhs, rhs);
           ^
../../ui/views/widget/native_widget_mac_accessibility_unittest.mm:176:3: note: in instantiation of function template specialization 'testing::internal::EqHelper<false>::Compare<long, unsigned long>' requested here
  EXPECT_EQ(NSNotFound, [ax_node accessibilityIndexOfChild:nil]);
  ^
../../third_party/googletest/src/googletest/include/gtest/gtest.h:1924:63: note: expanded from macro 'EXPECT_EQ'
                      EqHelper<GTEST_IS_NULL_LITERAL_(val1)>::Compare, \
                                                              ^
3 errors generated.

Original issue's description:
> Fix lifetime problems in AXPlatformNodeCocoa.
>
> AXPlatformNodeCocoa needs to check in more places for its |node_| being
> detached. Fix and add test coverage.
>
> BUG= 734939 
>
> Review-Url: https://codereview.chromium.org/2944103005
> Cr-Commit-Position: refs/heads/master@{#484048}
> Committed: https://chromium.googlesource.com/chromium/src/+/bb756436f55c01c2066c17d5eec5aaeb1e52a120

TBR=dmazzoni@chromium.org,tapted@chromium.org
# Using NOTRY because iOS bots are failing for unrelated reasons, this is a Mac patch, and Mac bots are green.
NOTRY=true
BUG= 734939 

Review-Url: https://codereview.chromium.org/2969293002
Cr-Commit-Position: refs/heads/master@{#484331}

[modify] https://crrev.com/da803e376ba46699f1800c6d0f3953ab61e78c9b/ui/accessibility/platform/ax_platform_node_mac.mm
[modify] https://crrev.com/da803e376ba46699f1800c6d0f3953ab61e78c9b/ui/views/widget/native_widget_mac_accessibility_unittest.mm
Project Member

Comment 5 by bugdroid1@chromium.org, Jul 6 2017 

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/98c0539b58c22054eb032bde7df0648846f9acbc

commit 98c0539b58c22054eb032bde7df0648846f9acbc
Author: tapted <tapted@chromium.org>
Date: Thu Jul 06 06:05:18 2017

Fix lifetime problems in AXPlatformNodeCocoa.

AXPlatformNodeCocoa needs to check in more places for its |node_| being
detached. Fix and add test coverage.

Previously committed in r484048. Reverted due to compile errors against
more recent SDKs. Now fixed.

BUG= 734939 

Review-Url: https://codereview.chromium.org/2944103005
Cr-Commit-Position: refs/heads/master@{#484477}

[modify] https://crrev.com/98c0539b58c22054eb032bde7df0648846f9acbc/ui/accessibility/platform/ax_platform_node_mac.mm
[modify] https://crrev.com/98c0539b58c22054eb032bde7df0648846f9acbc/ui/views/widget/native_widget_mac_accessibility_unittest.mm

Comment 6 by tapted@chromium.org, Jul 6 2017

Status: Fixed (was: Assigned)
►

Sign in to add a comment