Workaround for Safari issue with getAll(). #579

dfahlander · 2017-08-28T13:21:20Z

Resolves issue #565

timbru31 · 2017-09-29T09:59:18Z

This breaks the performance of e.g. Chrome, too and even the Safari (iOS 10.3, iOS 11) versions which do support IndexdedDB 2.0 and the getAll method.
Chrome user agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10_13_0) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/61.0.3163.100 Safari/537.36

See my stackoverflow question/issue regarding very, very bad performance. Testing with getAll instead results in 10x faster loading times for us.
Please consider reverting this issue - or at least make it somewhat configurable.

dfahlander · 2017-09-29T11:07:04Z

@timbru31 Thanks. I never like UA sniffing, but need to find a way to work around the bug of Safari idintified in the more recent comments of issue 565. If I roll back this PR we will get issues again on Safari: Version 10.1.2 (12603.3.8).

At least I must fix the false positive for Chrome (which is a major!) but can we identify the Safari issue some other way? Does this lib deal with that? https://github.com/raphinesse/dexie-batch ?

Any idea someone?

nponiros · 2017-09-29T12:11:32Z

Hm maybe navigator.vendor can help? On Mac OS Safari the vendor is "Apple Computer, Inc.". You should be able to detect Safari with this but you will probably have issues on iOS Chrome which will probably (didn't test) give vendor "Google Inc." although parts of Safari are used under the hood.

timbru31 · 2017-09-29T12:33:53Z

Besides that, getAll works fine for me on iOS 10.3 and iOS 11 - I'd like to keep support for these, too.

nponiros · 2017-09-29T12:56:56Z

We might be able to combine the vendor with the version from the userAgent but the test will probably be rather brittle..

timbru31 · 2017-09-29T13:41:48Z

navigator.appVersion might help to detect the iOS versions: https://developer.mozilla.org/en-US/docs/Web/API/NavigatorID/appVersion (deprecated though :/)

nponiros · 2017-09-29T14:58:15Z

appVersion on Mac OS Safari is basically the same as userAgent minus the "Mozilla" at the beginning of the UA string..

dfahlander · 2017-10-02T09:32:40Z

The issue in Safari 10.1 on iOS 10.3 is rather severe. Repro: http://dexie.org/test/dexie-issue-565/

dfahlander · 2017-10-02T09:33:29Z

Output: Testing Safari issue with IDBObjectStore.getAll()

Status: Preventing eternal reload. Last status was: Could put item. Now trying IDBObjectStore.getAll()

navigator.userAgent: Mozilla/5.0 (iPhone; CPU iPhone OS 10_3 like Mac OS X) AppleWebKit/603.1.30 (KHTML, like Gecko) Version/10.0 Mobile/14E269 Safari/602.1

navigator.vendor: Apple Computer, Inc.

navigator.appVersion: 5.0 (iPhone; CPU iPhone OS 10_3 like Mac OS X) AppleWebKit/603.1.30 (KHTML, like Gecko) Version/10.0 Mobile/14E269 Safari/602.1

dfahlander · 2017-10-02T09:37:37Z

Also on desktop mac, same issue:

navigator.userAgent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10_12_6) AppleWebKit/603.3.8 (KHTML, like Gecko) Version/10.1.2 Safari/603.3.8

navigator.vendor: Apple Computer, Inc.

navigator.appVersion: 5.0 (Macintosh; Intel Mac OS X 10_12_6) AppleWebKit/603.3.8 (KHTML, like Gecko) Version/10.1.2 Safari/603.3.8

dfahlander · 2017-10-02T09:44:10Z

Could someone try the repro on a newer Safari version to see if it is solved? Note that this happens from a WebWorker calling IDBObjectStore.getAll().

Unless we can see a non-failing Safari version, we need to keep preventing using getAll() on Safari (but keep Chrome untouched of course). If we see it's fixed, we could allow it functional versions only.

timbru31 · 2017-10-02T09:47:23Z

iPad with iOS 11.1 (15B5066f) is fine:

iPhone with iOS 11.0.1 (15A402) is fine

timbru31 · 2017-10-02T09:57:52Z

However, I noticed that running the code outside a Worker, the web page did not reload - this means we need to detect a Worker environment and the special Safari versions?

dfahlander · 2017-10-02T10:05:49Z

Ok, great, so Safari 604 is fine but not <=603. What about the following:

hasGetAll = 'getAll' in store &&
                !(/Apple/.test(navigator.vendor) &&
                  [].concat(navigator.userAgent.match(/Safari\/(\d*)/))[1] < 604);

timbru31 · 2017-10-02T10:15:37Z

What about the WebWorker check, too? Something like this could work: https://stackoverflow.com/a/18002694/1902598

see my test: https://pastebin.com/iC9jbmmZ (just logs to the console)

dfahlander · 2017-10-02T10:22:04Z

I haven't checked that it's only an issue in workers. I'll test now.

dfahlander · 2017-10-02T10:38:54Z

Tested. Issue seems only tied to workers, so I could use that check. But I found that navigator.vendor is not accessable from workers :( only navigator.userAgent seem to be accessable. So I have to do something else than /Apple/.test(navigator.vendor). Suggestion?

dfahlander · 2017-10-02T11:14:49Z

I've updated the check to:

// Bug with getAll() on Safari ver<604 on Workers only, see discussion following PR #579
if (/Safari/.test(navigator.userAgent) &&
    !/Chrome/.test(navigator.userAgent) &&
    _global.WorkerGlobalScope && _global instanceof _global.WorkerGlobalScope &&
    [].concat(navigator.userAgent.match(/Safari\/(\d*)/))[1] < 604)
{
    hasGetAll = false;
}

Thoughts?

From discussions on PR #579

dfahlander · 2017-10-02T11:38:43Z

Ah! Edge also has "Safari" in UA! Another false positive. Need to update yet again.

if (/Safari/.test(navigator.userAgent) &&
    !/(Chrome\/|Edge\/)/.test(navigator.userAgent) &&
    _global.WorkerGlobalScope && _global instanceof _global.WorkerGlobalScope &&
    [].concat(navigator.userAgent.match(/Safari\/(\d*)/))[1] < 604)
{
    hasGetAll = false;
}

PR #579 was meant to work around a severe Safari issue with IDBObjectStore.getAll() that when called from a Web worker, results in a page crash followed by an automatic page reload. Since getAll() is just an optimization and not needed for IndexedDB to work, PR 579 shut down this optimization of utilizing on Safari. Due to the nature of the Safari bug (page crash + reload), feature testing was not possible - we could only do user-agent sniffing to avoid the bug. The user-agent sniffing in PR #579 did only look for "Safari" in user-agent, which made it also affect Chrome and Opera browsers (which was not the intent). Also, the bug has since been fixed in a later version of Safari + it only happened when called from a web worker, so we needed a more fine grained control of whether the getAll() optimization should be avoided or not. This new PR will replace the user-agent code from PR #579 with another more fine grained check that only disables getAll() when: * browser is Safari (user agent contains "Safari" but not "Chrome" or "Edge") * we are in a Worker environment * Safari internal version number is less than 604, as the issue has been fixed in 604.

Sopamo · 2019-05-22T15:02:36Z

Sorry for opening this issue up again. I'm pretty sure I'm running into this issue again on an iPhone SE iOS 10.3 emulator using a webkit view (cordova). navigator.userAgent reports:
Mozilla/5.0 (iPhone; CPU iPhone OS 10_3_1 like Mac OS X) AppleWebKit/603.1.30 (KHTML, like Gecko) Mobile/14E8301 which doesn't contain "Safari", so the user agent sniffing doesn't work in this case. I'm currently unable to check if the issue only affects emulated devices because I do not own an iOS device running iOS 10. I'd be pretty surprised if the user agent differs between a real device and an emulator though.
Upon calling table.toArray() the app just crashes. Loading a single item via .get() works fine.

Edit: I can confirm that it fixes the bug when I manually set hasGetAll to false.

dfahlander · 2019-05-22T15:47:06Z

If you use dexie@3 it will work the same on all browsers. That is, it will avoid this issue without UA sniffing

Workaround for Safari issue with getAll().

1cc0e7b

Resolves issue #565

dfahlander merged commit 60e0ef8 into master Aug 29, 2017

dfahlander deleted the issue565 branch August 30, 2017 08:21

dfahlander mentioned this pull request Oct 2, 2017

Maximum call stack exceeded when running Dexie with zone.js 0.8.18 #589

Closed

dfahlander added a commit that referenced this pull request Oct 2, 2017

Resolves performance issue identified in PR #579

db244a8

dfahlander added a commit that referenced this pull request Oct 2, 2017

Fine-grained Safari workaround for getAll()

d9c358b

From discussions on PR #579

This was referenced Oct 2, 2017

SecurityError on Safari #565

Closed

Partially revert pr 579 #591

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Workaround for Safari issue with getAll(). #579

Workaround for Safari issue with getAll(). #579

dfahlander commented Aug 28, 2017

timbru31 commented Sep 29, 2017 •

edited

dfahlander commented Sep 29, 2017 •

edited

nponiros commented Sep 29, 2017

timbru31 commented Sep 29, 2017

nponiros commented Sep 29, 2017

timbru31 commented Sep 29, 2017

nponiros commented Sep 29, 2017

dfahlander commented Oct 2, 2017

dfahlander commented Oct 2, 2017

dfahlander commented Oct 2, 2017

dfahlander commented Oct 2, 2017 •

edited

timbru31 commented Oct 2, 2017

timbru31 commented Oct 2, 2017

dfahlander commented Oct 2, 2017 •

edited

timbru31 commented Oct 2, 2017 •

edited

dfahlander commented Oct 2, 2017

dfahlander commented Oct 2, 2017 •

edited

dfahlander commented Oct 2, 2017

dfahlander commented Oct 2, 2017

Sopamo commented May 22, 2019 •

edited

dfahlander commented May 22, 2019 •

edited

Workaround for Safari issue with getAll(). #579

Workaround for Safari issue with getAll(). #579

Conversation

dfahlander commented Aug 28, 2017

timbru31 commented Sep 29, 2017 • edited

dfahlander commented Sep 29, 2017 • edited

nponiros commented Sep 29, 2017

timbru31 commented Sep 29, 2017

nponiros commented Sep 29, 2017

timbru31 commented Sep 29, 2017

nponiros commented Sep 29, 2017

dfahlander commented Oct 2, 2017

dfahlander commented Oct 2, 2017

dfahlander commented Oct 2, 2017

dfahlander commented Oct 2, 2017 • edited

timbru31 commented Oct 2, 2017

timbru31 commented Oct 2, 2017

dfahlander commented Oct 2, 2017 • edited

timbru31 commented Oct 2, 2017 • edited

dfahlander commented Oct 2, 2017

dfahlander commented Oct 2, 2017 • edited

dfahlander commented Oct 2, 2017

dfahlander commented Oct 2, 2017

Sopamo commented May 22, 2019 • edited

dfahlander commented May 22, 2019 • edited

timbru31 commented Sep 29, 2017 •

edited

dfahlander commented Sep 29, 2017 •

edited

dfahlander commented Oct 2, 2017 •

edited

dfahlander commented Oct 2, 2017 •

edited

timbru31 commented Oct 2, 2017 •

edited

dfahlander commented Oct 2, 2017 •

edited

Sopamo commented May 22, 2019 •

edited

dfahlander commented May 22, 2019 •

edited