WorryFree Computers   »   [go: up one dir, main page]

Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

enablePersistence not working on Safari v14 #4076

Closed
burtonator opened this issue Nov 16, 2020 · 37 comments · Fixed by #5166
Closed

enablePersistence not working on Safari v14 #4076

burtonator opened this issue Nov 16, 2020 · 37 comments · Fixed by #5166

Comments

@burtonator
Copy link

[REQUIRED] Describe your environment

  • Operating System version: MacOS 10.15.6 (19G2021)
  • Browser version: Mozilla/5.0 (Macintosh; Intel Mac OS X 10_15_6) AppleWebKit/605.1.15 (KHTML, like Gecko) Version/14.0 Safari/605.1.15
  • Firebase SDK version: 8.0.2
  • Firebase Product: auth, firestore

[REQUIRED] Describe the problem

When calling enablePersistence in Safari it just locks and blocks and never returns.

The weird thing is if I do it 3-4 times it seems to break out of whatever was locking it up.

Then it works fine and works like my normal app does under Chrome.

However, if I clear all the browser data again, and do it a second time, it will lock up again.

Steps to reproduce:

Probably just clear your cache, and try to enablePerisistence with synchronizeTabs=true

@burtonator
Copy link
Author

synchronizeTabs=false doesn't fix it

The issue is just enablePersistence. With this enabled it won't work.

@wu-hui
Copy link
Contributor
wu-hui commented Nov 17, 2020

I tried reproduce on Safari Desktop 14 with firebase 8.0.2 with below code:

firebase
      .firestore()
      .enablePersistence({
        synchronizeTabs: true
      }).then(() => {
    db.collection("abc").onSnapshot((snapshot => {
      snapshot.docChanges().forEach(change => {
        console.log(`change type ${change.type}`);
      });
    }));
  });

And it works, as i can see the doc change printed to the console right away. Can you share your code?

@wu-hui wu-hui self-assigned this Nov 17, 2020
@burtonator
Copy link
Author

I think what I'll do is compile a small little app to reproduce it. It's definitely locking up in enablePersistence. It seems to eventually work at which point it doesn't have this issue anymore.

Did you clear all your data for this origin? Thats' the only way I can reproduce it. It will only work when there's no existing firestore login and no indexeddb data there.

@burtonator
Copy link
Author

This basic code snippet in typescript locks up for us:

        const app = firebase.initializeApp({
            ...conf
        })

        const firestore = firebase.firestore(app);

        const settings = {
            // timestampsInSnapshots: true
            cacheSizeBytes: firebase.firestore.CACHE_SIZE_UNLIMITED
        };

        firestore.settings(settings);

        console.log("Enabling firestore persistence....");
        await firestore.enablePersistence({synchronizeTabs: true});
        console.log("Enabling firestore persistence....done");

@burtonator
Copy link
Author

I can remove CACHE_SIZE_UNLIMITED and synchronizeTabs and it still locks up. It happened on my last MacOS box too but Safari wasn't super important.

@boldtrn
Copy link
boldtrn commented Jun 21, 2021

I just stumbled across this issue. When checking the network tab, no data was send to firestore. When I disable enablePersistence it works just fine.

Version 14.1.1 (16611.2.7.1.4)

firebase
    .firestore()
    .enablePersistence({ synchronizeTabs: true })
    .catch((err) => {
        console.error('Cannot enable firestore offline persistence', err);
    });

@burtonator
Copy link
Author

@boldtrn I think there might be an interaction with JS console.

What I think might be happening is that the SECOND time you're able to get it to work. Something weird is happening with it just needing to be initialized. Once it's working it just continues to work.

@keeth
Copy link
keeth commented Jun 30, 2021

@RERepos
Copy link
RERepos commented Jun 30, 2021

MORE DETAIL TO SHARE THAT I AM OBSERVING

OS/Safari Versions:

  • I encountered an issue where my app hung up getting firestore data a couple of days ago when testing my app development on my iPhone (iOS 14.6).
  • I use a PC and chrome for dev. So, I tested on on of my daughter's old Macs using OS 10.14.2 and Safari 12.0.2 (very out of date), and the issue is there also on these old versions. When I comment out the enablePersistence snippet, then the app works and the firestore get()'s are retrieving data (and, also works on my iPhone).
  • I tested on my other daughter's mac and updated the system to OS 11.4 and Safari 14.1.1, and the issue is on this mac too. Commenting out enablePersistence allows the app to work and firestore get()'s are retrieving data.

So, based on what I've experienced, I don't think its related to the new known Safari bug (still, a very bad thing) mentioned by Keeth.

More on what I'm observing:

  • I have a multi-page app.
  • If I navigate to a page that has a firestore get() and use Safari Develop/EmptyCaches, then: 1) I can work on the page and all of the get() calls work properly, 2) Looking at the network tab, I see a number of firestore related calls. First is getAccountInfo (which may be from an onAuthStateChanged call). Then, there are a number of 'channel' calls, which I believe are related to the get() calls retrieving data.
  • Now, if I navigate to another page that has firestore get() calls, the app gets hung up on the first get(). Looking at the network tab, getAccountInfo completes. But, there is not a single listing of a 'channel' call.
  • Now, if I return to the original page, the app is hung up on the first get(), just like when I navigated to the second page.
  • Now, if I again use Develop/EmptyCaches on the original page, then all of the get() calls on the page work again, just as in the beginning.

Everything is working fine on my PC using Chrome (observing all of the 'channel' calls on the dev tools network tab.

I hope there can be some kind of resolution to this soon. Turning off enablePersistence to get my app to work and not hang is takes away a lot of value to the app.

@schmidt-sebastian
Copy link
Contributor

@RERepos Can you share debug logs? I can take a quick look tomorrow or on Friday (after which I will be out for a week).

@burtonator
Copy link
Author

@keeth good catch! This sounds like the issue but I've seen this bug way earlier.

I have a potential workaround.

  • check for a localStorage property like 'safari.indexdb.workaround'
  • if it doesn't exist, init firestore, but set a timeout for say 200ms later that redirects the user back to the same app, after we set 'safari.indexdb.workaround' to true.

If it's true that it works the SECOND time this would fix the problem.

I've seen the exact same behavior and at first didn't understand what was happening.

Another idea could be to see if the IndexDB index exists or something along those line.

@burtonator
Copy link
Author

Apparently this is a workaround too:

https://twitter.com/feross/status/1404569098294501380

CleanShot 2021-06-30 at 16 58 34@2x

@RERepos
Copy link
RERepos commented Jun 30, 2021

@RERepos Can you share debug logs? I can take a quick look tomorrow or on Friday (after which I will be out for a week).

@schmidt-sebastian,

I'd love to help. But, I'm a self-taught dev. So, I need a little help understanding what you are looking for, and how to obtain it.

Are these logs from the safari console? There is nothing there unless I intentionally insert a console.log. I know that firebase functions have a log, but I don't see a log for firestore?

@schmidt-sebastian
Copy link
Contributor

@RERepos You just need to invoke firebase.firestore.setLogLevel('debug'). This will print the logs to the browser console.

@RERepos
Copy link
RERepos commented Jul 1, 2021

@schmidt-sebastian

Here are the logs:
[Log] [2021-07-01T04:49:51.030Z] @firebase/firestore: – "Firestore (8.6.8): FirebaseCredentialsProvider" – "Auth detected" (firebase-firestore.js, line 1)
[Log] [2021-07-01T04:49:51.315Z] @firebase/firestore: – "Firestore (8.6.8): FirestoreClient" – "Received user=" – "8lG935Ld5mNEmKEGDwsJg9REmfK2" (firebase-firestore.js, line 1)
[Log] [2021-07-01T04:49:51.315Z] @firebase/firestore: – "Firestore (8.6.8): FirestoreClient" – "Initializing OfflineComponentProvider" (firebase-firestore.js, line 1)
[Log] [2021-07-01T04:49:51.316Z] @firebase/firestore: – "Firestore (8.6.8): IndexedDbPersistence" – "Starting transaction:" – "updateClientMetadataAndTryBecomePrimary" (firebase-firestore.js, line 1)
[Log] [2021-07-01T04:49:51.316Z] @firebase/firestore: – "Firestore (8.6.8): SimpleDb" – "Opening database:" – "firestore/[DEFAULT]/fretdrills/main" (firebase-firestore.js, line 1)
[Log] [2021-07-01T04:55:24.894Z] @firebase/firestore: – "Firestore (8.6.8): AsyncQueue" – "Visibility state changed to hidden" (firebase-firestore.js, line 1)
[Log] [2021-07-01T04:57:17.819Z] @firebase/firestore: – "Firestore (8.6.8): AsyncQueue" – "Visibility state changed to visible" (firebase-firestore.js, line 1)

@schmidt-sebastian
Copy link
Contributor

@RERepos Are those all the logs you have? If so, then it looks like the SDK gets stuck waiting for IndexedDB. We could put a time limit on IndexedDB and continue without persistence if it does not respond within a certain amount of time. Would that work for your use case?

@RERepos
Copy link
RERepos commented Jul 1, 2021

@schmidt-sebastian

Thank you for your reply!

  1. "Are those all the logs you have?"
  • Yes, on Safari. On Chrome (on PC), the log is quite long.
  1. "We could put a time limit on IndexedDB and continue without persistence if it does not respond within a certain amount of time. Would that work for your use case?"
  • I believe so. I hope that this would allow me to keep the enablePersistence code snippet, so that users from the non-Apple world can get the value from persistence and work offline, while the users from the Apple world would just have to deal with working online only. I've been trying work-arounds to no avail, so my alternative to prevent bricking people from the Apple world is to turn off persistence for everyone.

@schmidt-sebastian
Copy link
Contributor

Thanks for the quick reply. Sounds like we have a path forward then. I will try to get this fixed before the end of the week (I am out next week).

@burtonator
Copy link
Author

@RERepos @schmidt-sebastian I think a time based bound is not the right strategy and would be considered harmful and actively a BAD solution.

Please do not do this.

  1. if the machine is heavily loaded at the time you're going to disable caching just because the computer is under heavy load. That's wrong and will create bugs. We're an offline app and when we tell you to go offline I don't expect magic to happen and it revert to not working. Locking up is preferred actually.

  2. There are other bugs related to Firestore being slow to init IndexedDB since it doesn't use indexes. That will trigger you to kick in this fix thus causing other bugs.

Can you try this instead?

image

... you should try this approach FIRST.

You would put this as the first line of the Firestore SDK so that an import() require() would immediately reference IndexedDB thus bypassing the bug.

That's a much cleaner way of resolving this.

This is really impacting us and we can't ship our iOS app without this fixed so I don't mind giving you feedback on how this is working as a resolution.

We're going to try this in our code since we don't rely on Firestore and I'll try to follow up once we have feedback.

But please please please don't to the above as that's actively harmful. Or at LEAST allow us to disable that behavior.

@RERepos
Copy link
RERepos commented Jul 1, 2021

I'm very sympathetic and understanding, that everyone may have different needs.

"we don't rely on Firestore"

My situation is that I do rely on Firestore ...

I thank everyone for their understanding, and hope there can be a solution for everyone's needs ...

@schmidt-sebastian
Copy link
Contributor

We can certainly just try to reference IndexedDB first. This is something that can be done outside of our SDK already, so @RERepos could see if this works.

If we do put a time-guard, it would only be around obtaining the IndexedDB file descriptor. Unfortunately, I cannot reproduce this bug locally (Safari 14.4.1 on OS X 11.4) so any solution will rely on feedback from the community. I will try a couple of different version on the iPhone Emulator.

@RERepos
Copy link
RERepos commented Jul 1, 2021

I placed this at the start:
const appleBugWorkaround = indexedDB.open('test-db1', 1);

  • It works with Safari on iPhone (have not connected to look at logs)
  • It doesn't work with Chrome on iPhone
  • It doesn't work with Safari on Mac (same logs as before)

@burtonator
Copy link
Author

@RERepos

Sorry... I misspoke.

We're going to try this in our code since we don't rely on Firestore and I'll try to follow up once we have feedback.

... I mean't to say I don't want to rely on Firestore fixing this and we can test it ourselves directly.

We heavily rely on Firestore.

I thank everyone for their understanding, and hope there can be a solution for everyone's needs ...

This will actively break for us and other people if the timeout proposal is implemented.

If we do put a time-guard, it would only be around obtaining the IndexedDB file descriptor. Unfortunately, I cannot reproduce this bug locally (Safari 14.4.1 on OS X 11.4) so any solution will rely on feedback from the community. I will try a couple of different version on the iPhone Emulator.

It seems to only happen the FIRST time. Maybe you'd have to wipe out all your IndexDB data and retry?

I agree it's frustrating and I think that's why no one has caught this bug before.

@RERepos
Copy link
RERepos commented Jul 1, 2021

I've never used indexDB before ...

I tried this at the start:

indexedDB.open('test-db1', 1);
indexedDB.deleteDatabase('test-db1');
indexedDB.open('test-db1', 1);

Yielded the same result (iphone/Safari worked, iphone/Chrome doesn't work, Mac/Safari doesn't work (with same logs))

If you have a code snippet you'd like me to try, then I'd be happy to run it.

Otherwise, I'm at the mercy of Firestore/Apple ...

@RERepos
Copy link
RERepos commented Jul 1, 2021

I'm reading some of the comments on the webkit bug report: https://bugs.webkit.org/show_bug.cgi?id=226547

////////
"Does the workaround work for you too? It basically boils down to adding another open call for a database that you don't care about before the one that you need to work. Something like indexedDB.open("dummy", 1) appears to be enough for me, but I'd be interested if that works for PWA added to the home screen too.

Of course I'd also welcome an update to fix this ASAP but as Safari is built into the iOS image, it means that updates are dependent on OS updates, so workarounds are usually required in practice for these kind of things. Apple also only tends to pull updates from the upstream webkit project every 6 months or so I believe."
/////////

My takeaway is:

  • I've tried the workaround proposed ... no joy
  • The Apple release cycle means there is going to a large infected customer base for quite some time
  • I'm hoping Firebase can offer some option to workaround this awful Apple screw-up ... 'time-guard' or otherwise ...

@burtonator
Copy link
Author

@RERepos thanks for looking into this workaround!

When you say it 'doesn't work' on iPhone chrome I guess what you're saying is that since Chrome on iOS has to use Safari this workaround does not work there?

If we can at least duplicate this on Safari / Mac that makes it easier to track down.

@burtonator
Copy link
Author

OK... just reporting back. Both workaround attempts did not work in our app.

I tried referencing IDB earlier and creating an empty database on it - that didn't work.

I tried using requestAnimationFrame - that didn't work either.

@burtonator
Copy link
Author

Is there any way we can get the core Firestore devs to acknowledge this and give us an update or some things we can test?. Firestore being broken for a major browser platform is really bad and I think it deserves some attention.

@burtonator
Copy link
Author

Hopefully I spoke too soon here.

I think this might work.

Our problem was that during login we were calling redirect and apparently this bug is caused by redirect when IndexDB is being used.

What I did is to try firestore.terminate() before we redirected.

This seems to work but I want to test more. I'm also using requestAnimationFrame to init Firestore and not sure if that is important too.

The key aspects to test though are:

  • change to about:blank in Safari
  • go into Setting | Manage Website Data and "Remove All" site data so
  • Quit and restart Safari

... this is needed for EACH test

When it breaks, if I just restart Safari then it works just fine. so that's not an indication that this is fixed.

The bug only happens when the database is opened initially.

@burtonator
Copy link
Author

OK.. I think that seems to fix it on our end. You have to call firestore.terminate() before you redirect the user and requestAnimationFrame might also need to be called.

@mlahp7
Copy link
mlahp7 commented Jul 14, 2021

We are having the same issues on Safari browsers (macOS). Updated firebase just in case and still no luck.
@schmidt-sebastian Below are our logs from Safari 14, they aren't much different than the ones posted above:

[Log] [2021-07-14T18:28:43.959Z]  @firebase/firestore: – "Firestore (8.7.0): FirebaseCredentialsProvider" – "Auth detected" (vendor.js, line 262371)
[Log] [2021-07-14T18:28:44.724Z]  @firebase/firestore: – "Firestore (8.7.0): FirestoreClient" – "Received user=" – "dG8oBAoqmAY6eKpzWAvT" (vendor.js, line 262371)
[Log] [2021-07-14T18:28:44.738Z]  @firebase/firestore: – "Firestore (8.7.0): FirestoreClient" – "Initializing OfflineComponentProvider" (vendor.js, line 262371)
[Log] [2021-07-14T18:28:44.740Z]  @firebase/firestore: – "Firestore (8.7.0): IndexedDbPersistence" – "Starting transaction:" – "updateClientMetadataAndTryBecomePrimary" (vendor.js, line 262371)
[Log] [2021-07-14T18:28:44.741Z]  @firebase/firestore: – "Firestore (8.7.0): SimpleDb" – "Opening database:" – "firestore/[DEFAULT]/dev-damotech-platform/main" (vendor.js, line 262371)

@burtonator thanks for the potential work around. We are also doing a redirect on login, and tried a firestore.terminate() just before the redirect. Unfortunately firestore is still hanging for us. Were you doing something specific with requestAnimationFrame?

@burtonator
Copy link
Author

@mlahp7 I'm initializing in requestAnimationFrame and then doing a terminate() with await before redirecting.

It seems to work but I don't have a confirmation that it's resolved in mobile Safari yet. IT does seems to be resolved on my desktop though.

@schmidt-sebastian
Copy link
Contributor

I haven't abandoned this issue, but I don't see a clear way forward for us yet. We currently close IndexedDb on pagehide (https://github.com/firebase/firebase-js-sdk/blob/master/packages/firestore/src/local/indexeddb_persistence.ts#L664) but this is executed after some other cleanup code and might not run. I can provide a custom build of Firestore that just closes IndexedDB to see if this would help. Let me know if there is interest for this - it might remove the need to call terminate() manually, but it would not affect reloads after purging the cache.

@ghost
Copy link
ghost commented Jul 16, 2021

I haven't abandoned this issue, but I don't see a clear way forward for us yet. We currently close IndexedDb on pagehide (https://github.com/firebase/firebase-js-sdk/blob/master/packages/firestore/src/local/indexeddb_persistence.ts#L664) but this is executed after some other cleanup code and might not run. I can provide a custom build of Firestore that just closes IndexedDB to see if this would help. Let me know if there is interest for this - it might remove the need to call terminate() manually, but it would not affect reloads after purging the cache.

Thanks for the update. Our team is using AngularFire and noticed these issues fairly recently.
We have tried:

Nothing seems to work with us.

Simply clearing cache via the developer menu seems to fix the issue (Option + Command + E), but it is not something we can ask our clients to do.

That being said, I am up for testing any potential fixes / workarounds.

@RERepos
Copy link
RERepos commented Jul 16, 2021

I'm at the point where I don't have time to try more roll-your-own workarounds. I would love to see something from the Firebase team, if that's possible, and I'd be willing to test that out. But for now, I'm turning off the feature by default, and having users turn on themselves if it works on their system.

@schmidt-sebastian
Copy link
Contributor

The PR linked above will (once released) skip our IndexedDB cleanup code for Safari 14. In general, the SDK tries to do a graceful shutdown, which includes writing to IndexedDB (to remove any locks and to persist outstanding mutations). This was always done on a best effort basis and we never relief on this code to run to completion. If I read the latest user comments correctly, these writes to IndexedDB could increase the frequency of the bug.

@schmidt-sebastian
Copy link
Contributor
schmidt-sebastian commented Jul 19, 2021

I have a relatively simple repro that shows that #5166 addresses one of the main causes of this problem. Repro is below:

async function run() {
  const fs = firebase.firestore();
  await fs.enablePersistence();
  
  const doc = fs.doc("coll/doc");
  
  await doc.set({foo:'foo'});
  
  for (let i = 0; i < 10; ++i) {
    doc.set({foo:'foo'});
  }
  
  setTimeout(() => {
	 window.location.href = 'https://abc/index.html?foo=new';
  }, 500);
}

This code likely writes a mutation to IndexedDB during page close, and hence causes the reload to fail. With the fix, the writes are not written to disk when run with Safari 14. Instead, they are not persisted, but the reload succeeds. If you need to persist all writes, you need to await termination.

Note that even during normal operation, it is not guaranteed that the client can persist outstanding writes during a page reload.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

8 participants