Fix tests in safari #1068

Closed
daleharvey opened this Issue Dec 1, 2013 · 21 comments

Projects

None yet

4 participants

@daleharvey
Member

Want to back this issue? Place a bounty on it! We accept bounties via Bountysource.

@daleharvey
Member

Tests are currently failing, the failure is fairly confusing, I run only only changes test suite and consitently on the 43rd test it fails, if I comment out tests before or after it, it generally always fails on the 43rd, at first assuming this may be some hard limit on open databases or size / request limit I commented out the http adapter tests and they all pass at that point, the point at which the tests fail looks to be consistent to I dont think its some race / open database situation, so far as far as I can tell safari just gives up working

unable to open database (14 unable to open database file)
[Error] InvalidStateError: DOM Exception 11: An attempt was made to use an object that is not, or is no longer, usable.

is the error

@daleharvey
Member

Also fairly sure this is a new error, either with mavericks or safari 7.0, I ran these tests on my older machine and remember only the base64 problem

@strmpnk
strmpnk commented Dec 2, 2013

I just ran a build on Safari 7.0 (9537.71) with the dev server using a default Apache CouchDB 1.5.0 installation. The only test which failed the 394th attachments: local-1: Test getAttachment with PNG (2nd assertion).

Expected:   
"iVBORw0KGgoAAAANSUhEUgAAABAAAAAQCAMAAAAoLQ9TAAAAMFBMVEX+9+j+9OD+7tL95rr93qT80YD7x2L6vkn6syz5qRT4ogT4nwD4ngD4nQD4nQD4nQDT2nT/AAAAcElEQVQY002OUQLEQARDw1D14f7X3TCdbfPnhQTqI5UqvGOWIz8gAIXFH9zmC63XRyTsOsCWk2A9Ga7wCXlA9m2S6G4JlVwQkpw/YmxrUgNoMoyxBwSMH/WnAzy5cnfLFu+dK2l5gMvuPGLGJd1/9AOiBQiEgkzOpgAAAABJRU5ErkJggg=="
Result:     
"wolQTkcNChoK"
Diff:   
"iVBORw0KGgoAAAANSUhEUgAAABAAAAAQCAMAAAAoLQ9TAAAAMFBMVEX+9+j+9OD+7tL95rr93qT80YD7x2L6vkn6syz5qRT4ogT4nwD4ngD4nQD4nQD4nQDT2nT/AAAAcElEQVQY002OUQLEQARDw1D14f7X3TCdbfPnhQTqI5UqvGOWIz8gAIXFH9zmC63XRyTsOsCWk2A9Ga7wCXlA9m2S6G4JlVwQkpw/YmxrUgNoMoyxBwSMH/WnAzy5cnfLFu+dK2l5gMvuPGLGJd1/9AOiBQiEgkzOpgAAAABJRU5ErkJggg==" "wolQTkcNChoK" 
Source:     
http://127.0.0.1:8000/tests/test.attachments.js:222:24
onloadend@http://127.0.0.1:8000/tests/test.utils.js:134:15
@daleharvey
Member

Hmm, exact same build, are you on mavericks? that error I expect and the one I was hoping to work on (#971)

fairly confused at this point

@strmpnk
strmpnk commented Dec 2, 2013

Yes. Safari 7.x is Mavericks only at the moment AFAICT. I'll try messing around a little more to see if I can trigger some variance. I've also got a 10.9 dev preview on a different machine I can try this with to see if it varies at all.

@vrutberg

Did you guys resolve this? I am having a similar problem in a totally unrelated project. I just upgraded to Safari 7.0.1 (as part of OS X 10.9.1) and the problem seems to persist.

@daleharvey
Member

Nope currently still looking into it, as far as I can tell webSQL is just completely broken under some conditions in safari

@vrutberg

Yes, seems so. Have you been able to find a relevant bug ticket in the WebKit bug tracker (or somewhere else)? I have been able to reproduce this on Safari 6.1.x (on OS X 10.8) as well, although it doesn't seem to even remotely as often as on Safari 7.

@vrutberg

I thought I'd share the progress we've made on this. The problem, in our cause, seemed to be that Safari never, ever performs GC on WebSQL database handles. This means that when you reach a certain limit, it can't open any new database handles. We solved this by simply reusing the database handle.

Worth noting is that the WebSQL spec doesn't mention GC at all, it seems to be completely up to the implementor.

Also, in case you didn't know there is a 'Debug' menu in the Safari 7. You can enable it by running this command:

$ defaults write com.apple.Safari IncludeInternalDebugMenu 1

Then, when you restart Safari, you will have a 'Debug' menu option.

@daleharvey
Member

Thats pretty consistent with what I am seeing, we do exactly 144 openDatabase calls before it gets killed

Its extremely strange that it doesnt reproduce consistently seeing the @strmpnk didnt see it, unfortunately we are actually opening entirely different databases during our tests, but we can workaround that, thanks a lot for sharing

@vrutberg

Pull request in Facebooks IndexedDB polyfill for reference: facebookarchive/IndexedDB-polyfill#7

@vrutberg

@daleharvey The number of openDatabase() calls we could perform before it got killed seemed to vary, but we were pretty consistently somewhere in the 240-250 range. And yes, it is extremely strange that it doesn't seem to be 100% reproducible for different users. Could it have something to do with system resources getting exhausted (or something similar)?

@nolanlawson
Member

Glad I'm not the only one seeing failed unit tests. Safari 6.1 on Mountain Lion (OS X 10.8.5) here.

I tried caching the database object, as @vrutberg did in the Facebook patch, but the same tests keep failing. Specifically, I'm seeing test.all_docs.js fail at line 83, because only two results are being returned (rather than three).

Bizarrely, if I set breakpoints in the debugger at each line of the test Testing allDocs opts.keys, all the tests pass except for correct answer if keys is empty, which shows rows to be

"[{"key":"1000","error":"not_found"},{"id":"0","key":"0","value":{"rev":"1-fb8a93eb436b7e799a7bbc578a08e9a5"}},{"id":"2","key":"2","value":{"rev":"1-3a0bf449367880a229ea7c61f9394c83"}}]"

instead of the expected empty list. Similar problems crop up in the other test when it fails; i.e. the results for the previous query get returned for the current query (hence a list of 2 elements instead of the expected 3).

This leads me to believe that this is some kind of concurrency issue. I'm still no closer to solving it, but for what it's worth, that's what I see so far.

FYI, this seems to be a Safari bug, not a generic WebSQL bug. I tried WebSQL on Chromium and even the stock Android 4.2 browser - no problems on either one.

@nolanlawson
Member

I'm gonna try a crazy wrapper around the db object that forces every transaction to execute sequentially. Will post results later.

@nolanlawson
Member

Updated to Safari 6.1.1, still broken.

@nolanlawson
Member

Wow, okay, even with crazy logic to make every query and transaction execute sequentially, and even adding an artificial 10-second delay between transactions (!), the tests still fail in Safari. (The same crazy code passes using WebSQL on Chromium.) Time to admit I have no idea what's going on here.

@daleharvey
Member

Caching the databases does slightly better but still failing in replication here

@nolanlawson nolanlawson added a commit to nolanlawson/pouchdb that referenced this issue Jan 21, 2014
@nolanlawson nolanlawson (#1068) - Make async tests sequential for Safari
Ensure that asyncronous calls are sequentially executed,
which slows down the unit tests but ensures that Safari
doesn't break due to problems with multiple open
database handles.
2e0fd17
@nolanlawson
Member

So to update this issue a bit based on discussion in #1172, I've now tested in both Safari 6 and Safari 7. Safari 6 has lots of errors (mostly related to replication), but it at least manages to finish the tests. Safari 7, on the other hand, times out on Create and Destroy Multiple Pouches and then the tests are permanently broken after that.

We may have to put a bounty on this, because it's a real pain debugging this sort of stuff in the Safari web inspector.

@daleharvey
Member

@nolanlawson you didnt make a PR from it, but r+'d and landed the caching daleharvey@6575e8d

with allDbs gone we get a big test bump, and every time we convert a test suite to the new format it also help, I think between the 3 of them we will get back to passing safari

@nolanlawson
Member

@daleharvey Awesome, I saw that. I can feel us getting closer, and this will definitely help us to get Cordova working too, which does have a big bounty on it.

@nolanlawson nolanlawson added the tests label Mar 16, 2014
@nolanlawson nolanlawson added a commit that referenced this issue Mar 25, 2014
@nolanlawson nolanlawson (#1068) - cache websql dbs
Safari 7 starts throwing DOM Exception 11 if
we open up too many db handles, which crashes
our unit tests. Caching them fixes that.
16ab53a
@nolanlawson nolanlawson added a commit to nolanlawson/pouchdb that referenced this issue Mar 25, 2014
@nolanlawson nolanlawson (#1068) - cache websql dbs
Safari 7 starts throwing DOM Exception 11 if
we open up too many db handles, which crashes
our unit tests. Caching them fixes that.

We actually already tried this once before in
6575e8d, but
we forgot to put the cachedDatabases outside
the constructor method.
58a3529
@nolanlawson nolanlawson added a commit that referenced this issue Mar 26, 2014
@nolanlawson @daleharvey nolanlawson + daleharvey (#1068) - cache websql dbs
Safari 7 starts throwing DOM Exception 11 if
we open up too many db handles, which crashes
our unit tests. Caching them fixes that.

We actually already tried this once before in
6575e8d, but
we forgot to put the cachedDatabases outside
the constructor method.
0ff464c
@daleharvey
Member

All green in safari :)

daleharvey@0ff464c

@daleharvey daleharvey closed this Mar 26, 2014
@nolanlawson nolanlawson added a commit that referenced this issue Mar 26, 2014
@nolanlawson nolanlawson (#1068) - cache websql dbs
Safari 7 starts throwing DOM Exception 11 if
we open up too many db handles, which crashes
our unit tests. Caching them fixes that.

We actually already tried this once before in
6575e8d, but
since db.destroy() didn't use the cached
versions, the tests still failed.
4844955
@sygi sygi added a commit to sygi/pouchdb that referenced this issue Apr 28, 2014
@daleharvey @sygi daleharvey + sygi (#1068) - Cache open webSQL databases 24814c9
@sygi sygi added a commit to sygi/pouchdb that referenced this issue Apr 28, 2014
@nolanlawson @sygi nolanlawson + sygi (#1068) - cache websql dbs
Safari 7 starts throwing DOM Exception 11 if
we open up too many db handles, which crashes
our unit tests. Caching them fixes that.

We actually already tried this once before in
6575e8d, but
we forgot to put the cachedDatabases outside
the constructor method.
c80159e
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment