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.

Sign up for GitHub

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

Jump to bottom

Mutating data with Firestore multi-tab-persistence from a browser tab which is not the leader tab causes stale data to emit #6511

Closed
jorroll opened this issue Aug 6, 2022 · 10 comments · Fixed by #6619
Closed

Mutating data with Firestore multi-tab-persistence from a browser tab which is not the leader tab causes stale data to emit #6511

jorroll opened this issue Aug 6, 2022 · 10 comments · Fixed by #6619
Assignees
@wu-hui
Labels
api: firestore

Comments

@jorroll
Copy link

jorroll commented Aug 6, 2022 •
edited
Loading

[REQUIRED] Describe your environment

  • Operating System version: macOS 12.4 (though the reproduction is running inside a devcontainer)
  • Browser version: Google Chrome 103.0.5060.134
  • Firebase SDK version: 9.9.2
  • Firebase Product: Firestore

[REQUIRED] Describe the problem

When using the Firestore SDK with enableMultiTabIndexedDbPersistence(), if you open two tabs to the app and you perform a mutation in the second tab (i.e. the tab that was opened second) Firestore will emit an update in response to the change which contains the expected data, then it will emit stale data, then it will emit the correct data again. This happens quickly, I think in less than 100ms (possibly a lot less than 100ms). Meanwhile, the first tab will only emit the correct data (as expected). If you perform the same mutation in the first tab, you do not encounter this error (though the second tab still seems to emit stale data briefly).

My theory is that the bug is triggered when enableMultiTabIndexedDbPersistence is turned on and a client tab which is not the Firestore internal "leader tab" attempts to make a change.

Receiving correct data, then stale data, then correct data again is proving very problematic because there doesn't appear to be a good way for the app to tell that the data is stale (other than, perhaps, debouncing Firestore emissions which feels potentially brittle and will slow the UI down). It's possible I'll need to disable enableMultiTabIndexedDbPersistence as a work around. I'll also note that, in attempting to diagnose this issue, I may have been able to recreate it when persistence was turned off (still using two tabs), but I'm not positive and, regardless, this reproduction only focuses on the scenerio where multi-tab-persistence is enabled.

Note: I ran into this bug in production, though the included repository (below) reproduces the bug using the emulators.

Steps to reproduce:

See this video showing the bug

Here is a repository that can be cloned to reproduce the bug. Note it has an included .devcontainer configuration for VSCode if you're into that sort of thing.

Relevant Code:

See the included video and repository.
@jorroll jorroll changed the title Mutating data with multi-tab-persistence from a client tab which is not the leader tab causes stale data to emit Mutating data with Firestore multi-tab-persistence from a client tab which is not the leader tab causes stale data to emit Aug 6, 2022
@jorroll jorroll changed the title Mutating data with Firestore multi-tab-persistence from a client tab which is not the leader tab causes stale data to emit Mutating data with Firestore multi-tab-persistence from a browser tab which is not the leader tab causes stale data to emit Aug 6, 2022
@dconeybe dconeybe self-assigned this Aug 10, 2022
@dconeybe
Copy link
Contributor

Hi @jorroll. Thank you for taking the time make a thorough bug report. I'll take a look.

@dconeybe
Copy link
Contributor

I've been able to reproduce this issue for myself using a much simpler repro app: https://github.com/dconeybe/FirestoreJsIssue6511. There is definitely a bug here and I'll dig into it further.

@dconeybe
Copy link
Contributor

@jorroll Through bisection, it appears that this bug was introduced in v9.6.10. Could you try downgrading to v9.6.9 and see if that fixes this bug for you, as a temporary workaround?

@dconeybe
Copy link
Contributor

One workaround that came to mind would be to add a field to the documents with the value serverTimestamp(). And every time you update that document you also set it to serverTimestamp() again. Then, if you ever see this timestamp go back in time you can ignore that snapshot.

@jorroll
Copy link
Author

jorroll commented Aug 12, 2022

@jorroll Through bisection, it appears that this bug was introduced in v9.6.10. Could you try downgrading to v9.6.9 and see if that fixes this bug for you, as a temporary workaround?

Thanks so much for tracking this down @dconeybe! It looks like downgrading to v9.6.9 does indeed fix this bug. I tested both in my app as well as the reproduction repo I included when reporting this issue. This being said, I notice in the release notes for 9.6.10 that the format that Firestore data is persisted in has changed. I expect that we'd need to clear the Firestore indexedDB cache if we were to downgrade our production app? At the moment we'll probably stick with the current workaround of using single tab persistence only.

One workaround that came to mind would be to add a field to the documents with the value serverTimestamp(). And every time you update that document you also set it to serverTimestamp() again. Then, if you ever see this timestamp go back in time you can ignore that snapshot.

We already set an updatedAt property whenever a document changes so we wouldn't need to change our Firestore schema, but updating queries to filter on this would be non-trivial so, at the moment, I don't expect this is an option we'll pursue.

@jorroll
Copy link
Author

jorroll commented Aug 12, 2022

As an aside, because of a separate issue in the rxfire package, filtering on serverTimestamp() values locally would be a little more involved than it otherwise would.

@dconeybe
Copy link
Contributor

@jorroll Thanks for confirming that downgrading to 9.6.9 fixes the bug for you. You are right that deploying a new version of your app that downgrades from 9.9.2 to 9.6.9 would have issues with persistence. So you're right, that's probably not a viable option.

The serverSnapshot workaround should still be usable despite that rxfire issue, no? The heuristic would be that if the serverTimestamp is null then you consider that snapshot to be new data. And then when you get a subsequent snapshot you'd compare that new snapshot's serverTimestamp with the serverTimestamp value before the null one.

In any case, I'm continuing to investigate and will keep you posted.

@dconeybe
Copy link
Contributor

Just an update that investigation is continuing. No updates yet (I've just returned from vacation).

@dconeybe dconeybe assigned wu-hui and unassigned dconeybe Aug 22, 2022
@wu-hui wu-hui mentioned this issue Sep 22, 2022
Merged
@dconeybe
Copy link
Contributor

@jorroll Thank you for your patience on this issue. My colleague, @wu-hui, found the root cause and has fixed it. The fix will be included in the next release, which should come out mid-October 2022.

@jorroll
Copy link
Author

jorroll commented Sep 28, 2022

Thanks to both of you!

@firebase firebase locked and limited conversation to collaborators Oct 27, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees

@wu-hui wu-hui

Labels
api: firestore
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Fix document change leads to time travel across multiple tabs firebase/firebase-js-sdk
4 participants
@jorroll @google-oss-bot @wu-hui @dconeybe