Add support to log JavaScript exceptions to Sentry

[fluiddot]

Related PRs:

• [RNMobile] Add error boundary components and exception logging WordPress/gutenberg#59221
• Add error boundary components and exception logging wordpress-mobile/gutenberg-mobile#6655
• [Gutenberg] Add error boundary components and exception logging wordpress-mobile/WordPress-iOS#22655

This PR expands the Crash logging service to support logging JavaScript exceptions to Sentry. This will allow hybrid sources like React Native (used in Gutenberg Mobile) to log exceptions with detailed information and symbolicated stack traces, which will greatly simplify the crash debugging process.

The approach is based on previous attempts to log JavaScript exceptions (p9ugOq-249-p2). However, in this case, we are following an SDK-less implementation where the exception is bubbled up to the host app and sent as a Sentry event using the Sentry iOS SDK. As can be seen in the implementation, the Sentry event is adjusted to ensure that Sentry processes it as a JavaScript exception and hence, symbolicate the stack trace with previously uploaded source maps.

The process to log a JavaScript exception has the following steps:

• Gutenberg:
  • Captures exceptions via error boundary components and a global error handler.
  • Parses the exception to convert the format of the stack trace from plain text to an array. (parser)
  • Sends the parsed exception to the host app (e.g. Jetpack app).
• Host app:
  • Logs the JavaScript exception to the Crash logging service.
• Crash logging service:
  • Creates a Sentry event with the JavaScript exception information, ensuring that it will be reported as a JavaScript exception in Sentry.

Regarding the source maps, they are uploaded as part of the build process of the host app (reference).

More information about this approach is detailed in p9ugOq-4p6-p2.

To test

Since it involves testing exceptions, we need to modify the code to force them and generate an installable build. A test PR has been created for this purpose, follow the testing instructions from wordpress-mobile/gutenberg-mobile#6654.

---

• I have considered if this change warrants release notes and have added them to the appropriate section in the CHANGELOG.md if necessary.

[crazytonyli]

The callback argument feels redundant to me. I have asked about removing it in WordPress/gutenberg#59221 (review).

[fluiddot]

I shared a comment about this here. In React Native, the communication between JavaScript and native is asynchronous so it's common to use callbacks.

[jhnstn]

I'm wondering about the callback too. It doesn't seem to add much value when passed this far down the call stack. As you suggest in this comment, it's only needed to inform about a malformed rawException. Even there I'm not sure how useful it will be.

[fluiddot]

@jhnstn As shared here, the main purpose of the callback is to ensure that an unhandled exception is sent to Sentry before letting the app crash. Hence, I'd advocate keeping it here.

[fluiddot]

@crazytonyli Regarding this comment, I think we need to have the callback here in Tracks because we need to guarantee that it's triggered after flushing all queued events:

Automattic-Tracks-iOS/Sources/Remote Logging/Crash Logging/CrashLogging.swift

Lines 159 to 162 in 07a7012

	DispatchQueue.global().async {
	SentrySDK.flush(timeout: self.flushTimeout)
	callback()
	}

This implementation is based on other functions we have to ensure that events are sent, e.g.:

Automattic-Tracks-iOS/Sources/Remote Logging/Crash Logging/CrashLogging.swift

Lines 247 to 274 in 07a7012

	private func logErrorsImmediately(
	_ errors: [Error],
	userInfo: [String: Any]? = nil,
	level: SentryLevel = .error,
	andWait wait: Bool,
	callback: @escaping () -> Void
	) {
	errors.forEach { error in
	// Amending the global scope on a per-event basis seems like the best way to add the
	// caller-provided `userInfo` and `level`.
	SentrySDK.capture(error: error) { scope in
	// Under the hood, `setExtras` uses `NSMutableDictionary` `addEntriesFromDictionary`
	// meaning this won't replace the whole extras dictionary.
	scope.setExtras(userInfo)
	scope.setLevel(level)
	}
	}
	let flushEventThenCallCallback: () -> Void = {
	SentrySDK.flush(timeout: self.flushTimeout)
	callback()
	}
	if wait {
	flushEventThenCallCallback()
	} else {
	DispatchQueue.global().async { flushEventThenCallCallback() }
	}
	}

[crazytonyli]

Nitpick: If handledBy is declared as String?, I think the isHandled property can be implied as var isHandled: Bool { handledBy != nil }?

[fluiddot]

Although both seem connected, I'm hesitant to infer that having handledBy defined implies that isHandled should be false. Let me explain a couple of cases to clarify it.

isHandled determines if the exception is handled by the editor, currently, this happens for exceptions captured by the error boundary components (reference). When an error is encountered, for instance, at block level we display an error message but let the user continue editing a post. In this case, handledBy will tell the mechanism used to handle it and isHandled will be true.

On the other hand, other exceptions can be captured via the global error handler (reference). These are exceptions produced outside the error boundaries. Here, we use handledBy to tell that the exception was captured by the global error handler but we mark them as unhandled (i.e. isHandled as false) because the editor will crash.

[crazytonyli]

Thanks for the explanation!

[crazytonyli]

LGTM!

I'm not familiar with this codebase and not sure how feasible/easy it is to add unit tests, but it'd be nice to add some unit tests to ensure the event payload scheme is correct, i.e. no threads and debug_meta.

[fluiddot]

I'm not familiar with this codebase and not sure how feasible/easy it is to add unit tests, but it'd be nice to add some unit tests to ensure the event payload scheme is correct, i.e. no threads and debug_meta.

Yep, I haven't added unit tests yet to this repository either. I briefly checked the unit tests of CrashLogging class and noticed that the ones related to error reporting are disabled (reference). I can take a look in a separate PR to incorporate tests for the logic around parsing a JavaScript exception.

Thank you so much @crazytonyli for your suggestions and for reviewing the PR 🙇 !

[fluiddot]

I noticed that the job buildkite/tracks-ios/validating-podspec is failing in trunk (reference) and other PRs (reference). In fact, I saw #274 (comment) on a past PR stating that should be safe to merge. Hence, I'm going to proceed with merging the PR.