rules_apple icon indicating copy to clipboard operation
rules_apple copied to clipboard

Bundle libMainThreadChecker.dylib to detect main thread violations

Open chiragramani opened this issue 2 years ago • 5 comments

Context

This PR introduces the integration of libMainThreadChecker.dylib to enhance quality checks by detecting main thread violations. By passing the feature=include_main_thread_checker flag, the library libMainThreadChecker.dylib will be embedded. Additionally, modifications to the test runner implementation ensure the DYLD_INSERT_LIBRARIES is updated to recognize this dylib. This aims to bolster both unit and UI test reliability.

https://developer.apple.com/documentation/xcode/diagnosing-memory-thread-and-crash-issues-early

For the following test-case,

func testTriggerMainThreadChecker() {
        let expectation = self.expectation(description: "Background operation")

        DispatchQueue.global().async {
            // This will trigger the Main Thread Checker because we are 
            // trying to update the UI from a background thread.
            let label = UILabel()
            label.text = "Test"

            expectation.fulfill()
        }

        waitForExpectations(timeout: 5) { (error) in
            if let error = error {
                XCTFail("waitForExpectations errored: \(error)")
            }
        }
    }

bazel build //examples/ios/HelloWorld --features=include_main_thread_checker

Screenshot 2023-10-04 at 12 36 40 AM

Discuss:

  1. The tests are successful but the test logs report the violation.
  2. Now, this is in line with what Xcode does. Even Xcode reports the violation but doesn't fail the test suite, which is reasonable. Still, from a quality assurance angle where we need structured data of where the violation happened, it might not be super convenient as parsing of the logs is needed.

TODO:

  1. Documentation

chiragramani avatar Oct 02 '23 22:10 chiragramani

@chiragramani Tested this today and its working nicely! One thing left would be failing (crashing) when the main thread checker is hit, I was able to get this by adding MTC_CRASH_ON_REPORT=1 to the TEST_ENV in the test runner template.

TEST_ENV="$TEST_ENV,DYLD_INSERT_LIBRARIES=$DYLD_INSERT_LIBRARIES_VALUE,MTC_CRASH_ON_REPORT=1"

We'd need to conditionalize it on that feature though

luispadron avatar Nov 03 '23 15:11 luispadron

should it always crash when this is enabled? otherwise it's just in the log which seems like it would more likely get lost in the noise?

keith avatar Nov 04 '23 20:11 keith

I think a use case for not crashing is potentially logging these issues somehow.

luispadron avatar Nov 04 '23 20:11 luispadron

@chiragramani Do you have time to finish this PR up? If not, I could look at finishing any remaining work. Thank you!

luispadron avatar Dec 04 '23 16:12 luispadron

@chiragramani Do you have time to finish this PR up? If not, I could look at finishing any remaining work. Thank you!

Hey, I'm back from medical leave and want to apologize for not addressing this PR sooner. I'll be catching up on the discussions now. Thanks for your understanding and patience.

One thing left would be failing (crashing) when the main thread checker is hit, I was able to get this by adding MTC_CRASH_ON_REPORT=1 to the TEST_ENV in the test runner template. We'd need to conditionalize it on that feature though

I added the following in the PR with a test-case, are you referring to something similar? --features=apple.fail_on_main_thread_checker.

I did not enable it by default to have parity with what Xcode does today.

chiragramani avatar Dec 11 '23 13:12 chiragramani

@chiragramani I'm going to merge this once CI passes on the latest master

luispadron avatar Mar 13 '24 19:03 luispadron

Tested end-to-end on our project and got the expected crash and checks when using the new features. Thanks @chiragramani!

luispadron avatar Mar 13 '24 19:03 luispadron