1305180 - TestBrowserDB.testMovesDB test is broken.

Status: RESOLVED FIXED

(Firefox for iOS :: Build & Test, defect, P2)

Description

[Farhan Patel [:farhan]]

Recently the test testMovesDB has broke. This might be related to the recent additions to the browser table related to Recommendations or the hotfix that went in for the top crasher we have. 

Simply run the test to reproduce the issue. It crashes with 

fatal error: 'try!' expression unexpectedly raised an error: Error Domain=NSCocoaErrorDomain Code=260 "The file “foo.db-shm” couldn’t be opened because there is no such file."

This test is currently broken on master 9f724fbe8ee46d5ba812fbbfc7235f10df370fb6

Comment 1

[Farhan Patel [:farhan]]

Steph, If you could have a look at this. That would be awesome. We have an assert that is suppose to check if the file exists and that passes. But somehow when it tries to open the same file it crashes. Someone on irc was having this issue earlier and I tried it myself on master.

Comment 2

[Stephan Leroux [:sleroux]]

Looks like this was caused by the patch we landed to close the database when the app goes into the background [1] which was causing our iOS 10 springboard crash. What seems to be happening is that when try to create the failing table in the test case [2], the database files are never moved after the database is closed because we now completely ignore [3] any connections made after we call `forceClose` on the DB [4].

:rnewman, would it make sense to move the .bak moving file logic before we call forceClose?

[1] https://github.com/mozilla-mobile/firefox-ios/commit/c3d742b80fe6af361cf9543271b9978c2d66730f
[2] https://github.com/mozilla-mobile/firefox-ios/blob/master/StorageTests/TestBrowserDB.swift#L86
[3] https://github.com/mozilla-mobile/firefox-ios/blob/master/Storage/ThirdParty/SwiftData.swift#L87
[4] https://github.com/mozilla-mobile/firefox-ios/blob/master/Storage/SQL/BrowserDB.swift#L186

Comment 3

[Richard Newman [:rnewman]]

If I'm understanding you correctly, this might be a real bug.

The forceClose call in BrowserDB is trying to forcibly close the database handle, so that it's free to move all of the old stuff out of the way and try again.

Later on, we call db.transaction to open a connection to a new database in the same place, and create all the tables[1]. This relies on the underlying DB quietly reopening or recreating the DB.

Now that forceClose means we never open a connection again, this logic is broken: we'll force close, move the files out of the way, and then we won't be able to 'reopen' the database again!

In the test, the subsequent recreate will fail… which the test already expects, but for the wrong reason.

My suggestion is that we add `self.reopenIfClosed()` at line 227, so that we get back on the old tracks.


[1] <https://github.com/mozilla-mobile/firefox-ios/blob/master/Storage/SQL/BrowserDB.swift#L228>

Comment 4

[Stephan Leroux [:sleroux]]

Attachment: Link to Github pull-request: https://github.com/mozilla-mobile/firefox-ios/pull/2133

Comment 5

[Farhan Patel [:farhan]]

Comment on attachment 8796616 [details] [review]
Link to Github pull-request: https://github.com/mozilla-mobile/firefox-ios/pull/2133

Thanks for looking at this. Is this something that we might want to uplift to 5.x?

Comment 6

[Stephan Leroux [:sleroux]]

> Thanks for looking at this. Is this something that we might want to uplift to 5.x?

Yup definitely.

Comment 7

[Stephan Leroux [:sleroux]]

master https://github.com/mozilla-mobile/firefox-ios/commit/970cb784ee3edbf5deb266f8a77075d16016d076
v5.x https://github.com/mozilla-mobile/firefox-ios/commit/002c42f5d7586f92485949f335ea75827e0d6a40