fix(android): Prevent libhwui crash when WebView is in ScrollView

[Kudo]

Why

possible solution for #2771

How

observations:

• the crash is from https://android.googlesource.com/platform/frameworks/base/+/f05f9b960832b6272b6740721c0a4bbd1ce632c1/libs/hwui/pipeline/skia/GLFunctorDrawable.cpp#95 which looks like for WebView to draw something on offscreen buffer because it hits the view boundary.

• the MakeRenderTarget() returns null because SK_SUPPORT_GPU is undefined in skia build. the offscreen surface size is 0, i.e. !imageInfo.width() || !imageInfo.height() I could confirm it returns null by attaching debugger. https://android.googlesource.com/platform/external/skia/+/be52e9ba4d0a3c1e27c2ac215ddb14ab2b72274b/include/core/SkSurface.h#337

• modern devices should use skia vulkan (skiavk) renderer for hwui. supposedly it will not crash. i.e. adb shell getprop ro.hwui.use_vulkan returns true and adb shell getprop debug.hwui.renderer returns nothing.

• the call path happens only on hwui skiagl render type. it is the default setting for emulator on my mac m1. adb shell getprop debug.hwui.renderer returns skiagl for me.

• i could also reproduce the crash on devices by forcing hwui skiagl mode: adb shell setprop debug.hwui.renderer skiagl.

• from the crash stacktrace, i could see android::uirenderer::skiapipeline::RenderNodeDrawable::forceDraw(). it might also be a root cause where android should not call forceDraw at this point.

• since it's offscreen only, i could also reproduce the crash from a single webview example.

<ScrollView>
  <View style={{ height: 5000, flexDirection: 'column' }}>
    <View style={{ height: 2000, backgrounColor: 'green' }} collapsible={false} />
    <WebView style={{ height: 1000 }}  source={{ html: HTML }} />
    <View style={{ height: 2000, backgrounColor: 'green' }} collapsible={false} />
  </View>
</ScrollView>

• tried to rewrite the above example in android native code without react-native. it works fine without crashes for me. my hypothesis is that the reason to trigger forceDraw should be somewhere in react-native. finally get the code in ReactViewGroup which clip the canvas and it may come with side effects for webview compositing.

• then the question is "how could we create a dedicated android canvas for webview?" the answer in this pr is to wrap a webview inside a FrameLayout.

this pr basically updates the view hierachy to have

<FrameLayout>
  <WebView />
</FrameLayout>

<FrameLayout>
  <WebView />
</FrameLayout>

Test Plan

• test ‼️ Repro for Android libhwui crash (💸 $500 bounty for fix) #2771 example.
• check if there's any regression from example test cases.

[andreialecu]

This looks really promising! I was not able to get it to crash yet after removing all previous crash prevention workarounds we had on the simulator in our production app. Without the workarounds, it would instantly crash on load.

But I notice something odd. It appears the webview is not triggering the onLoadEnd event anymore after this PR. Had some code handling that is no longer working.

[Kudo]

But I notice something odd. It appears the webview is not triggering the onLoadEnd event anymore after this PR. Had some code handling that is no longer working.

good catch! will follow up soon.

[andreialecu]

Also injectedJavaScript isn't working. Probably something to do with prop passing.

[Kudo]

onLoadEnd should be fixed by the latest changes.

injectedJavaScript works for me on the examples/Injection.tsx, could you check whether you could repro on the same example?

[andreialecu]

Excellent, injectedJavaScript works now too. It may have been a side effect of onLoadEnd not firing and things specific to our code.

Things are running great now.

I don't have a real device to reproduce this easily on, but I will send an update to Google Play, set the crashes as resolved in Sentry, and check if they resurface.

Thanks so much for looking into this! (PS: I DM'd you on Twitter)

[Kudo]

cool! thanks much for your testing and even willing to try it on production directly. hopes the solution doing well and fixes the problem for you.

[TheAlmightyBob]

Thanks for the investigation/PR!

I don't quite follow the last bit of the description about how using a FrameLayout "creates a dedicated Android canvas"? i.e., how does the FrameLayout avoid the clipping problem?

Also, this overlaps/conflicts with some other PRs, such as the new architecture support (#2686) and "keeping native WebView in memory when react component is unmounted" (#2469). The latter of which is not close to being merged, but it does a similar thing with refactoring to wrap the WebView in a FrameLayout...

[andreialecu]

Huge thanks to @Kudo for investigating and finally fixing these crashes.

The image shows the Android crash rate before the fix. It should hopefully finally go to 0. Been a tough couple of years for our Android users.

I haven't seen a single crash in Sentry related to webviews since deploying this fix.

[Kudo]

I don't quite follow the last bit of the description about how using a FrameLayout "creates a dedicated Android canvas"? i.e., how does the FrameLayout avoid the clipping problem?

that's just my hypothesis and could be wrong. i tried to dig out more for the difference. unfortunately, to really figure out the whole thing, we may need a custom AOSP with native debug symbols and set breakpoints in native side.

as far as my understanding, a View (also ViewGroup) will have an underlying RenderNode and RecordingCanvas. having a new canvas might not cause the crash. i guess the difference might come from here.

without FrameLayout, it MAY go to the if block and the displayList->draw()

with FrameLayout, it MAY go to the else block and there's drawRect

i would say that's somehow an android's bug where not handle the case when GLFunctorDrawable clipBounds is empty. if wrapping webview in FrameLayout doesn't impact to react-native-webview much, we could try to have this and avoid the crash from android.

Also, this overlaps/conflicts with some other PRs, such as the new architecture support (#2686) and "keeping native WebView in memory when react component is unmounted" (#2469). The latter of which is not close to being merged, but it does a similar thing with refactoring to wrap the WebView in a FrameLayout...

totally understand there's much more work for those conflict. i just want to find a way to fix the strange crash. let me know if there's anything i can do for this pr. thanks!

[andreialecu]

Here's a list of crashes from the Google Play console prior to applying this patch:

We've been running the patched version for a few days now, and none of those crashes still exist.

There's only this one though, occured a single time. Not sure if something in this PR might be the cause:

More details here: https://letspoker.sentry.io/share/issue/bc60aa7fef5a49498c9cdac79aa89b48/

[Kudo]

There's only this one though, occured a single time. Not sure if something in this PR might be the cause:

More details here: https://letspoker.sentry.io/share/issue/bc60aa7fef5a49498c9cdac79aa89b48/

this updated commit should address this crash: 4674ae8

looks like it is caused when webview messages happen even after the webview is being removed.

[nluzzato]

Amazing work, waiting for it to enter the main branch as it's an issue that's been plaguing us as well

[Titozzz]

Adding this to the pile of things I need to take care of! Thanks @Kudo

[andreialecu]

@Titozzz perhaps you could consider adding some new maintainers? Maybe from the Expo team, if they're up for it? 🫶

[andreialecu]

Crash rate update, since deploying fix. It's not 0 because not all users updated yet.

[Kudo]

haha we're just a small team at expo and may not have much bandwidth and expertise to maintain many libraries. @Titozzz and all the react-native-webview maintainers are great and actively maintain this library. i don't think it's necessary to look up for new maintainers. as a good react native community, we should collaborate together anyway 🙌

[andreialecu]

As a user of this library, I have great respect for @Titozzz and the other maintainers. However, in my experience, it seems that the library is not currently being actively maintained.

I fully understand that open source is difficult and that the maintainers are working on this in their free time. However, I have noticed that a critical issue has gone unacknowledged for over two years, and I am concerned about this. I assume that if they were using the library in their own projects they'd have noticed this issue themselves.

I would like to suggest that perhaps the maintainers could consider adding more contributors to the project, rather than handing over the keys entirely. I know from personal experience that maintaining libraries can be a time-consuming task, and additional help could be very beneficial.

Please do not take this the wrong way - I am only offering a friendly suggestion. 🫶

@jamonholmgren seems to agree that more maintainers are needed as per this https://twitter.com/jamonholmgren/status/1628995972314591232

[Titozzz]

@andreialecu which critical issue has gone unacknowledged? I am curious to hear more about this

[andreialecu]

@andreialecu which critical issue has gone unacknowledged? I am curious to hear more about this

#1915

That's just one of them, this is also the same issue: #575 (see stack trace) and is from 2019

[Titozzz]

Just to clarify, I am not going to spend many hours fixing any issue that isn't affecting everyone. I will gladly merge and discuss any solution but this community package has always work like that for 5 years succesfully which I feel is a huge community achievement!

As Kudo said this is a package were we all collaborate. As for adding new maintainers I'm happy to have more people on board, but I'm not going to add everyone that just wants to get his PR in and then disappear, it wouldn't make sense.

Do you think since I don't actively read issues I should close the tabs and just keep the PR + discussions ?

[andreialecu]

#623
#1838
#575
#1069
#2364
#1915
#2494
#429

All of these are the exact same issue.

These are also this issue, but from other repos:

react-navigation/react-navigation#9067
react-navigation/react-navigation#10662
software-mansion/react-native-screens#1494
software-mansion/react-native-screens#105
invertase/react-native-firebase#2935
facebook/react-native#25494
facebook/react-native#34669
facebook/react-native#29978

There's a plethora of other assorted reports everywhere, including StackOverflow and other places.

https://stackoverflow.com/questions/56553663/react-native-null-pointer-dereference-after-navigating-to-from-a-page-containi
https://stackoverflow.com/questions/63171131/when-rendering-iframes-with-html-android-crashes-while-navigating-back-to-s
I could go on and on :)

@Kudo was a godsend for getting to the bottom of it.

Do you think since I don't actively read issues I should close the tabs and just keep the PR + discussions ?

Not reading issues is why I feel this library is not being maintained.

There's no alternative in the RN ecosystem to react-native-webview and it seems like a critical piece of the puzzle. You guys could probably get some of the big corporate users of react-native who have been active lately in this space to sponsor some maintenance work. Just trying to provide positive feedback.

[Kudo]

if those issues are all the same, i would say it's a SUPER HARD problem though. not only have to get a repro but also need some background of chromium, android webview adapter and the android framework. though i was a browser engineer, till now i'm not figuring out the exact root case. i think everyone want to get this problem fixed, it's just tough. other than this tough issue, i think the react-native-webview quality/maintenance should be healthy.

[andreialecu]

One other thing to note is that I have had this happen in absolutely every app I work on with react native, to the point where I actively avoid having to use react-native-webview and try to use other things like https://www.npmjs.com/package/react-native-render-html or more lately something like https://github.com/cawfree/react-native-wormhole

So I genuinely wonder how it didn't get any attention in the past 4 years.

Our apps are not even doing anything particularly special. This crash is very easy to run into.

if those issues are all the same, i would say it's a SUPER HARD problem though.

I agree it's a super hard problem, and those are definitely all related to this issue. If you look inside the details they're all webview related. I've been dealing with it for years, and I know all the workarounds. Even with all of them in place, you can still see that the crash rate was 8% or so (down from 100% on some devices).

[TheAlmightyBob]

As a fellow maintainer (and user of react-native-webview in production since well before it was split out of React Native core), I just want to +1 everything @Titozzz said.

I have been reading and responding to issues and reviewing PRs. There's currently a long backlog of PRs only because of the new architecture work, which is a massive refactor of the entire library.

My team did spend a lot of time investigating this a few years ago, but we finally settled on the opacity: 0.99 fix and have been stable ever since. We also only started seeing the issue after upgrading React Navigation, so were never certain where the root problem was. So kudos to @andreialecu for developing a simple repro and to @Kudo for the investigation/PR.

[VadimDyliko]

please merge

[janpe]

It's not just that, it reveals an issue that's core to react native itself, so I also need to discuss it with the core team, as it probably affects other libraries and should be fixed directly in react native. Please use a fork/patch if this is critical to you / your business

Any updates on this? Is there an issue in the RN repo about this?

[AdamSinnottSSG]

Could anyone confirm when this might be merged?

[AdamSinnottSSG]

Can we please get an update on this?
As I see it there is a fix here that works and this has been open for 7 months now.
It requires 1 approving review and merge conflicts resolving.
Is it possible to get this prioritised?

[soutua]

For confirmation, for the last 5 months, we have been running this PR as a patch on an app that has roughly 500 000 active Android installations, and the web view related crashing practically stopped when we applied the patch.

[Kudo]

This has conflicts that need to be resolved

resolved

Testing (I didn't get an answer to my question above as to what testing has been done)

did some smoke testing on the example project, both old arch and new arch.

sorry be late to response, hopes we can get the pr merged this time.

[JohnFTitor]

This fix worked for me. Thanks @Kudo!

[Titozzz]

After internal discussion, I've decided to move this forward as this is the only fix and the "quickest" (it tooks already too long) way to get it to everyone. Thanks @Kudo and everyone involved

[react-native-community-bot]

🎉 This PR is included in version 13.6.2 🎉

The release is available on:

• npm package (@latest dist-tag)
• GitHub release

Your semantic-release bot 📦🚀

[fgilio]

OMG thank you all!!

[berkaybeyaz1]

yesssss, thank you all and @Kudo 🙏🏽 🙏🏽 🙏🏽

[Kudo]

thanks @Titozzz for having this pr forward 👏 🚀

[AdamSinnottSSG]

That's fantastic news thank you very much to everyone involved.

[irekrog]

Great news! Thank you all

[mharrison-nzme]

Thank you very much