Tentative fix for #248

[pedrovgs]

:pushpin: References

• Issue: Tentative fix for #248

:tophat: What is the goal?

Fix an exception blowing the stack I've found while using the library in a medium-size project. Please read #248 because there is a lot of information there that will be really useful.

How is it being implemented?

As the exception is being thrown by a third party library we can't control and there is a retry mechanism already implemented I've just changed the try/catch used to implement the retry mechanism when generating the accessibility information. The original code was expecting an IndexOutOfBoundException and there is a comment explaining the author of the code had no idea where the exception comes from.

// For some unknown reason, Android seems to occasionally throw a IndexOutOfBoundsException
// from onInitializeAccessibilityNodeInfoInternal in ViewGroup.  This seems to be
// nondeterministic, so lets retry if this happens.

You can find the code here

I know this implementation is not ideal at all but I think this is our best chance to get the library working right now. In the future, this issue should be fixed by the ViewCompat class authors.

How can it be tested?

The current test suit implementation should validate my fix. As this error is not 100% reproducible the only choice to generate coverage for this bugfix would be to wrap the usage of the ViewCompat class, inject it into the utility class using it and using a test double simulate the method onInitializeAccessibilityNodeInfo throws the exception I've found. I don't think this is worth it right now because I don't even know if my fix is compatible with the current library implementation 😞 Sorry about that, but this is the first time I take a look at this library internals.

Important considerations and questions.

Looks like the accessibility information might be optional when generating the report. Should we consider adding a flag to disable this feature for the users who would not need it?

Could we merge this fix and release a new patch? This crash is blocking me right now and blocks me in the library version 0.8.0 which uses an old metadata format for the screenshots.

[facebook-github-bot]

Hi @pedrovgs!

Thank you for your pull request and welcome to our community.We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file.

In order for us to review and merge your code, please sign at https://code.facebook.com/cla. If you are contributing on behalf of someone else (eg your employer), the individual CLA may not be sufficient and your employer may need to sign the corporate CLA.

If you have received this in error or have any questions, please contact us at cla@fb.com. Thanks!

[pedrovgs]

I've already signed the CLA 😃 If you need anything else from my end before reviewing the PR, just let me know.

[facebook-github-bot]

Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Facebook open source project. Thanks!

[xiphirx]

Looks like the accessibility information might be optional when generating the report. Should we consider adding a flag to disable this feature for the users who would not need it?

Yes I think this would be good. PR also very welcome if youre up for it :)

[blavalla]

Hi there, I'm the author of the original code that was checking for the IndexOutOfBoundsException.

When I ran into this exception, it was very specifically happening on ViewGroup's only, and caused by the "childrenForAccessibility" array internal to the onInitializeAccessibilityNodeInfoInternal method of that class (shown here - https://github.com/facebook/screenshot-tests-for-android/commit/009c6ad930e63c3282d7f826ad99c1ffc60a1d9a)

Since there was no way to prevent what seemed like a bug in the framework, it seems like catching the error and trying again was the best course of action.

With your issue though, I'm not so sure. If you don't mind, I'd like to try some debugging first here to make sure this can't be fixed some other way.

A couple of questions:

1. The stacktrace pasted seems to show the error coming from attempting to get a reference to the ClipboardManager from within the canPaste method of TextView, is this true for all errors you are seeing, or are they sometimes coming from somewhere else?
2. Does retrying ever fix the issue as it did with the IndexOutOfBoundsException, or does it still fail with each retry?
3. Looking into the specific stack trace, it seems like this method would only be called if the TextView in question was being focused. I'm curious if setting focusable="false" on the EditText that is triggering this issue also prevents the problem, or if it will instead just surface with a slightly different stack trace.

I'm fine with the fix overall (and like the suggestion of making the accessibility stuff optional if you aren't using it), but I'm concerned that what will actually happen here is that the same nodes will always fail, which will always cause an inaccurate accessibility hierarchy to be generated since it wouldn't include this node. This would defeat the purpose of this hierarchy existing since you'd never be able to see the accessibility properties of that node, and therefore never know if it will cause any accessibility problems for users.

[pedrovgs]

Hi @blavalla and thanks for your quick response!

This morning I've been testing a little bit more what's going on with this crash and I've added more information to the original issue in this comment. The exception is thrown when using this library to take screenshots of any instance of a Dialog. Looking at the code I knew there was nothing we could do to improve this scenario but keep using the retry mechanism so that's why I decided to send this PR as a tentative fix. However, if you look at my comment in the original issue you'll see even when catching a RuntimeException the library keeps failing in a different way with an IndexOutOfBoundException. That's why I'm going to send another PR to make the usage of the accessibility feature configurable and enabled by default. @xiphirx suggested PRs for this feature are welcome and I'm happy to help 😃

About the questions, I'm going to answer them one by one.

The stacktrace pasted seems to show the error coming from attempting to get a reference to the ClipboardManager from within the canPaste method of TextView, is this true for all errors you are seeing, or are they sometimes coming from somewhere else?

The trace is the same for every test failing.

Does retrying ever fix the issue as it did with the IndexOutOfBoundsException, or does it still fail with each retry?

The behavior is completely random and sometimes pass and sometime fails. There is something wrong deep down in the accessibility library where ViewCompat is implemented scheduling future evaluations of the accessibility information breaking the tests executions randomly even if we catch every exception 😢 I couldn't create a test scenario reproducing this issue 100% of the times I execute my tests but I'm thinking about how we could find an example reproducing the issue in an easy way. I'll send a PR if I find it.

Looking into the specific stack trace, it seems like this method would only be called if the TextView in question was being focused. I'm curious if setting focusable="false" on the EditText that is triggering this issue also prevents the problem, or if it will instead just surface with a slightly different stack trace.

I'm afraid in the tests where I've found this error there are no focusable views at all.

As you suggested in your final comment. This is not an ideal fix at all and making the accessibility feature optional will not fix the bug, but at least will let us keep using the library.

Please, let me know if you have any other questions or suggestions. If you could merge this PR and the one I'm about to send and publish a new release of the library I'd really appreciate it 😃 In my team we are hardcore users of this library and we'd love to be able to use it in our projects.

[facebook-github-bot]

@xiphirx merged this pull request in facebook/screenshot-tests-for-android@9e3b940e7dee53950633719747da2689535f8ce4.