Crashlytics add new Record Exception Model API #5055
Merged
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
jasonhu-g
reviewed
Mar 12, 2020
jasonhu-g
reviewed
Mar 12, 2020
jasonhu-g
reviewed
Mar 12, 2020
Crashlytics/Crashlytics/Operations/Symbolication/FIRCLSThreadArrayOperation.m
Outdated
Show resolved
Hide resolved
jasonhu-g
reviewed
Mar 12, 2020
jasonhu-g
reviewed
Mar 12, 2020
jasonhu-g
reviewed
Mar 12, 2020
jasonhu-g
reviewed
Mar 12, 2020
jasonhu-g
reviewed
Mar 12, 2020
jasonhu-g
reviewed
Mar 12, 2020
jasonhu-g
reviewed
Mar 12, 2020
|
This isn't needed for merge the change, but may you also document new scenarios in the test plan and also update the test apps to expose this new API? |
jasonhu-g
suggested changes
Mar 12, 2020
samedson
force-pushed
the
crashlytics-exception-model
branch
from
a75005b to
5bb0350
Compare
jasonhu-g
reviewed
Mar 12, 2020
jasonhu-g
reviewed
Mar 12, 2020
jasonhu-g
reviewed
Mar 12, 2020
jasonhu-g
approved these changes
Mar 12, 2020
samedson
force-pushed
the
crashlytics-exception-model
branch
from
dfb061e to
fbe2e52
Compare
cbonnie
reviewed
Mar 18, 2020
| FIRCLSFileManager* fileManager) { | ||
| // Because we need to start the crash reporter right away, | ||
| // it starts up either with default settings, or cached settings | ||
| // from the last time they were fetched |
There was a problem hiding this comment.
Suggest replacing with: "Because we need to start the crash reporter right away, it starts up with either default settings or cached settings from the last time the crashes were fetched."
| // If this is set, then we could attempt to do a synchronous submission for certain kinds of | ||
| // events (exceptions). This is a very cool feature, but adds complexity to the backend. For now, | ||
| // we're going to leave this disabled. It does work in the exception case, but will ultimtely | ||
| // result in the following crash to be discared. Usually that crash isn't interesting. But, if it |
There was a problem hiding this comment.
Suggest replacing with:
"If initData.delegate is set, we can attempt a synchronous submission for certain kinds of events (exceptions). This is a useful feature but adds complexity to the backend. For now, this is disabled. It works in the exception case but ultimately results in discarding the following crash. Usually the crash doesn't provide interesting data, but if it does, you may want to see it before its discarded."
This avoids using "we" and "cool". Makes it clear what crash is going to be discarded. Removes past tense. Makes it clear what "we'd never have a chance to see it" means. Makes it clear what method we're talking about (initData.delegate).
| // If this is set, then we could attempt to do a synchronous submission for certain kinds of | ||
| // events (exceptions). This is a very cool feature, but adds complexity to the backend. For now, | ||
| // we're going to leave this disabled. It does work in the exception case, but will ultimtely | ||
| // result in the following crash to be discared. Usually that crash isn't interesting. But, if it |
There was a problem hiding this comment.
Do you mean "discarded" instead of "discared"?
| * Crashlytics will attempt symbolication and could overwrite other properities in the process. | ||
| * | ||
| * This class is used in conjunction with recordExceptionModel to record information about | ||
| * non-ObjC/C++ exceptions. All information included here will be displayed in the Crashlytics UI, |
| * | ||
| * This class is used in conjunction with recordExceptionModel to record information about | ||
| * non-ObjC/C++ exceptions. All information included here will be displayed in the Crashlytics UI, | ||
| * and can influence crash grouping. Be particularly careful with the use of the address property. |
There was a problem hiding this comment.
Starting from "All information included..." to "...other properties in the process", replace with:
"The recorded information is displayed in the Crashlytics dashboard (in the Firebase console) and can influence the grouping of crashes. Be careful when using the address property; if set, Crashlytics attempts symbolication and may overwrite other properties in the process."
This removes past tense and makes it clear what "Crashlytics UI" means.
|
|
||
| /** | ||
| * Initializes a symbolicated Stack Frame with the given required fields. Symbolicated | ||
| * Stack Frames will appear in the Crashlytics dashboard as reported in these fields. |
There was a problem hiding this comment.
"Symbolicated stack frames appear in the Crashlytics dashboard..."
Remove past tense
| /** | ||
| * Creates an Exception Model model with the given required fields. | ||
| * | ||
| * @param name - typically the type of the Exception class |
| * Stack Frames will appear in the Crashlytics dashboard as reported in these fields. | ||
| * | ||
| * @param symbol - The function or method name | ||
| * @param file - the file where the exception occurred |
There was a problem hiding this comment.
"@param file - The file where the exception occurred"
| * | ||
| * @param symbol - The function or method name | ||
| * @param file - the file where the exception occurred | ||
| * @param line - the line number |
There was a problem hiding this comment.
"The line number of the exception"
It's not clear what line number you're talking about
| - (instancetype)initWithSymbol:(NSString *)symbol file:(NSString *)file line:(NSInteger)line; | ||
|
|
||
| /** | ||
| * Creates a symbolicated Stack Frame with the given required fields. Symbolicated |
doudounan
pushed a commit
that referenced
this pull request
Mar 19, 2020
* Move UpdateFirebasePod to Swift's ArgumentParser library. (#4993) * Move UpdateFirebasePod to Swift's ArgumentParser library. This was introduced in URL, leaving a nice clean way to provide command line arguments. Example output: ``` $ swift run UpdateFirebasePod Error: Missing expected argument '--git-root <git-root>' Usage: firebase-pod-updater --git-root <git-root> --current-release <current-release> ``` * Removed unnecessary file. * Fixed formatting. * Use existing argument name for currentRelease. * Sorted package dependencies. * Pin to exact version. * Rename UpdateFirebasePod to firebase-pod-updater This sticks with convention expected by the tooling. Future PRs will change the `ReleasePackager` and other tools as well. * Renamed pod updater. (#5003) * Upload Symbols 3.1 (#4988) * Crashlytics Update CHANGELOG.md for v4.0.0-beta.5 (#5004) * Checks for connectivity when App is moving to foreground. (#4985) * Checks for connectivity when App is moving to foreground. * Split MaybeInvokeCallbacks and invoke them when foregrounding and network is available. * Update CHANGELOG for Firestore v1.11.1 (#5008) * Support AB testing of Firebase In-App Messages (#5005) * Parse out experiment payload for experimental FIAMs and store it on FIRIAMMessageDefinition * Fix bugs in parsing experimental payload * Add nullable experiment payload to FIAM message definition. Add method in FIRExperimenmtController to validate running experiments and call this when new messages are fetched from FIAM backend * Add and document new ABT methods, update FirebaseInAppMessaging.podspec to depend on ABT * Clean up deprecated public message initializers, add experiment payload to private initializers * Activate experiment for experimental messages upon impression * Clean up activateExperiment API * Add experimental payload to all message definition initializers * Add unit test coverage for new ABT methods * Add test coverage for parsing experimental messages * Verify experiment payload gets passed to message superclass * Scripts/style.sh * Update Podfile for UI test app to include AB Testing SDK * Add back deprecated initializers for now, these are needed for the test app * Test for non-nil experiment payload * Use mutable array directly, better memory usage * Bump ABTesting podspec version, depend on new ABT in FIAM * Remove mentions of FIREventOrigins.h * Remove experiment payload from public API, hide it in private * Scripts/style.sh * Fix comment indentation, allow for FIAM to depend on minor version updates of ABTesting * Move ABTExperimentPayload import to private header * Core M66 changelog update (#5013) * Core M66 changelog update * Core M65 notes corrected. * FirebaseStorage.podspec - update Core dependency version to one supporting watchOS (#5014) * Update versions for Release 6.19.0 * Add FirebaseABTesting to travis cron tests (#5015) * GoogleDataTransportCCTSupport - bump GoogleDataTransport dependency to ~> 5.0 * FirebaseCoreDiagnostics 1.2.2 - bump GoogleDataTransportCCTSupport dependency to ~> 2.0 (#5016) * M66 FirebaseCoreDiagnostics added to the release manifest (#5018) * M66 FIAM GoogleDataTransportCCTSupport dependency bump to '~> 2.0' (#5019) * update storage changelog (#5017) * Integrating GoogleDataTransport in Crashlytics to Report Crashes (#4989) * Remove setting CMAKE_BUILD_TYPE in CMakeLists.txt (#5027) This really needs to be set on the command-line if it's to be effective, and setting it here breaks downstream projects that include firebase-ios-sdk as a subdirectory. * Consolidate the Google Analytics origin for Crashlytics. Only use "clx". (#5030) * Fix assertions not printing correctlty (#5026) * M66 changelog for FIAM and ABT SDKs (#4920) * M65 changelog for ABT SDK * Improve CHANGELOG notes * Fix CHANGELOG version * M66 changelog for both FIAM & ABT * Fix formatting * Fix formatting pt2 * Put deprecated method in backticks * add Installations to docs script (#5035) * Update GDT and GDTCCT CHANGELOGs (#5034) * Update CHANGELOG.md * Update CHANGELOG.md * Update CHANGELOG for M66. (#5036) * Update CHANGELOG for M66. * Update CHANGELOG.md * Fix typos in doc comments (#5038) * Cherry Pick Crashlytics Upload Symbols (#5046) * Fix compilation under Linux/GCC 9.2.1 in Release mode (#5042) The `firebase_firestore_protos_libprotobuf` library contains protoc-generated code. Compile with standard flags rather than Firestore's default of `-Wall -Wextra -Werror`. * Fix nightly zip GHA build (#5041) * CocoaPods 1.9.1 (#5050) * Cherry Pick Crashlytics Upload Symbols (#5046) (#5047) Co-authored-by: Sam Edson <samedson@google.com> * MFA beta (#4823) * Fix a usage of `Mutation` at a point where it's only been forward-declared (#5037) * Address a few potential security issues (#5059) * Address a few potential security issues Eliminates use of memcpy and malloc * Update CHANGELOG * Merge release 6.19.0 (#5058) * Update versions for Release 6.19.0 * GoogleDataTransportCCTSupport - bump GoogleDataTransport dependency to ~> 5.0 * FirebaseCoreDiagnostics 1.2.2 - bump GoogleDataTransportCCTSupport dependency to ~> 2.0 (#5016) * M66 FirebaseCoreDiagnostics added to the release manifest (#5018) * M66 FIAM GoogleDataTransportCCTSupport dependency bump to '~> 2.0' (#5019) * Cherry Pick Crashlytics Upload Symbols (#5046) (#5047) Co-authored-by: Sam Edson <samedson@google.com> * M66: Crashlytics dependency versions update. Co-authored-by: Sam Edson <samedson@google.com> * Update environment checks (#5056) * update environment checks * replace deprecated macro * update GoogleUtilities changelog * Refactor and clean up the CMake build ahead of CMake/iOS support (#5051) * Use early exit in objc_framework CMakeLists.txt * Clean up objc_framework CMakeLists.txt * Use `version` consistently * Use `firebase_ios_version` for the repo-level version * Remove INCLUDE directories that are no longer needed * Sort dependencies * Simplify Example/App/CMakeLists.txt * Add `OBJC_FLAGS` directly, avoiding a separate `sources` variable. * Add resources directly * Rename test host app to firebase_firestore_example_app * Add a firebase_ios prefix to functions and variables This reduces the chance of collision with other projects. * Simplify compiler_setup Now that compiler_setup.cmake no longer has side-effects, fold compiler_id.cmake and archive_options.cmake into it and move the include of compiler_setup up to the top of the main build. * Capitalize (#5063) * Explicitly state Firestore's dependency on UIKit (#5067) In #4985 we added support for listening to `UIApplicationWillEnterForegroundNotification` notifications to better handle situations where we may have missed callbacks from `SCNetworkReachabilitySetCallback` while Firestore was in the background. Fixes #5065. * Explicitly state Firestore's dependency on UIKit (#5077) * Explicitly state Firestore's dependency on UIKit (#5067) In #4985 we added support for listening to `UIApplicationWillEnterForegroundNotification` notifications to better handle situations where we may have missed callbacks from `SCNetworkReachabilitySetCallback` while Firestore was in the background. Fixes #5065. * Update CHANGELOG for Firestore v1.11.2 * b/151167694 - sendUnsentReport might not upload until the second time the API is called (#5060) * Fix fuzz build (#5069) It was broken by our transition to avoid Abseil types in signatures. * Remove cleanup of experiments that are no longer running (#5078) * Remove cleanup of experiments that are no longer running (product decision) * Revert podspec bump * Bump Auth pod version (#5064) * Don't attempt to create data if the event's fileURL is nil. (#5088) * Don't attempt to create data if the event's fileURL is nil. This could be because of changes to NSCoding w.r.t. transitioning from GDTCORStoredEvent to GDTCOREvent. * Attempt to address sudden analyzer issues * Don't pass nil to a function that requires non-nil arguments. * Change error code to the new GDTCORMCEFileReadError * Update CHANGELOG.md (#5094) * Remove smart quotes (#5090) * Update M66 CHANGELOG (#5039) * Update M66 CHANGELOG * Update CHANGELOG * Add iOS build support to the CMake build (#5052) * Use early exit in objc_framework CMakeLists.txt * Clean up objc_framework CMakeLists.txt * Use `version` consistently * Use `firebase_ios_version` for the repo-level version * Remove INCLUDE directories that are no longer needed * Sort dependencies * Simplify Example/App/CMakeLists.txt * Add `OBJC_FLAGS` directly, avoiding a separate `sources` variable. * Add resources directly * Rename test host app to firebase_firestore_example_app * Add a firebase_ios prefix to functions and variables This reduces the chance of collision with other projects. * Simplify compiler_setup Now that compiler_setup.cmake no longer has side-effects, fold compiler_id.cmake and archive_options.cmake into it and move the include of compiler_setup up to the top of the main build. * Add support for building for iOS * Make building benchmarks optional * Disable protoc-based source generators when cross-compiling * Make downloading googletest optional * Add DISABLE_STRICT_WARNINGS mode to cc_rules This makes it possible to build non-Firestore components in the CMake build to a lower standard than that to which we hold ourselves without sacrificing the utility of the various build rules that we have. * Move to xcframework bundles (#4737) * Enable Mac Catalyst in support project * Update builder to output xcframework Replaces fat framwork and adds Mac Catalyst support Note: This removes i386 and armv7 support, as xcframework doesn’t support these * Add Modile and Info.plist fles * Generate real dynamic frameworks * Remove unnecessary file * Preserve symlinks for Mac SDK * Fix typo * Re-enable armv7 and i386 * Green CI for xcframework-master branch (#4778) * Static library framework build for xcframeworks zip (#4799) * Build xcframeworks for pods with resources (#4911) * Make Zip Builder temp dirs sortable and understandable with timestamp (#4919) * Zip Builder support for dependencies through subspecs (#4940) * Restore Firestore build (#4995) * Use static frameworks to build zip (#5029) * Add Swift module support to ZipBuilder (#5040) * Zip Carthage refactor (#5080) * Zip Resource handling for xcframeworks (#5093) * Add Crashlytics to if_changed for GoogleDataTransport changes (#5102) * FirebaseAuth should be in implicit pod list (#5104) * Use FIRInstallations SDK to fetch FID and FIS token, send those values in requests to FIAM backend (#5081) * Depend on FIRInstallations instead of IID, load an installations object into FIRInAppMessagingPrivate.h * Bootstrap FIRInAppMessaging.h with an instance of FIRInstallations * Update FIRIAMClientInfoFetcher to use FIRInstallations method to grab FID and FIS Token * Call new client info fetcher method * Fix up tests * Change nullability for FIRInstallations object, handle null case * More verbose comments in FIRIAMClientInfoFetcher, call completion in case that FIS token is fetched, but FID fails * Throw error and call completion if Firebase Installations object is not created * Send FIS token with method fetch requests * Add test class for FIRIAMClientInfoFetcher * Refactor FIRIAMClientInfoFetcher to get a FIRInstallations object injected * Add unit testing for fetchFirebaseInstallationDataWithProjectNumber: in FIRIAMClientInfoFetcher * Streamline unit tests, add case for nil FirebaseInstallations, clean up comments that were left in * Fix Firebase/Auth version (#5106) * Remote Config: enable ARC for tests. (#5108) * Remote Config: enable ARC for tests. * run ./scripts/style.sh * Fix warnings * Attempt to fix intermittent failing Crashlytics settings tests in CI (#5107) * GoogleUtilities 6.5.2 (#5110) * Crashlytics add new Record Exception Model API (#5055) * Installations: access FIRHeartbeatInfo from a background thread (#5114) * FIS: assert heartbeat storage is not accessed in main thread. * FIS: Access hearbeat storage from a background queue. * Make the prioritizer save state between app launches (#5120) * Revert "Address a few potential security issues (#5059)" (#5121) This reverts commit 86189f7. * The uploader should retry when there's an error (#5116) * The uploader should retry when there's an error * Log the decoding error if there's one * Migrate from using IID SDK for Remote Config to the new FIS SDK (#5096) * Update podspec to replace IID with installations SDK. * Update sample app podfile to include installations sdk. * Source code changes to support migration to FIS. The FIS SDK now supports multi-app FIS ids and tokens. * Update RC test app to use FIS SDK. * WIP: Unit tests for incorporating FIS with Remote Config. * Remote Config: fix Installations tests (#5109) * Fix Installations tests. * Fix Firebase/Auth version (#5106) * Remote Config: enable ARC for tests. * run ./scripts/style.sh * Fix warnings * Remote Config: enable ARC for tests. (#5108) * Remote Config: enable ARC for tests. * run ./scripts/style.sh * Fix warnings Co-authored-by: Paul Beusterien <paulbeusterien@google.com> * Add the installations token as an HTTP header. * Add changelog for FIS changes. Co-authored-by: Maksym Malyhin <mmaksym@google.com> Co-authored-by: Paul Beusterien <paulbeusterien@google.com> * Merge release 6.20.0 (#5118) * FIS changelog M67 (#5125) * Add Catalyst testing for GHA (#5115) * Crashlytics changelog v4.0.0-beta.6 (#5117) * Crashlytics upload-symbols 3.2 (#5129) * Enable lint for Auth (#5097) * In banner view, pin top elements to the bottom of the status bar (#5100) * Add constraint to pin title label below status bar in banner FIAM * Remove title label top pin from storyboard at build time * Use conditional compilation for iOS 13 check * Release notes for M67 (#5127) * Crashlytics cache key app version (#5132) * Pin Crashlytics to GoogleDataTransport 5.0.1 and GoogleDataTransportC… (#5133) * Fix deprecation warnings by consolidating coding calls (#5131) * Fix deprecation warnings by consolidating coding calls Fixes #5086 * Fix all NSSecureCoding references * Fix changed API usage * Update GDT versions, deps, and CHANGELOGs (#5138) * M67 Core Changelog (#5135) * Default to use new report upload endpoint when settings were failed to be fetched (#5128) * Fix missing path specs for pods in repo (#5140) * Refactor event fileURLs to be derived (#5137) * Refactor event fileURLs to be derived Apparently, since iOS 8, it's possible for the app's bundle to change directories as often as every launch. This is a mechanism that's hopefully slightly more robust against that, as data is supposed to carry over, the UUID component of the app's absolute directory might just change. * Convert a couple of tests, actually use a non-absolute URL in GDTCOREvent * Make sure to use the event fileURL property as the source of truth. Add some additional logs and improve the integration test * Fix KVC not working for the removed property * Dynamic Links: cleanup Apple framework dependencies (#5141) * Dynamic Links changelog M64 M67 (#5142) * Fix and enable flakey Crashlytics tests (#5145) * Fix changelog typo (#5146) * Libraries should save state if a completion block is requested (#5147) * Libraries should save state if a completion block is requested Currently, the onComplete block of sendXEvent:onComplete: is invoked when the event bytes are written to disk. However, if the app crashes before a lifecycle event occurs (trigger GDTCORStorage to serialize to disk), that event if effectively lost because the metadata has all been lost. This change triggers the saving of state of GDTCORStorage and prioritizers (if they implement it) if an onComplete block has been given. * Remove flaky check * Update GDTCORStorage.m * Make `api::CollectionReference::collection_id` return a const reference Primarily for consistency with `DocumentReference::document_id`. Note that `BasePath::last_segment` returns a const reference. Co-authored-by: Ryan Wilson <wilsonryan@google.com> Co-authored-by: Sam Edson <samedson93@gmail.com> Co-authored-by: wu-hui <53845758+wu-hui@users.noreply.github.com> Co-authored-by: Gil <mcg@google.com> Co-authored-by: christibbs <43829046+christibbs@users.noreply.github.com> Co-authored-by: Maksym Malyhin <mmaksym@google.com> Co-authored-by: Paul Beusterien <paulbeusterien@google.com> Co-authored-by: Chen Liang <chliang@google.com> Co-authored-by: Jason Hu <43789343+jasonhu-g@users.noreply.github.com> Co-authored-by: Michael Haney <mikehaney24@users.noreply.github.com> Co-authored-by: Morgan Chen <morganchen12@gmail.com> Co-authored-by: dmandar <dmandar@users.noreply.github.com> Co-authored-by: Sam Edson <samedson@google.com> Co-authored-by: Chuan Ren <renkelvin@gmail.com> Co-authored-by: Konstantin Varlamov <var-const@users.noreply.github.com> Co-authored-by: Peter Steinberger <steipete@gmail.com>
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.
This is a replacement for the
recordCustomExceptionName:reason:frameArray:API in the Fabric Crashlytics SDK