-
Notifications
You must be signed in to change notification settings - Fork 640
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
CB-13094: (android) Don't show splash when activity being finished #130
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In theory this looks fine to me.
Is there a situation where the splash screen is shown during this "finishing" phase? Just wondering how I can reproduce this to test it locally.
I got it so: window.addEventListener('beforeunload', function()
{
navigator.splashscreen.show();
}); I show splash when reload my application without restarting cordova in some cases (language switching, debug mode, etc). Closing by "back" button fires "beforeunload" (to "about:blank" i think) and showing splash in same time activity being finished. Also i use crosswalk, maybe it fires "beforeunload", not the native WebView. Trace here: FATAL EXCEPTION: main
Process: scat.su.calltaxi, PID: 16999
android.view.WindowManager$BadTokenException: Unable to add window – token android.os.BinderProxy@5d0142d is not valid; is your activity running?
at android.view.ViewRootImpl.setView(ViewRootImpl.java:567)
at android.view.WindowManagerGlobal.addView(WindowManagerGlobal.java:310)
at android.view.WindowManagerImpl.addView(WindowManagerImpl.java:86)
at android.app.Dialog.show(Dialog.java:326)
at org.apache.cordova.splashscreen.SplashScreen$5.run(SplashScreen.java:318)
at android.app.Activity.runOnUiThread(Activity.java:5558)
at org.apache.cordova.splashscreen.SplashScreen.showSplashScreen(SplashScreen.java:281)
at org.apache.cordova.splashscreen.SplashScreen.onMessage(SplashScreen.java:189)
at org.apache.cordova.PluginManager.postMessage(PluginManager.java:312)
at org.apache.cordova.CordovaWebViewImpl.postMessage(CordovaWebViewImpl.java:377)
at org.apache.cordova.splashscreen.SplashScreen$3.run(SplashScreen.java:169)
at android.os.Handler.handleCallback(Handler.java:739)
at android.os.Handler.dispatchMessage(Handler.java:95)
at android.os.Looper.loop(Looper.java:148)
at android.app.ActivityThread.main(ActivityThread.java:5517)
at java.lang.reflect.Method.invoke(Native Method)
at com.android.internal.os.ZygoteInit$MethodAndArgsCaller.run(ZygoteInit.java:726)
at com.android.internal.os.ZygoteInit.main(ZygoteInit.java:616) |
Ah, great! Thanks for describing that. I will take a look at that. |
OK, I've been able to reproduce the behaviour - sort of. In my case, it wasn't that the my app showed the splashscreen, but that the app crashed while exiting! My test app was a fresh cordova project with cordova-android 6.2.3 and cordova-plugin-splashscreen 4.0.3 added. I added nothing to the default cordova app code other than attaching Here's my stack trace:
This was on a Google Pixel running Android 7.1.2. Next up I'll test out your patch and see how the behaviour differs. FYI @infil00p. |
And @SharUpOff your intuition is confirmed: the logcat logs show that after pressing the back button in my app after loading it up, Cordova tries to load about:blank:
|
Going to about:blank is done as a garbage collection technique before closing out the WebView entirely so that the memory is freed. There's a hilarious TODO here saying that it shouldn't destroy the WebView until about:blank is done loading. But yeah, I agree with the fix. The Splashscreen shouldn't try to attach a UI element to the Activity while the Activity is basically killing itself on a backbutton, or some other exit event, and that's clearly what's causing this behaviour. Of course, if we never made this a plugin in the first place, we wouldn't have such a weird disconnect between this plugin and the platform code, so there is that. This is why we're looking at wrapping this plugin back into the platform. |
Confirmed that this patch fixes the crash. Merged it in. Thanks for the contribution @SharUpOff! + on your first ever GitHub PR, too |
@filmaj, thank you for the prompt response! (: |
Platforms affected
What does this PR do?
What testing has been done on this change?
Checklist