1354939 - Crash in TabManager.tabsToRestore() -> [SavedTab]?

Status: RESOLVED FIXED

(Firefox for iOS :: General, defect, P1)

Description

[Stefan Arentz | :st3fan | ⏰ EST | he/him]

Here is the reason why we should not touch NSArchiving anymore: tabsToRestore() tries to decode a tab, which fails. NSKeyedUnarchiver throws an Objective-C exception but it is not actually marked in Swift as being able to throw exceptions. So the result is a hard crash.


0   CoreFoundation                      0x186566fd8 __exceptionPreprocess + 124 (NSException.m:165)
1   libobjc.A.dylib                     0x184fc8538 objc_exception_throw + 56 (objc-exception.mm:521)
2   Foundation                          0x186fe9afc -[NSCoder(Exceptions) __failWithException:] + 132 (NSCoder.m:550)
3   Foundation                          0x186fe9cb0 -[NSCoder(Exceptions) __failWithExceptionName:errorCode:format:] + 436 (NSCoder.m:586)
4   Foundation                          0x186fb5d9c _decodeObjectBinary + 408 (NSKeyedArchiver.m:2241)
5   Foundation                          0x186fbcf00 -[NSKeyedUnarchiver _decodeArrayOfObjectsForKey:] + 1544 (NSKeyedArchiver.m:2634)
6   Foundation                          0x186f5283c -[NSArray(NSArray) initWithCoder:] + 216 (NSArray.m:140)
7   Foundation                          0x186fb6420 _decodeObjectBinary + 2076 (NSKeyedArchiver.m:2304)
8   Foundation                          0x186fb5b58 _decodeObject + 308 (NSKeyedArchiver.m:2467)
9   Client                              0x10022e12c specialized static TabManager.tabsToRestore() -> [SavedTab]? (TabManager.swift:640)
10  Client                              0x100286d68 specialized BrowserViewController.shouldRestoreTabs() -> Bool (TabManager.swift:0)
11  Client                              0x1002601ec BrowserViewController.showRestoreTabsAlert() -> () (BrowserViewController.swift:0)
12  Client                              0x10025fe30 BrowserViewController.viewWillAppear(Bool) -> () (BrowserViewController.swift:528)
13  Client                              0x1002601b8 @objc BrowserViewController.viewWillAppear(Bool) -> () (BrowserViewController.swift:0)
14  UIKit                               0x18c6adcd0 -[UIViewController _setViewAppearState:isAnimating:] + 632 (UIViewController.m:3801)
15  UIKit                               0x18c6ada40 -[UIViewController __viewWillAppear:] + 156 (UIViewController.m:3911)
16  UIKit                               0x18c74d52c -[UINavigationController _startTransition:fromViewController:toViewController:] + 752 (UINavigationController.m:4821)
17  UIKit                               0x18c74cd00 -[UINavigationController _startDeferredTransitionIfNeeded:] + 856 (UINavigationController.m:4971)
18  UIKit                               0x18c74c8b4 -[UINavigationController __viewWillLayoutSubviews] + 64 (UINavigationController.m:5224)
19  UIKit                               0x18c74c818 -[UILayoutContainerView layoutSubviews] + 188 (UILayoutContainerView.m:86)
20  UIKit                               0x18c693158 -[UIView(CALayerDelegate) layoutSublayersOfLayer:] + 1200 (UIView.m:14232)
21  QuartzCore                          0x189883274 -[CALayer layoutSublayers] + 148 (CALayer.mm:8937)
22  QuartzCore                          0x189877de8 CA::Layer::layout_if_needed(CA::Transaction*) + 292 (CALayer.mm:8817)
23  QuartzCore                          0x189877ca8 CA::Layer::layout_and_display_if_needed(CA::Transaction*) + 32 (CALayer.mm:2345)
24  QuartzCore                          0x1897f3360 CA::Context::commit_transaction(CA::Transaction*) + 252 (CAContextInternal.mm:1689)
25  QuartzCore                          0x18981a3c0 CA::Transaction::commit() + 504 (CATransactionInternal.mm:420)
26  QuartzCore                          0x18981ae8c CA::Transaction::observer_callback(__CFRunLoopObserver*, unsigned long, void*) + 120 (CATransactionInternal.mm:793)
27  CoreFoundation                      0x1865149a0 __CFRUNLOOP_IS_CALLING_OUT_TO_AN_OBSERVER_CALLBACK_FUNCTION__ + 32 (CFRunLoop.c:1802)
28  CoreFoundation                      0x186512628 __CFRunLoopDoObservers + 372 (CFRunLoop.c:1898)
29  CoreFoundation                      0x186442db4 CFRunLoopRunSpecific + 456 (CFRunLoop.c:3114)
30  UIKit                               0x18c70045c -[UIApplication _run] + 652 (UIApplication.m:2658)
31  UIKit                               0x18c6fb130 UIApplicationMain + 208 (UIApplication.m:4089)
32  Client                              0x100042994 main (main.swift:21)
33  libdyld.dylib                       0x18545159c start + 4

Comment 1

[Farhan Patel [:farhan]]

There is a new method (9.0+) in NSKeyedUnarchiver called unarchivetoplevelobjectwithdata which throws. Is this what Apple expects us to use now?

https://developer.apple.com/reference/foundation/nskeyedunarchiver/1413622-unarchivetoplevelobjectwithdata

Comment 2

[Stefan Arentz | :st3fan | ⏰ EST | he/him]

I also see that since iOS 9 there is a decodingFailurePolicy property that you can set to 'throw exception' or 'return nil and set error'. We should check if it defaults to the former and then change it. That could be a good remediation for some of the crashes that we have seen. Better to not have tabs than to crash.

Comment 3

[Farhan Patel [:farhan]]

that actually looks like it works! I'll write a few tests around this and submit a patch.

Comment 4

[Stefan Arentz | :st3fan | ⏰ EST | he/him]

Can you also look if there is something similar for bug 1354942, which is about archiving? I don't see that failure policy there, but they do have a invalidArchiveOperationException. Strangely none of the NSKeyedArchiver methods actually throw exceptions?

If not already, maybe we can wrap this in our own exception catcher? See Try.m / Try.h

Comment 5

[Farhan Patel [:farhan]]

Attachment: Pull Request

Comment 6

[Stefan Arentz | :st3fan | ⏰ EST | he/him]

Please get some more crash reports to se the different types of crashes in this area.

Comment 7

[Stefan Arentz | :st3fan | ⏰ EST | he/him]

Attachment: PR https://github.com/mozilla-mobile/firefox-ios/pull/2642

This is an absolute minimal approach to make `NSUnarchiver.decodeObject()` stop raising exceptions that we cannot catch. With this `decodingFailurePolicy` it will simply return `nil` if the tabs could not be decoded.

Comment 8

[Stefan Arentz | :st3fan | ⏰ EST | he/him]

Let's address the encoding issues in the bug that we have on file for that. I'd prefer to do the smallest fix right now specifically for this bug. The test in the original PR is great but I am worried about the additional logic changes that required in the application. So I decided to not land that just yet.

Comment 9

[Stefan Arentz | :st3fan | ⏰ EST | he/him]

Marking this as resolved, but we should REOPEN to land tests. Or file a separate bug for that.