Skip to content

Commit

Permalink
Tentative fix for #248 (#249)
Browse files Browse the repository at this point in the history
Summary:
### 📌 References
* **Issue:** Tentative fix for #248

### 🎩 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.

```java
// 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](https://github.com/facebook/screenshot-tests-for-android/blob/master/core/src/main/java/com/facebook/testing/screenshot/layouthierarchy/AccessibilityUtil.java#L189)

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.
Pull Request resolved: #249

Reviewed By: sjkirby

Differential Revision: D21314219

Pulled By: xiphirx

fbshipit-source-id: 57030c61dc043ea0eaa5d538c88cad226261477c
  • Loading branch information
pedrovgs authored and facebook-github-bot committed Apr 30, 2020
1 parent 28dcef9 commit 9e3b940
Showing 1 changed file with 2 additions and 1 deletion.
Original file line number Diff line number Diff line change
Expand Up @@ -182,11 +182,12 @@ private static AccessibilityNodeInfoCompat createNodeInfoFromView(
nodeInfo.recycle();
}
return null;
} catch (IndexOutOfBoundsException e) {
} catch (RuntimeException e) {
if (nodeInfo != null) {
nodeInfo.recycle();
}
// For some unknown reason, Android seems to occasionally throw a IndexOutOfBoundsException
// and also random RuntimeExceptions because the handler seems not to be initialized
// from onInitializeAccessibilityNodeInfoInternal in ViewGroup. This seems to be
// nondeterministic, so lets retry if this happens.
if (retryCount > 0) {
Expand Down

0 comments on commit 9e3b940

Please sign in to comment.