Telerik-Verified-Plugins / WKWebView

DEPRECATED - this plugin served a purpose in the past, but there are better implementation now
832 stars 149 forks source link

Crash detection mechanism makes several apps unusable #104

Closed alain-r closed 9 years ago

alain-r commented 9 years ago

The current implementation of the web server crash detection by checking if the page title is empty is very unclean and breaks several apps. All apps having a page with an empty title will stop working (web server constantly rebooted) This is especially a problem as crash recovery is now enabled by default. A better way to detect if the web server has crashed needs to be found.

EddyVerbruggen commented 9 years ago

I understand your concern but didn't find a better way. Providing a title is recommended, but to help you out I've just added the option to add this to config.xml which will disable crash recovery:

<preference name="DisableCrashRecovery" value="true" />

Still, if you omit the property or set it to false, using a title is recommended.

This change is now in master and will be part of the upcoming 0.4.0 release.

ephemer commented 9 years ago

@EddyVerbruggen thanks for providing the option to disable this (and @alain-r thanks for reporting). I found that even when you refresh the page from within WKWebView, the crash detection kicks in, theoretically making the entire app restart (bad). In practice, this also leaves you with the old web view in memory (worse). In fact, you get (via Cordova) a new UIWebView as well – so each 'crash', or simple refresh in my case, leads to two new web views being created, leaving the old ones in memory. As you can imagine, this very quickly becomes unsustainable, leading to real (hard) app crashes.

The crash recovery feature also breaks LocalStorage in WKWebView (for our web app, and surely many others, a deal breaker), because the UIWebView cache replace's WKWebView's on startup (as per commit https://github.com/Telerik-Verified-Plugins/WKWebView/commit/d7df9c2f2463cef2ed9c6ccfd74bd4d59c3a10b3), but the crash recovery has no call to backup WKWebView's to the UIWebView store before this happens.

Given the above points, I think this feature should be opt-in, not opt-out. I do wonder what kind of crashes you are hoping to recover from here. I have been testing without the feature for a few hours and have found the overall experience much more predictable and reliable – haven't seen any crashes at all. If this is supposed to protect against JS 'crashes', maybe a general JS error handler would be more appropriate.

EddyVerbruggen commented 9 years ago

@ephemer Thanks for posting your concerns. I have not experienced the crashes myself, but others have. Those were not catchable with JS handlers btw.

The problem was that WKWebView crashed, but the app did not (like it would do without the plugin installed), so a PR added the feature to disable it which worked great for those cases and it seemed useful to have this feature as standard.

Your comment about LS is noted - the hard part with that though is that I have never been able to reproduce the crash myself. Do you have a reproductionpath?

Interesting note about the multiple webviews in memory: have you been able to see that in a debugger/profiler?

ephemer commented 9 years ago

@EddyVerbruggen you can see the extra web views stacking in the Safari Web Inspector (developer menu). The crash detection just calls [appDelegate createWindowAndStartWebServer:true] without cleaning up any old references – in fact, this method also creates a new GCDWebServer too. createWindowAndStartWebServer creates a new instance of MyViewController, but the old one isn't removed. I guess there's a reference cycle in there somewhere.

The refresh crash happens on our end with any Meteor app (presumably any web app whose assets need to be entirely loaded before starting, because this inevitably takes more than 1 second, which is the crash detection period)

Still not sure what you mean by WKWebView crashing. Sure this isn't just an unhandled javascript error?

EddyVerbruggen commented 9 years ago

@ephemer Aha! Can I 'borrow' your XCode project to play with this?

ephemer commented 9 years ago

Unfortunately the project contains a lot of proprietary code, but can I assist with specific questions.

If you just create a cordova bundle with a lot of files to load on refresh (> 100) or a couple of large images, that should do it. There's nothing code-specific causing the crash, as far as I can see.

The only common factor between the project crashes I've seen is Meteor (not actively involved with any other frameworks atm). This is because everything gets loaded at once (all templates js, css, html). Although their solution could also be cleaner, the problem is really the 1 second crash detection window, or, realistically, that the crashes are detected via the <title> check.

A colleague of mine suggested an alternative crash check (if it's really necessary): try to evaluate some simple javascript (maybe every 5 seconds, instead of every second) via the WKWebView – say 2 + 2, and checking the result is returned correctly. We haven't tested this though, maybe this wouldn't work either if the page is reloading.

EddyVerbruggen commented 9 years ago

I only need something that crashes :) Never used meteor.. can you perhaps set me up?

The problem with a 5 sec check is that the app will be blank max 5 sec after a crash..

ephemer commented 9 years ago

@EddyVerbruggen I just made this, from possibly the most popular open source Meteor project: https://github.com/ephemer/Telescope It has the same issue with refresh / LocalStorage etc.

Just clone the repo and run meteor run ios-device (you'll need meteor installed, see meteor.com). To reproduce, just run the xcode project on the simulator or device, open the Safari Inspector for that session and do a window.location.reload(). Note that the entire app has reloaded, not just the page. Also that you now have twice as many sessions to attach to in Safari's Debug menu (most of which are effectively dead).

Telescope is a good example, because you can also see the issue with LocalStorage / user accounts (try registering / logging in and then doing a reload).

EddyVerbruggen commented 9 years ago

@ephemer Thanks for the project, I can now indeed reproduce those two issues.

To come up with a fix I really need to actually crash the wkwebview, but I have never been able to do that (others have so that's what the recovery is for - it wasn't added by me btw because I couldn't reproduce it).

So anything I change in order to fix the Meteor case (reloading content which looks like a crash but isn't) may break the fix for the real crash. F.i. that 1+1 trick is a nice suggestion, but indeed it may still work even when the webview crashed...