Fix b/72502745: OnlineState changes cause limbo document crash. #470
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Context: I made a previous change to raise isFromCache=true events when the client goes offline. As part of this change I added an assert for "OnlineState should not affect limbo documents", which it turns out was not valid because: * When we go offline, we set the view to non-current and call View.applyChanges(). * View.applyChanges() calls applyTargetChange() even though there's no target change and it recalculates limbo documents. * When the view is not current, we consider no documents to be in limbo. * Therefore all limbo documents are removed and so applyChanges() ends up returning unexpected LimboDocumentChanges. Fix: I've modified the View logic so that we don't recalculate limbo documents (and generate LimboDocumentChanges) when the View is not current, so now my assert holds and there should be less spurious removal / re-adding of limbo documents.
wilhuff
approved these changes
Jan 26, 2018
mikelehen
pushed a commit
to firebase/firebase-ios-sdk
that referenced
this pull request
Jan 26, 2018
[PORT OF firebase/firebase-js-sdk#470] Context: I made a previous change to raise isFromCache=true events when the client goes offline. As part of this change I added an assert for "OnlineState should not affect limbo documents", which it turns out was not valid because: * When we go offline, we set the view to non-current and call View.applyChanges(). * View.applyChanges() calls applyTargetChange() even though there's no target change and it recalculates limbo documents. * When the view is not current, we consider no documents to be in limbo. * Therefore all limbo documents are removed and so applyChanges() ends up returning unexpected LimboDocumentChanges. Fix: I've modified the View logic so that we don't recalculate limbo documents (and generate LimboDocumentChanges) when the View is not current, so now my assert holds and there should be less spurious removal / re-adding of limbo documents.
wilhuff
pushed a commit
to firebase/firebase-ios-sdk
that referenced
this pull request
Jan 27, 2018
#714) [PORT OF firebase/firebase-js-sdk#470] Context: I made a previous change to raise isFromCache=true events when the client goes offline. As part of this change I added an assert for "OnlineState should not affect limbo documents", which it turns out was not valid because: * When we go offline, we set the view to non-current and call View.applyChanges(). * View.applyChanges() calls applyTargetChange() even though there's no target change and it recalculates limbo documents. * When the view is not current, we consider no documents to be in limbo. * Therefore all limbo documents are removed and so applyChanges() ends up returning unexpected LimboDocumentChanges. Fix: I've modified the View logic so that we don't recalculate limbo documents (and generate LimboDocumentChanges) when the View is not current, so now my assert holds and there should be less spurious removal / re-adding of limbo documents.
rsgowman
added a commit
to firebase/firebase-ios-sdk
that referenced
this pull request
Mar 8, 2018
* Version bump for 4.8.1 Release Updated the version numbers of pods which are being released in 4.8.1 * Update from master Picking up a few last-minute changes to CHANGELOGs and tests for 4.8.1 release. * Update Core info for M21.1 Update missing version bump for M21.1 for FirebaseCore. * Remove over-aggressive assert. closeWithFinalState: assumes delegate != nil, but that is not true if when startWithdelegate: was called we entered backoff (performBackoffWithDelegate:) and so self.delegate did not get assigned yet. We could rework the code to make the assertion hold, but per offline discussion this assert doesn't represent an invariant that we care about and so I'm just removing it. * Remove over-aggressive closeWithFinalState: delegate assert. (#656) Fixes #596. closeWithFinalState: asserted delegate != nil, but that is not true if when startWithdelegate: was called we entered backoff (performBackoffWithDelegate:) and so self.delegate did not get assigned yet. We could rework the code to make the assertion hold, but per offline discussion this assert doesn't represent an invariant that we care about maintaining and so I'm just removing it. * Increase FirebaseAuth version for M21.1 This version was missed in the M21.1 version bump PR. * Fix import formatting (#660) * Fix issue @morganchen12 discovered where we weren't properly creating FIRQueryDocumentSnapshot instances. (#662) * Validate clang-format compliance in travis (#648) * Build gRPC for Firestore C++ (#652) * Clean up quoting and other minor issues * Reorganize CMake build output Make it clearer which parts of the output pertain to external projects. * Use a consistent ordering of ExternalProject arguments * Prevent the top-level build from running in parallel This prevents spurious failures when running make -j. * Actually parse arguments in the xcodebuild function * Use ExternalProject features when available * submodule limits from CMake 3.0 * shallow clones from CMake 3.6 * git progress output from CMake 3.8 * Only build the parts of leveldb we need Skip building the tools and other libraries * Avoid installing ExternalProjects Consume build output directly so that we can build just the targets we need. Installing causes all targets to be built. This doesn't matter as much for these targets but the gRPC build includes a ton of stuff we don't need so it's worth adopting this as a general strategy. * Define an external build for grpc * Test that grpc can link successfully. * Add a FindGRPC CMake module * Actually comment ExternalProjext_GitSource * Inject infoDictionary to fix flakey tests. (#664) * Inject infoDictionary to fix flakey tests. * Remove outdated comment, update format. * Fix issue @morganchen12 discovered where we weren't properly creating FIRQueryDocumentSnapshot instances. (#662) * Travis - run tests only for changed code (#665) * Add assert_test to the Xcode build (#671) * Exclude stdio-backed assert from the Xcode build * Add assert_test to the Xcode build * Enable warnings in the CMake build (#669) * Enable warnings when building with GCC or clang * Fix warnings * Fix C++ lint errors (#668) * Misc style.sh fixes * Allow test-only to use a revision; to check your changes since master: ./scripts/style.sh test-only master * Avoid diffing deleted files * 80 columns * Fix C++ lint errors * implement FieldValue for null, boolean, and array in C++. (#637) * implement FieldValue for null and boolean. * refactoring to use union type instead of poly * refactor using union design intead of poly * refactoring to use anonymous union and fix styles * small fix * add field_value_test to the project * fix warning of cmake and fix style * Simplify integration with googletest (#672) This makes it possible to build the Firestore subproject with CLion because it no longer needs to be told where googletest is. * Add a cc_library to the CMake build (#670) * Rewrite cc_test to take named arguments Cut down on build file verbosity by having cc_test take SOURCES and DEPENDS. The separate invocation of target_link_libraries is no longer necessary. * Add a cc_library rule to parallel cc_test This cuts down on build file verbosity. * Automatically add OBJC_FLAGS to cc_libraries if applicable * Exclude platform-specific libraries from 'all' This is makes it possible to declare this kind of library unconditionally. Usage within a test or as a dependency will actually trigger building. * Restore secure_random_test.cc; clean-up comments * Fix CMake build and lint warnings in field_value.cc (#677) Fixes these cpplint warnings: Firestore/core/src/firebase/firestore/model/field_value.cc:162: Semicolon defining empty statement. Use {} instead. [whitespace/semicolon] [5] Firestore/core/src/firebase/firestore/model/field_value.cc:170: Semicolon defining empty statement. Use {} instead. [whitespace/semicolon] [5] Firestore/core/src/firebase/firestore/model/field_value.cc:126: Add #include <utility> for swap [build/include_what_you_use] [4] * Listen sequence numbers (#675) * Generate and save sequence numbers for listens * Add documentation * Fix include path * Fix unavailable comment * Review feedback * Fix build warnings by making void parameter explicit (#681) * Add platform detection logic for SecureRandom (#676) * Add CMake platform detection logic for SecureRandom Now only builds secure_random_arc4random.cc if available. Remove firebase/firestore/base/port.h. Nothing else was in that directory. * Add a SecureRandom implementation that uses OpenSSL This is usable on Linux, Windows, and Android * Properly check return from RAND_bytes * Port comparison to C++ (#678) This reimplements our comparison functions as C++ Comparators and then provides compatibility shims for interoperating with existing Objective-C usage. A few specialized comparators aren't suitable for porting but only have a single usage (e.g. CompareBytes for comparing NSData * instances). In these cases I've moved them into the caller. * Use int32_t for typeof(ID) in FSTDocumentReference * Migrate callers of FSTComparison.h to Objective-C++ * Port comparison to C++ * Migrate usages of FSTComparison.h to C++ equivalents * Remove FSTComparison * Use Comparator in FieldValue. (#686) * style.sh - output unformatted files in test-only and ignore generated config.h (#690) * Cleanup imports and isEqual (#685) * Fix headers * Fix isEqual verbosity * Fix isEqual for nullable properties * Fix nullability on FSTTestDocSnapshot * Fixing spelling error in FIRStorageErrors * Update travis to use CocoaPods 1.4.0 (#692) * add FIRUser behavior to documentation * Disable Messaging build warnings (#697) * Update FirebaseMessaging protos (#696) * Adding enable/disable property to FCM token auto Initialization * revert unrelated changes * revert unrelated changes * revert unrelated changes * Adds explicit core graphics dependency to auth (#700) * Properly publish Abseil sources as a part of the podspec (#704) * Properly include abseil sources * Exclude abseil tests * Implement the rest of FieldValue types for C++ (#687) * implement FieldValue for null and boolean. * Implement number and string FieldValue. * Implement object FieldValue. * implement timestamp FieldValue. * Implement number and string FieldValue. * implement public type `Blob` and `GeoPoint` * implement Blob FieldValue * Implement GeoPoint FieldValue * refactoring `Blob` * README - FirebaseCore source pod must be included with all source pods (#705) * Fix incorrect deprecation message (#688) * Fix incorrect deprecation message for Obj-C and Swift * Update CHANGELOG for M21.1 release (#679) * normalize string_util (#708) * refactoring string_util * port string_util to iOS * implement `TargetIdGenerator` in C++ for Firestore (#701) * implement `TargetIdGenerator` * address changes * adding unit test for auto init enable function (#710) * port TargetIdGenerator to iOS (#709) * port TargetIdGenerator to iOS * fix style * move pointer property to instance variable * TriggerTravis * normalize and port the rest of Firebase/Port code (#713) * normalize bits * normalize ordered_code * Fix b/72502745: OnlineState changes cause limbo document crash. (#470) (#714) [PORT OF firebase/firebase-js-sdk#470] Context: I made a previous change to raise isFromCache=true events when the client goes offline. As part of this change I added an assert for "OnlineState should not affect limbo documents", which it turns out was not valid because: * When we go offline, we set the view to non-current and call View.applyChanges(). * View.applyChanges() calls applyTargetChange() even though there's no target change and it recalculates limbo documents. * When the view is not current, we consider no documents to be in limbo. * Therefore all limbo documents are removed and so applyChanges() ends up returning unexpected LimboDocumentChanges. Fix: I've modified the View logic so that we don't recalculate limbo documents (and generate LimboDocumentChanges) when the View is not current, so now my assert holds and there should be less spurious removal / re-adding of limbo documents. * travis: check for copyright in sources (#717) * Add the C++ linter to the repo (#720) * Fix a number of c++ build errors (#715) * Move -fvisibility-inlines-hidden from common_flags to cxx_flags This option isn't supported by C (and causes the build to fail under at least rodete). Manifested error was: ``` -- Looking for include file openssl/rand.h -- Looking for include file openssl/rand.h - not found CMake Error at core/src/firebase/firestore/util/CMakeLists.txt:94 (message): No implementation for SecureRandom available. -- Configuring incomplete, errors occurred! ``` which is completely misleading. :( * Fix exception include for std::logic_error Was causing compiler error on (at least) rodete. (http://en.cppreference.com/w/cpp/error/logic_error suggests that stdexcept is the correct header.) * Remove 'const' from vector contents. vectors cannot contain const objects as the contained objects are required to be assignable and copy constructable. (Not *quite* strictly true; since c++11, this requirement now depends on the operations performed on the container. But my compiler objects at any rate.) http://en.cppreference.com/w/cpp/container/vector https://stackoverflow.com/questions/8685257/why-cant-you-put-a-const-object-into-a-stl-container * Add brackets as suggested (required) by my compiler. * Add missing include For LLONG_MIN, etc. * Enable gnu extension to c++11 for firestore/util *test*. We disable them by default (in cmake/CompilerSetup.cmake) but our tests requires hexidecimal floating point, which is not supported in c++11 (though is supported in C++17, gnu++11, and others). http://en.cppreference.com/w/cpp/language/floating_literal https://bugzilla.redhat.com/show_bug.cgi?id=1321986 * Fix printf format for uint64_t http://en.cppreference.com/w/cpp/types/integer https://stackoverflow.com/questions/9225567/how-to-print-a-int64-t-type-in-c * Allow 0 length printf template strings in tests. * Get rid of a multi line comment. The backslash is apparently interpreted by the compiler cause the next line to be ignored too. In this case, the next line is (currently) a comment, and would be ignored anyways. (But that doesn't stop the compiler from yelling.) * Run ./scripts/style.sh * Version updates to 4.8.2 (#722) * Use fixed-sized types (#719) Should have caught this during review, but cpplint caught it * Import iterator_adaptors from google3 (#718) * Import iterator_adapters from google3 * Remove -Wconversion which is annoyingly hard to satisfy * Strip dependency on absl_container from iterator_adapters_test * Format and lint iterator_adaptors * More flexible copyright checking in Travis * Add changelog entry for my last PR (oops) and also add a few that we missed last release. (#724) * Add absl_strings to firebase_firestore_util_test dependencies (#725) Required to link the test on rodete. * Firestore DatabaseId in C++ (#727) * Implement DataBaseId in C++ * add database_id_test to project * fix project * address changes * fix style * Schema migrations for LevelDB (#728) * Implement schema versions * Style fixes * newlines, copyrights, assumptions * Fix nullability * Raw ptr -> shared_ptr * kVersionTableGlobal -> kVersionGlobalTable * Drop utils, move into static methods * Drop extra include * Add a few more comments * Move version constant into migrations file * formatting? * Fix comment * [FCM] Add completion handler to subscribe/unsubscribe topic actions Added functionality not exposed in public header yet. I would leave to next FCM SDK owner to expose this functionality (and follow any required review process). Bugs: 72701086 * Fix tests. * Move all Firestore Objective-C to Objective-C++ (#734) * Move all Firestore files to Objective-C++ * Update project file references * Don't use module imports from Objective-C++ * Use extern "C" for C-accessible globals * Work around more stringent type checking in Objective-C++ * NSMutableDictionary ivars aren't implicitly casted to NSDictionary * FSTMaybeDocument callback can't be passed a function that accepts FSTDocument * NSComparisonResult can't be multiplied by -1 without casting * Add a #include <inttypes.h> where needed * Avoid using C++ keywords as variables * Remove #if __cplusplus guards * Start on ArraySortedMap in C++ (#721) * Implement ArraySortedMap.remove * Implement ArraySortedMap.insert * Ensure ArraySortedMap.insert avoids copying on duplicates * Port more ArraySortedMapTests * Remove predecessorKey,Object,Document, etc (#735) This is dead code. I think it was probably useful in the RTDB because of the way it notified of changes, but we give changes with indexes in Firestore so I think we don't need it. * Align tests and integration test header search paths (#737) * fix (#739) * Increase expectation timeout to 25 seconds. (#744) I'm doing this to: 1) Experimentally see if it improves the flakiness we've been seeing in the Query Conformance Tests. 2) Insulate us from the fact that GRPC seems to take a /minimum/ of 10 seconds to reconnect (at least in some cases) after a connection failure. I've opened b/72864027 to revisit this in the future. * fix null block execution crash * Make sure Firestore/core/include is in the podspec (#748) * Skip 'update' step for external dependencies We check them out from a git tag, so this *should* be a noop. However, cmake seems to want to rebuild these dependencies every time you run make as it assumes the dependency *might* have been updated. (In practice, this isn't completely awful, as make notices the files haven't changed, so files don't actually get recompiled. But the configure step is still re-run and all the files still need to be rescanned.) Skipping the update step speeds up the build considerably. On my linux box, running: cmake .. && make -j && time make -j takes ~8.5s prior to this CL and ~6.5 afterwards. (6s is used by the test suite.) The upcoming protobuf addition would otherwise have made this much worse. (It takes a long time to ./configure.) * Add ability to build nanopb and protobuf * Add instructions for building nanopb protos Currently only supported on osx * Add generated nanopb protos. 100% machine generated (except adding of the license). * Import "well-known" protos (and generated nanopb files) For use by nanopb (which otherwise doesn't know what to do with these.) * Hook up nanopb to firestorep project Use remote/serializer placeholder class as a hook for the test to ensure nanopb headers can be found, and test can be linked. * style.sh now ignores generated nanopb files * ./style.sh * Eliminate sequencing dependencies We originally had this as it was thought that cmake would spawn n*m jobs (where n is the number of jobs it was instructed to create, ie. make -j8, and m is the number of external projects.) However, it isn't supposed to do that, and doesn't appear to be doing that right now. So we'll remove this. If the problem re-appears, we'll add these back in. (Symptom was being unable to spawn /bin/sh.) * Downgrade nanopb from 0.4.0-dev to 0.3.8. Also regenerate the protos * Minor refactoring * Ignore trailing whitespace in autogenerated nanopb files * Update abseil-cpp to a new upstream (#754) Update to bf7fc9986e20f664958fc227547fd8d2fdcf863e * Add StrCat, StrJoin and the rest of //strings from abseil-cpp (#756) From abseil-cpp version bf7fc9986e20f664958fc227547fd8d2fdcf863e * Fix xcode build errors re nanopb Involves adding PODS_ROOT/nanopb to include path (to allow include <pb.h>) and Firestore/Protos/nanopb to include path (to allow include "google/api/annotations.pb.h" and similar). In both cases, this is to allow auto-generated code to function properly. * Fix firebaes typo (#755) * Implement Firestore DatabaseInfo and port both Database{Id,Info} C++ to the iOS code (#738) * implement Firestore DatabaseInfo in C++ * temporary stash changes; blocking on the massive renaming of .m to .mm * add database_info_test to project * finish port DatabaseId and fix style, modular fixing DatabaseInfo * port DatabaseInfo * remove FSTDatabase{ID,Info} and their tests from project * fix unit test * use namespace alias * use namespace alias, leftover * address more changes * refactoring to use raw pointer instead of value for property * address changes * remove self-> * fix style * remove the name suffix Alloc * fix a bug * C++ port: port FSTFieldPath and FSTResourcePath to C++ (#749) Similar to Objective-C, FieldPath and ResourcePath share most of their interface (and implementation) by deriving from BasePath (using CRTP, so that factory methods in BasePath can return an instance of the derived class). * Creating CHANGELOG for FCM (#764) * add CHANGELOG for FCM * fix the typo * avoid using initWithSuiteName which does not support iOS 7 (#765) * Updates version of Firebase Auth in Changelog (#771) * Updates version of Firebase Auth in Changelog * addresses comments * Update CHANGELOG for Firestore v0.10.1 (#768) * Explicitly specify the default constructor to FieldPath (#773) Xcode prior to 8.3 does not accept an explicitly defaulted constructor (`= default`) for the purposes of default initializing a const object. This fixes a build failure under Xcode 8.2: ``` Firestore/core/src/firebase/firestore/model/field_path.cc:144:26: error: default initialization of an object of const type 'const firebase::firestore::model::FieldPath' without a user-provided default constructor static const FieldPath empty_path; ``` * Update deprecation message for Firestore. (#758) * Update FIRFirestore.h * Provide full command to enable debugging. * Version bumps for 4.9.0 (#774) * cmake build fixes (#770) * Fix nanopb (in cmake build) Look for binaries in the src dir (since that's where we build now.) This error would be masked if a previous build had completed prior to switching nanopb to build out of src. Also, don't patch the protoc path multiple times. This could be triggered by (eg) 'make && make clean && make'. * Add resource_path.{h,cc} to the cmake build * Fix signed/unsigned int comparison warnings * Ensure FieldValue tag_ is initialized during cp/mv ctor. Otherwise, the assignment operator attempts to deallocate based on the (uninitialized) tag_ variable, posssibly leading to segfaults. * Fix tests that throw exceptions. The (previous) tests checked to ensure that an abort() occurs, but if ABSL_HAVE_EXCEPTIONS is defined on non-macos (which is currently the default) then the assertions will throw a std::logic_error rather than abort()ing. On macos, an exception is thrown too, but the exception doesn't derrive from std::exception, so ASSERT_DEATH_* doesn't catch it (hence why ASSERT_DEATH_* actually works.) To resolve this, I've switched to ASSERT_ANY_THROW. * Let Travis run for `CMake` test and `lint.sh` (#769) * Fix nanopb (in cmake build) Look for binaries in the src dir (since that's where we build now.) This error would be masked if a previous build had completed prior to switching nanopb to build out of src. Also, don't patch the protoc path multiple times. This could be triggered by (eg) 'make && make clean && make'. * Add resource_path.{h,cc} to the cmake build * Fix signed/unsigned int comparison warnings * let Travis run for `CMake` test and `lint.sh` * Ensure FieldValue tag_ is initialized during cp/mv ctor. Otherwise, the assignment operator attempts to deallocate based on the (uninitialized) tag_ variable, posssibly leading to segfaults. * address change * fix trailing space * address change * moving Firestore checks closer together * Fix tests that throw exceptions. The (previous) tests checked to ensure that an abort() occurs, but if ABSL_HAVE_EXCEPTIONS is defined on non-macos (which is currently the default) then the assertions will throw a std::logic_error rather than abort()ing. On macos, an exception is thrown too, but the exception doesn't derrive from std::exception, so ASSERT_DEATH_* doesn't catch it (hence why ASSERT_DEATH_* actually works.) To resolve this, I've switched to ASSERT_ANY_THROW. * ./scripts/lint.sh * Move FieldValue::tag_ initializer to be in class. * check Travis ulimit * check travis limit * set make -j 200 instead of unlimited * use cpu core number instead of 200 * port Firestore Auth module in C++ (#733) * Implement firestore/auth/user * add user to project and some fixes * implement firestore/auth/{credentials_provider,empty_credentials_provider} * implement firestore/auth/firebase_credentials_provider * refactoring firebase_credentials_provider and add (disabled but working) unit test * add auth test to project * address changes * small fix to style and project * fix the firebase_credentials_provider_test * fix style * address changes * revert the change to static mutex_ * remove my custom plist path * fix style * address changes * refactoring FirebaseCredentialsProvider to fix the issue w.r.t. auth global dispatch queue * add /*force_refresh=*/ tag to bool literal for style purpose * Use a shared_ptr/weak_ptr handoff on FirebaseCredentialsProvider (#778) * Revert "refactoring FirebaseCredentialsProvider to fix the issue w.r.t. auth global dispatch queue" This reverts commit 87175a4. * Use a shared_ptr/weak_ptr handoff on FirebaseCredentialsProvider This avoids any problems with callsbacks retaining pointers to objects destroyed by a C++ destructor * port Firestore SnapshotVersion in C++ (#767) * implement SnapshotVersion and test * Fix Core CLANG_WARN_OBJC_IMPLICIT_RETAIN_SELF warnings (#779) * C++ port: add C++ equivalent of FSTDocumentKey. (#762) Also move kDocumentKeyPath to the only point of usage - make it a static member variable of FieldPath. * Strawman C++ API (#763) Incomplete, and what does exist in still slightly vague. It's expected that this will change. * Move to explicit nil check. (#782) * Update FieldValue of type Reference (#775) * update FieldValue of type Reference * address change * fix bad path string literal in test * use ReferenceValue and qualified name * Port Firestore Document to C++ (#777) * implement SnapshotVersion and test * update project * implement MaybeDocument and test * move snapshot-version from core to model * fix a bug * implement Document and test * implement NoDocument * adding type tag and fix style * fix a few bugs, discovered after merging and test run. * add assert to check FieldValue type and more test for comparision. * address changes * allow moving FieldValue to construct Document. * address changes * add document tests to project * use std::less convention * make Type::Unknown static initializer * Cleaning up implicit retain for the RTDB and Storage * Fix double parsing on 32 bit devices. (#787) * Keep track of number of queries in the query cache (#776) * Implement schema versions * Style fixes * newlines, copyrights, assumptions * Fix nullability * Raw ptr -> shared_ptr * kVersionTableGlobal -> kVersionGlobalTable * Drop utils, move into static methods * Drop extra include * Add a few more comments * Move version constant into migrations file * formatting? * Fix comment * Split add and update queryData * Work on adding targetCount * More work on count * Using shared_ptr * Implement count for query cache * use quotes * Add cast * Styling * Revert year bump in copyright * Add adversarial key to migration test * Add comment * Fix style * style fix * fix flaky test (#788) * Fix off-by-one error in Auth changelog (#789) * Serialize and deserialize null (#783) * Build (grpc's) nanopb with -DPB_FIELD_16BIT We require (at least) 16 bit fields. (By default, nanopb uses 8 bit fields, ie allowing up to 256 field tags.) Also note that this patch adds this to grpc's nanopb, rather than to our nanopb. We'll need to eventually either: a) we instruct grpc to use our nanopb b) we rely on grpc's nanopb instead of using our own. (^ marked as a TODO for now.) * Add some dependant protos Imported from protobuf. Nanopb requires these to be present (though anything using libprotobuf does not, as these are already built into that.) * Add generated nanopb protos based off of newly added proto definitions * Build the nanopb protos * Serialize and deserialize null * Actually ignore events on inactive streams, rather than just logging that we're going to. (#790) * Improve documentation on auto-init property in FCM. (#792) * Fixed capitalization of init in test function. (#797) * Update README.md with code markup (#800) Add code markup to a few paths for consistency. * replacing Auth/FSTUser by C++ auth implementation (#804) * replacing Auth/FSTUser by C++ auth implementation * address changes * DispatchQueue delayed callback improvements + testing (#784) Basically a port of firebase/firebase-js-sdk@a1e346f and firebase/firebase-js-sdk@fce4168 * Introduces a DelayedCallback helper class in FSTDispatchQueue to encapsulate delayed callback logic. * Adds cancellation support. * Updates the idle timer in FSTStream to use new cancellation support. * Adds a FSTTimerId enum for identifying delayed operations on the queue and uses it to identify our existing backoff and idle timers. * Added containsDelayedCallback: and runDelayedCallbacksUntil: methods to FSTDispatchQueue which can be used from tests to check for the presence of a callback or to schedule them to run early. * Removes FSTTestDispatchQueue and changes idle tests to use new test methods. * Enable -Wcomma for our build; disable it for abseil. (#799) In order to use different cflags for abseil, this patch splits it out into a subspec within the pod. The cmake side of things "just works" since Firestore/CMakeLists.txt includes abseil before setting our compiler flags. * Serialize (and deserialize) bool values (#791) * Require official 1.4.0 for min CocoaPods version (#806) * Disable -Wrange-loop-analysis for abseil (#807) absl includes code like this: ``` void fn(std::initializer_list<absl::string_view> pieces) { ... for (const absl::string_view piece : pieces) total_size += piece.size(); ``` clang objects, suggesting that a reference should be used instead, i.e.: ``` for (const absl::string_view& piece : pieces) total_size += piece.size(); ``` But: a) we don't want to touch absl code b) string_views are cheap to copy (and absl recommends copying string_views rather than taking references as it may result in smaller code) c) some brief, naive benchmarking suggests there's no significant different in this case (i.e. (b) is correct.) Note that -Wrange-loop-analysis is already exlicitly enabled in our cmake build. * Delete stale Firestore instances after FIRApp is deleted. (#809) * replacing FSTGetTokenResult by C++ Token implementation (#805) * replacing Auth/FSTUser by C++ auth implementation * address changes * replacing FSTGetTokenResult by C++ Token implementation * address changes * address changes * fix another const& v.s. dispatch bug * fix more const& v.s. dispatch bug zxu123 committed * fix * passing by value in callback * Fix two stream close issues (b/73167987, b/73382103). (#810) * Fix b/73167987: Upon receiving a permanent write error when we had additional pendingWrites to send, we were restarting the stream with a new delegate and then immediately setting the delegate to nil, causing the stream to ignore all GRPC events for the stream. * Fix b/73382103: We were attempting to gracefully teardown the write stream (i.e. send an empty WriteRequest) even when the stream was already failed due to an error. This caused no harm other than log pollution, but I fixed it. * Use -[GRPCCall setResponseDispatchQueue] to dispatch GRPC callbacks directly onto the Firestore worker queue. This saves a double-dispatch and simplifies our logic. * Add stricter assertions regarding stream state now that dispatch queue / callback filter race conditions are eliminated. * [En|De]codeUnsignedVarint -> [En|De]codeVarint (#817) * [En|De]codeUnsignedVarint -> [En|De]codeVarint The 'unsigned' portion was misleading, as these varints work with both signed and unsigned integers. (The 'signed' varints also work with both signed and unsigned integers, but use zig-zag encoding so that negative numbers are encoded more efficiently. Note that 'signed' varints aren't used in the Value proto, so won't appear in the serializer class for at least the short term.) Added some docstrings to help disambiguate this. * Make FSTTimestamp into a public Firestore class (#698) - FSTTimestamp is now FIRTimestamp, under Firestore/Source/{Public,API}. This is a temporary solution; eventually, FIRTimestamp is supposed to live somewhere under Firebase; - move most internal Timestamp methods to the public header (the only exception is ISOString). * Fix implicit retain self warnings (#808) Xcode has starting warning about us implicitly retaining self references within blocks. This commit fixes it by explicitly mentioning self. No real changes are introduced here; this is effectively just making implicit behaviour explicit. * Accept FIRTimestamp where NSDate is currently accepted as a parameter (#823) * Fix trivial mem leak in the test suite (#828) Reduces noise while running valgrind so that I can see the leaks that I'm introducing. :/ * Serialize (and deserialize) int64 values (#818) (#829) * Avoid wrapping and rewrapping NSStrings when constructing DatabaseId (#833) * Avoid wrapping and rewrapping NSStrings when constructing DatabaseId * Shorten DatabaseId::kDefaultDatabaseId * Fix Firestore tests for M22 (#834) * Add FIRFirestoreTests to the Firestore Xcode project * Avoid waitForExpectations:timeout: This API was added in Xcode 8.3, but we still build production releases with Xcode 8.2. waitForExpectationsWithTimeout:handler: is available from Xcode 7.2. * Add AppForUnitTesting Add a utility for constructing a Firebase App for testing. * Handle the nil UID from FIRAuth * Avoid running CMake tests twice * Only build app_testing on Apple platforms * Revise test.sh messages * Avoid warnings about failing to override a designated initializer (#832) * address PR comments * Refactor [En|De]codeVarint to be symetric wrt tags (#837) Since we can't decode a value before knowing it's type, I've pulled the tag handling out of these methods. More context over here: #829 * Fix lint warnings (#840) * Don't build firebase_firestore_testutil_apple on non-apple platforms. (#841) Was failing the cmake build on linux. * Initial Carthage distribution instructions (#821) I couldn't find a way to get the Carthage installer to install Resources, so withdrawing Firestore and Invites. The other 11 components passed testing. * Update CHANGELOG for release (#845) * improve the documentations on auto init property * adjust the comments * Add build infrastructure for Codable support in Firestore (#815) * Add Firestore_SwiftTests_iOS target to Xcode * Add FirebaseFirestoreSwift podspec * Add Firestore_SwiftTests_iOS to the Podfile * Add CodableGeoPoint and tests * Version FirebaseFirestoreSwift separately * Move -messageWasLogged to FIRTestCase This makes it accessible to other test classes. * Add third-party library version registration This will allow us to collect the version of platform libraries that developers use in conjunction with Firebase. * Fixes clang warnings for Auth (#848) * Fixes clang warnings for Auth * Addresses comments * Revert "Move -messageWasLogged to FIRTestCase" This reverts commit dfda142. * Add tests for useragent Tests a variety of simple use cases. * Auto-style swift sources (#847) * Fix bash style issues * Exclude additional build output directories * Format swift files with scripts/style.sh * Reformat swift sources * Allow swiftformat 0.32.0 on travis * PR feedback * Add FirebaseCore version reporting I've also added clearing of the library names for tests to avoid the auto found versions on load. * Deflake tests * Updating RTDB Changelog for v4.1..5 * Update CHANGELOG.md * Style fixes * Change library name * Update CHANGELOG.md * Update CHANGELOG.md * Add XCode and Apple SDK versions * Update CHANGELOG for Firestore v0.10.2 (#856) * Manually fix clang-format issues * Remove unnecessary nonnull annotations * Update changelog for release (#857) * Updates changelog in preparation for release * Adds another change. * Update Core CHANGELOG for 4.0.16 (#858) * Convert cmake build to (mostly) use urls rather than git clones (#852) Exception: grpc. Due to it's use of git submodules, it's not completely trivial to convert it (though probably wouldn't be too much more work.) This helps address our build being throttled. (Maybe. This assumes github allows more fetching of tgz's than clones.) It should also speed up our build times. The downloaded tarballs are placed into ${PROJECT_BINARY_DIR}/downloads. This allows for eventual caching in travis. * Eliminate TypedValue and serialize direct from FieldValue to bytes. (#860) This will likely only apply for proto messages that use 'oneof's. (Because we need to serialize these manually.) The problem was that as we were adding additional types, TypeValue was evolving into a parallel implementation of the FieldValue union. When serializing/deserializing oneofs we need to supply a custom value to serialize and a custom function to do the work. There's no value in translating from FieldValue to TypeValue just to store as this custom object. We might as well just store the FieldValue model directly and write the custom serializer/deserializer to use the model directly. * replacing Auth by C++ auth implementation (#802) * lazy replacing FST(Firebase)CredentialsProvider by (Firebase)CredentialsProvider * lazy replacing FSTUser by User * adding error-code parameter to TokenListener * actually use const user& instead of pointer; also add an error util * add HashUser and pass into the unordered_map * use User in test * use c++ CredentialsProvider and subclass in test * fix unit test * use explicit capture in lambda instead of capture all by reference * cache currentUser explicitly when reset sync engineer test driver * objc object should be captured by value in lambda * replacing Auth/FSTUser by C++ auth implementation * address changes * replacing FSTGetTokenResult by C++ Token implementation * address changes * fix unintentional change in merging * patch the change in objc Auth up-stream * somehow, the lambda-version of set-user-change-listener does not work... fallback to block * address changes * fix another const& v.s. dispatch bug * fix more const& v.s. dispatch bug zxu123 committed * fix a bad sync line * address changes * address change * address change * fix upstream change from merge * fix upstream changes * Suggested fixes for cpp/port_auth (#846) * Get rid of MockDatastore factory This avoids the need to statically allocate (and leak) a credentials provider * Use absl::make_unique std::make_unique technically does not exist until C++14. * #include <utility> for std::move * Use std::future for the initial user * fix style * Add FieldValue.boolean_value() (#862) * style.sh (for swift) * remove forward declarations for classes that no longer exist
minafarid
pushed a commit
to minafarid/firebase-ios-sdk
that referenced
this pull request
Jun 6, 2018
…base#470) (firebase#714) [PORT OF firebase/firebase-js-sdk#470] Context: I made a previous change to raise isFromCache=true events when the client goes offline. As part of this change I added an assert for "OnlineState should not affect limbo documents", which it turns out was not valid because: * When we go offline, we set the view to non-current and call View.applyChanges(). * View.applyChanges() calls applyTargetChange() even though there's no target change and it recalculates limbo documents. * When the view is not current, we consider no documents to be in limbo. * Therefore all limbo documents are removed and so applyChanges() ends up returning unexpected LimboDocumentChanges. Fix: I've modified the View logic so that we don't recalculate limbo documents (and generate LimboDocumentChanges) when the View is not current, so now my assert holds and there should be less spurious removal / re-adding of limbo documents.
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Context: I made a previous change to raise isFromCache=true events when the client goes offline. As part of this change I added an assert for "OnlineState should not affect limbo documents", which it turns out was not valid because:
Fix: I've modified the View logic so that we don't recalculate limbo documents (and generate LimboDocumentChanges) when the View is not current, so now my assert holds and there should be less spurious removal / re-adding of limbo documents.