Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

13778: Avoid crash while getting an accessibility string. #7708

Merged
merged 1 commit into from
Jan 28, 2021

Conversation

iefremov
Copy link
Contributor

For some reason, MacOS call the patched method for tabs,
and tabs cannot provide an accessibility string.

Fix brave/brave-browser#13778

Resolves

Submitter Checklist:

  • There is a ticket for my issue.
  • Used Github auto-closing keywords in the commit message.
  • Wrote a good PR/commit description
  • Added appropriate labels (QA/Yes or QA/No; release-notes/include or release-notes/exclude; OS/...) to the associated issue
  • Checked the PR locally: npm run test -- brave_browser_tests, npm run test -- brave_unit_tests, npm run lint, npm run gn_check, npm run tslint
  • Ran git rebase master (if needed).
  • Requested a security/privacy review as needed.

Reviewer Checklist:

  • New files have MPL-2.0 license header.
  • Adequate test coverage exists to prevent regressions
  • Major classes, functions and non-trivial code blocks are well-commented
  • Changes in component dependencies are properly reflected in gn
  • Code follows the style guide
  • Test plan is specified in PR before merging

After-merge Checklist:

Test Plan:

@iefremov iefremov requested a review from a team as a code owner January 26, 2021 21:51
@iefremov iefremov self-assigned this Jan 26, 2021
@simonhong
Copy link
Member

Below method calls same one. Then, crash can occur with this?

- (id)AXStringForRange:(id)parameter {
  DCHECK([parameter isKindOfClass:[NSValue class]]);
  return [[self getAXValueAsString] substringWithRange:[parameter rangeValue]];
}

@iefremov
Copy link
Contributor Author

@simonhong AFAIK, objective C allows to call methods of nil object, so in this particular case we are safe even if getAXValueAsString returns nil.

Apparently there are many dangerous spots (since the whole file has been a WIP for a long time), but I'm not sure whether we should patch anything else until it bothers us

Copy link
Collaborator

@bridiver bridiver left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

please make sure we have a follow-up issue to investigate this and there are also some crashes for substring out of range

@iefremov
Copy link
Contributor Author

Follow-up brave/brave-browser#13802

For some reason, MacOS call the patched method for tabs,
and tabs cannot provide an accessibility string.

Fix brave/brave-browser#13778
@iefremov iefremov merged commit f846a91 into master Jan 28, 2021
@iefremov
Copy link
Contributor Author

CI failures unrelated

@petemill
Copy link
Member

petemill commented Jan 29, 2021

Does this fix these issues which happen for all signed official builds and crash PWA windows with the following error?

-[NSAccessibilityRemoteUIElement accessibilityTitle]: unrecognized selector sent to instance 0x7fe0a0eadbe0

brave/brave-browser#8117
brave/brave-browser#7205

@iefremov
Copy link
Contributor Author

Does this fix these issues which happen for all signed official builds and crash PWA windows with the following error?

unfortunately, it doesn't

@LaurenWags
Copy link
Member

LaurenWags commented Jan 29, 2021

Attempted to verify using

Brave | 1.21.35 Chromium: 88.0.4324.96 (Official Build) nightly (x86_64)
-- | --
Revision | 68dba2d8a0b149a1d3afac56fa74648032bcf46b-refs/branch-heads/4324@{#1784}
OS | macOS Version 10.15.7 (Build 19H15)

Reproduced the issue multiple times using steps and version listed in brave/brave-browser#13778 (comment) (1.21.30).

Executed those same steps using 1.21.35 and was eventually able to reproduce the crash. It took a bit longer, but it was reproducible using the same steps. Crash report id: a81a0000-9cc8-c705-0000-000000000000

Per discussion with @kjozwiak and @iefremov - logged follow up issue brave/brave-browser#13837 and current fix can be uplifted since it seems to have decreased the frequency of the crash.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Accessibility crash while closing tabs or restoring a session
5 participants