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

WebView select boxes drop down dialogs are throwing on some tablets #54164

Closed
amirh opened this issue Apr 7, 2020 · 4 comments · Fixed by flutter/plugins#2663
Closed

WebView select boxes drop down dialogs are throwing on some tablets #54164

amirh opened this issue Apr 7, 2020 · 4 comments · Fixed by flutter/plugins#2663
Assignees
Labels
a: platform-views Embedding Android/iOS views in Flutter apps customer: dream (g3) p: webview The WebView plugin platform-android Android applications specifically
Milestone

Comments

@amirh
Copy link
Contributor

amirh commented Apr 7, 2020

The failing codepath is only active on tablets, I was only able to reproduce the crash on Android Things.

Repro steps, load a web page with a drop-down select box (like: https://loc.gov) on a tablet running Android Things, and tap the drop down.

The following exception is thrown:

04-06 02:00:58.035  3336  3336 E AndroidRuntime: FATAL EXCEPTION: main
04-06 02:00:58.035  3336  3336 E AndroidRuntime: Process: com.example.flutter_app, PID: 3336
04-06 02:00:58.035  3336  3336 E AndroidRuntime: java.lang.NullPointerException: Attempt to invoke virtual method 'void android.widget.DropDownListView.setSelection(int)' on a null object reference
04-06 02:00:58.035  3336  3336 E AndroidRuntime: 	at android.widget.ListPopupWindow.show(ListPopupWindow.java:697)
04-06 02:00:58.035  3336  3336 E AndroidRuntime: 	at org.chromium.ui.DropdownPopupWindow.show(DropdownPopupWindow.java:31)
04-06 02:00:58.035  3336  3336 E AndroidRuntime: 	at org.chromium.ui.DropdownPopupWindow$1.onLayoutChange(DropdownPopupWindow.java:4)
04-06 02:00:58.035  3336  3336 E AndroidRuntime: 	at android.view.View.layout(View.java:19677)
04-06 02:00:58.035  3336  3336 E AndroidRuntime: 	at android.widget.AbsoluteLayout.onLayout(AbsoluteLayout.java:123)
04-06 02:00:58.035  3336  3336 E AndroidRuntime: 	at android.view.View.layout(View.java:19659)
04-06 02:00:58.035  3336  3336 E AndroidRuntime: 	at android.view.ViewGroup.layout(ViewGroup.java:6075)
04-06 02:00:58.035  3336  3336 E AndroidRuntime: 	at android.widget.FrameLayout.layoutChildren(FrameLayout.java:323)
04-06 02:00:58.035  3336  3336 E AndroidRuntime: 	at android.widget.FrameLayout.onLayout(FrameLayout.java:261)
04-06 02:00:58.035  3336  3336 E AndroidRuntime: 	at android.view.View.layout(View.java:19659)
04-06 02:00:58.035  3336  3336 E AndroidRuntime: 	at android.view.ViewGroup.layout(ViewGroup.java:6075)
04-06 02:00:58.035  3336  3336 E AndroidRuntime: 	at android.widget.FrameLayout.layoutChildren(FrameLayout.java:323)
04-06 02:00:58.035  3336  3336 E AndroidRuntime: 	at android.widget.FrameLayout.onLayout(FrameLayout.java:261)
04-06 02:00:58.035  3336  3336 E AndroidRuntime: 	at android.view.View.layout(View.java:19659)
04-06 02:00:58.035  3336  3336 E AndroidRuntime: 	at android.view.ViewGroup.layout(ViewGroup.java:6075)
04-06 02:00:58.035  3336  3336 E AndroidRuntime: 	at android.widget.FrameLayout.layoutChildren(FrameLayout.java:323)
04-06 02:00:58.035  3336  3336 E AndroidRuntime: 	at android.widget.FrameLayout.onLayout(FrameLayout.java:261)
04-06 02:00:58.035  3336  3336 E AndroidRuntime: 	at android.view.View.layout(View.java:19659)
04-06 02:00:58.035  3336  3336 E AndroidRuntime: 	at android.view.ViewGroup.layout(ViewGroup.java:6075)
04-06 02:00:58.035  3336  3336 E AndroidRuntime: 	at android.widget.LinearLayout.setChildFrame(LinearLayout.java:1791)
04-06 02:00:58.035  3336  3336 E AndroidRuntime: 	at android.widget.LinearLayout.layoutVertical(LinearLayout.java:1635)
04-06 02:00:58.035  3336  3336 E AndroidRuntime: 	at android.widget.LinearLayout.onLayout(LinearLayout.java:1544)
04-06 02:00:58.035  3336  3336 E AndroidRuntime: 	at android.view.View.layout(View.java:19659)
04-06 02:00:58.035  3336  3336 E AndroidRuntime: 	at android.view.ViewGroup.layout(ViewGroup.java:6075)
04-06 02:00:58.035  3336  3336 E AndroidRuntime: 	at android.widget.FrameLayout.layoutChildren(FrameLayout.java:323)
04-06 02:00:58.035  3336  3336 E AndroidRuntime: 	at android.widget.FrameLayout.onLayout(FrameLayout.java:261)
04-06 02:00:58.035  3336  3336 E AndroidRuntime: 	at com.android.internal.policy.DecorView.onLayout(DecorView.java:761)
04-06 02:00:58.035  3336  3336 E AndroidRuntime: 	at android.view.View.layout(View.java:19659)
04-06 02:00:58.035  3336  3336 E AndroidRuntime: 	at android.view.ViewGroup.layout(ViewGroup.java:6075)
04-06 02:00:58.035  3336  3336 E AndroidRuntime: 	at android.view.ViewRootImpl.performLayout(ViewRootImpl.java:2496)
04-06 02:00:58.035  3336  3336 E AndroidRuntime: 	at android.view.ViewRootImpl.performTraversals(ViewRootImpl.java:2212)
04-06 02:00:58.035  3336  3336 E AndroidRuntime: 	at android.view.ViewRootImpl.doTraversal(ViewRootImpl.java:1392)
04-06 02:00:58.035  3336  3336 E AndroidRuntime: 	at android.view.ViewRootImpl$TraversalRunnable.run(ViewRootImpl.java:6752)
04-06 02:00:58.035  3336  3336 E AndroidRuntime: 	at android.view.Choreographer$CallbackRecord.run(Choreographer.java:911)
04-06 02:00:58.035  3336  3336 E AndroidRuntime: 	at android.view.Choreographer.doCallbacks(Choreographer.java:723)
04-06 02:00:58.035  3336  3336 E AndroidRuntime: 	at android.view.Choreographer.doFrame(Choreographer.java:658)
04-06 02:00:58.035  3336  3336 E AndroidRuntime: 	at android.view.Choreographer$FrameDisplayEventReceiver.run(Choreographer.java:897)
04-06 02:00:58.035  3336  3336 E AndroidRuntime: 	at android.os.Handler.handleCallback(Handler.java:790)
04-06 02:00:58.035  3336  3336 E AndroidRuntime: 	at android.os.Handler.dispatchMessage(Handler.java:99)
04-06 02:00:58.035  3336  3336 E AndroidRuntime: 	at android.os.Looper.loop(Looper.java:164)

Note: I'm assigning April milestone as this is when customer: dream want it by, though at this point I'm still investigating and before we root cause the issue I have no real estimate for how involved is the fix.

b/152465158

@amirh amirh added platform-android Android applications specifically severe: customer blocker customer: dream (g3) a: platform-views Embedding Android/iOS views in Flutter apps p: webview The WebView plugin labels Apr 7, 2020
@amirh amirh added this to the April 2020 milestone Apr 7, 2020
@amirh amirh self-assigned this Apr 7, 2020
@amirh
Copy link
Contributor Author

amirh commented Apr 13, 2020

This only happens with Android WebView versions older than 67.0.3367.0 (prior to Chromium commit afb450).

Before afb450, on tablets Chromium used a ListPopupWindow to show select drop downs. ListPoupWindow is using DropDownListView which internally is using a ListDropDownWindow, which partially hijacks view focus (most importantly it overrides isFocused to return true, which causes requestChildFocus to be invoked here) .

With Android WebView versions prior to afb450, the following sequence happens when a select drop down is shown:

  1. WebView is callingListPopupWindow#show
  2. buildDropDown is invoked, which sets mDropDownList to a DropDownListView.
  3. showAsDropDown is invoked - resulting in mDropDownList being added to the window and is also synchronously performing the following sequence:
    1. WebView's focus change listener is loosing focus (as mDropDownList got it)
    2. WebView is hiding all popups (as it lost focus)
    3. WebView's SelectPopupDropDown#hide is invoked.
    4. DropDownPopupWindow#dismiss is invoked setting mDropDownList to null.
  4. mDropDownList#setSelection is invoked and is throwing a NullPointerException (as we just set mDropDownList to null).

This is an unfortunate case where a combination of grey area behaviors by Flutter, Chromium, and Android, each work on their own, crash when they are combined together.

This crash should not be popular as it requires a combination of an old Chromium version (vast majority of devices have a newer webview than 67.0.3367.0) and a tablet (on non tablets Chromium is using an AlertDialog for select elements).

I can imagine a workaround in the form of preventing the webview's focus lost listener from being invoked in this specific case, though I'm not super excited to land such a hack. I'm currently working with customer: dream to see whether they can increase their minimal webview version requirement.

@amirh
Copy link
Contributor Author

amirh commented Apr 14, 2020

We need to workaround as customer: dream can't rely on a recent enough Android WebView.

I tried this as a workaround (in our WebView subclass):

  @Override
  protected void onFocusChanged(boolean focused, int direction, Rect previouslyFocusedRect) {
    if (isCalledFromListPopupWindowShow() && !focused) {
      return;
    }
    super.onFocusChanged(focused, direction, previouslyFocusedRect);
  }

  private boolean isCalledFromListPopupWindowShow() {
    StackTraceElement[] stackTraceElements = Thread.currentThread().getStackTrace();
    for (int i = 0; i < stackTraceElements.length; i++) {
      if (stackTraceElements[i].getClassName().equals(ListPopupWindow.class.getCanonicalName())
              && stackTraceElements[i].getMethodName().equals("show")) {
        return true;
      }
    }
    return false;
  }

Extremely hacky. But seems to work. I'm actually considering landing this given that I know for sure that with the current implementations of ListPopupWindow and a WebView older than 67.0.3367.0, if we let this call go through the app will crash (I'd gate this on an Android version so we won't drop calls if ListPopupWindow changes, and also on a webview version). Theoretically there could be some visual difference caused by this workaround (the webview will visually present as focused where it previously didn't), but with these old webview versions I don't see any other choice (the alternative would be to let it crash). FWIW I couldn't easily find an example where it actually appears different than the native WebView, and also on newer WebView versions they migrated off the "focus hijacking" trick so the behavior with the workaround should be more similar to the newer webviews anyway.

Difficulty testing this

Integration test

This is the kind of fix that would normally make most sense to test with an integration test, though integration testing this is going to have a significant overhead (as non of the available emulator images use an old enough webview to have this bug, it will probably require us to set up our own emulator image which is not a business I want to get into), and probably puts customer: dream's the launch at risk.

Unit testing

This is not really trivial to test, you would normally want to confirm whether WebView#onFocusChanged was or was not called, typically by injecting the WebView implementation and then in the test replacing it with a mock, however this class extends WebView so we can't replace it with a testing implementation when under test.

Something we can do is instrument our overridden onFocusChanged and add a public API for observing when the call was propagated to the superclass, though this provides a partial test coverage (e.g nothing tests that the report that onFocusChanged was propagated to the superclass is correct) and costs in a public API.

Testing exemption?

Flutter's testing policy has been recently formalized as: no code lands without a test, on extreme cases a testing exemption should be approved by @Hixie.

I think this is a case where our limited ability to unit test, and the high cost of putting together an integration test harness (both bootstraping and ongoing maintenance of an emulator image with an outdated Android webview), in addition with the low and temporary return (the vast majority of webviews out there don't have this issue, and AT are likely to upgrade soon as well) calls for a test exemption. @Hixie do you approve?

@Hixie
Copy link
Contributor

Hixie commented Apr 14, 2020

can you unit test isCalledFromListPopupWindowShow() in some way? e.g. pass it a stack trace known to be relevant, and one known not to be, and verify that it handles them correctly?

anyway, skipping tests here is fine either way given your description, thanks for checking.

@jmagman jmagman added this to Awaiting triage in Mobile - Android platform view review via automation Apr 16, 2020
Mobile - Android platform view review automation moved this from Awaiting triage to Engineer reviewed Apr 20, 2020
hisaichi5518 added a commit to hisaichi5518/native_webview that referenced this issue Apr 29, 2020
@lock
Copy link

lock bot commented May 5, 2020

This thread has been automatically locked since there has not been any recent activity after it was closed. If you are still experiencing a similar issue, please open a new bug, including the output of flutter doctor -v and a minimal reproduction of the issue.

@lock lock bot locked and limited conversation to collaborators May 5, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
a: platform-views Embedding Android/iOS views in Flutter apps customer: dream (g3) p: webview The WebView plugin platform-android Android applications specifically
Projects
Development

Successfully merging a pull request may close this issue.

2 participants