Run getGlobalVisibleRect on the main thread instead of a background thread #736
Conversation
| adBaseView.getGlobalVisibleRect(rect); | ||
|
|
||
| Handler mainHandler = new Handler(Looper.getMainLooper()); | ||
| mainHandler.post(() -> adBaseView.getGlobalVisibleRect(rect)); |
There was a problem hiding this comment.
The quick search shows that there are more than 20 usages of the adBaseView object in the BaseJSInterface class.
I'm afraid any of them can lead to a similar crash.
I'd suggest really, hard to reproduce the current crash first, then fix it, then try to reproduce with other usages of the adBaseView before the release. It will help to understand the curcomstanses and prevent other crashes, potentially in the same ads.
If we can't reproduce it, of course, we need to release the fix based on the assumptions, but it is better to cover as much potential cases as possible. Otherwice we will get the same crash statistic but with another method, for example: adBaseView.getGlobalVisibleRect(rect)
There was a problem hiding this comment.
I have already looked through the usages, most are checks if null or are already called on the main/ui thread. I have not been able to reproduce though and you are correct that we need to try to reproduce first.
There was a problem hiding this comment.
It looks like the Handler runs the task asynchronously. However, we need the data synchronously to process it in the next step.
There was a problem hiding this comment.
Here is the behaviour before changes
There was a problem hiding this comment.
Ok, it should fix the current crash. But I'm not sure about all other possible cases.
Ideally, since we've faced such an issue, we should wrap a managed WebView in some background-thread-safe-invocation wrapper. To make sure that all clients of the JS bridge (ad code and SDK code) can use in safely. It could be done in the scope of the separate tech debt ticket.
| adBaseView.getGlobalVisibleRect(rect); | ||
|
|
||
| Handler mainHandler = new Handler(Looper.getMainLooper()); | ||
| mainHandler.post(() -> adBaseView.getGlobalVisibleRect(rect)); |
There was a problem hiding this comment.
Here is the behaviour before changes
|
I think we should release this hotfix as soon as possible, since it leads to multiple crashes right now. |
I will be merging this and the other PR's and putting out a release today. |
Closes #735