[payments] Upgrade delegate DCHECK to CHECK in SPC app factory

MERGED2026components/paymentssecure-payment-confirmation
2026. 8. 24.jmsmg 프로필 이미지jmsmg
https://crrev.com/c/8278112

Secure Payment Confirmation(SPC) 결제 앱 팩토리의 delegate 검사를 DCHECK에서 CHECK로 승격했습니다(crbug.com/443042812). 처음에는 "delegate가 파괴될 수 있으니 가드를 넣자"는 접근으로 CL을 올렸으나, 리뷰에서 그 전제 자체가 틀렸다는 것이 밝혀져 방향을 바꾼 기여입니다.

문제 설명

  • issue url: https://crbug.com/443042812
  • 이슈 제목은 "Guard or CHECK delegate in SecurePaymentConfirmationAppFactory::Create"이고, 실제 유저 크래시 리포트가 근거로 첨부되어 있었습니다.
  • SecurePaymentConfirmationAppFactory::Create()는 delegate(결제 세션)를 base::WeakPtr로 받아 DCHECK(delegate)로만 확인한 뒤 바로 다음 줄에서 역참조합니다.
  • DCHECK는 유저에게 배포되는 official 빌드에서 컴파일 시 제거됩니다. 따라서 릴리스에서는 이 검사가 아무 역할도 하지 못하고, 만약 delegate가 무효라면 다음 줄의 역참조에서 base::WeakPtr 내부의 CHECK(ref_.IsValid())가 터집니다. 그 경우 크래시 리포트는 weak_ptr.h를 가리켜 원인 추적이 어렵습니다.

첫 접근과 리뷰에서의 반전

처음에는 delegate가 실제로 파괴될 수 있다고 보고 early return 가드와 회귀 테스트를 넣은 CL(+23/−1)을 올렸습니다. payments OWNER인 Stephen McGruer가 Code-Review -1과 함께 두 가지를 지적했습니다.

  1. null이 될 수 없다. Create()를 호출하는 곳은 PaymentAppService::Create 하나뿐이고, 그것을 호출하는 곳은 데스크톱의 PaymentRequestState 생성자와 안드로이드의 PaymentAppServiceBridge::CreatePaymentApps 둘뿐입니다. 둘 다 살아있는 객체가 자기 자신의 weak_ptr_factory_.GetWeakPtr()을 동기 fan-out에 즉시 넘기는 구조라 중간에 무효화될 틈이 없습니다. git grep으로 호출처를 직접 확인해 그의 분석이 맞다는 것을 검증했습니다.
  2. 버그 리포트 자체가 부정확하다. 첨부된 크래시 스택을 보면 SPCAppFactory::Create 바로 아래에 SystemNetworkContextManager::UpdateTrustAnchorIDs 같은 무관한 프레임이 붙어 있습니다. 스택이 깨져 크래시 시그니처가 엉뚱한 함수에 잘못 귀속(misattribution)된 것으로, 실제로 이 함수에서 죽은 것이 아닙니다.

즉 제가 단 주석("delegate가 파괴될 수 있다")은 다음에 코드를 읽는 사람에게 사실이 아닌 정보를 남기는 것이었고, 조용한 early return은 만약 진짜 문제가 생기더라도 크래시도 로그도 없이 그것을 숨겨버리는 코드였습니다.

해결 내용

리뷰어 제안대로 가드 대신 CHECK 승격으로 방향을 바꿨습니다. 최종 diff는 1개 파일 1줄입니다.

 void SecurePaymentConfirmationAppFactory::Create(
     base::WeakPtr<Delegate> delegate) {
-  DCHECK(delegate);
+  CHECK(delegate);
  • 발생할 수 없는 상황을 검증하던 회귀 테스트는 함께 삭제했습니다. CHECK 승격 이후에는 그 테스트가 의도적으로 크래시하게 되며, 애초에 도달 불가능한 상태를 검증하는 테스트는 존재 이유가 없습니다.
  • 커밋 메시지도 "가드를 넣는다"에서 "이 불변식을 릴리스 빌드에서도 강제한다"로 다시 썼고, 리뷰어의 호출처 분석을 근거로 포함했습니다.

CHECK가 옳은 이유는 Chromium 스타일가이드(styleguide/c++/checks.md)에 거의 같은 예제로 나와 있습니다.

// Good:
// Testing pointer equality is very cheap so write this as a CHECK. A security
// bug would happen afterward if the CHECK fails (in this case, on the next line).

"검사가 싸고, 실패하면 바로 다음 줄에서 문제가 터지는 경우"가 정확히 이 코드의 모양입니다. 같은 문서는 "DCHECK는 프로덕션에서 아무것도 검증하지 못하므로 불변식이 깨진 채 프로그램이 계속 진행되는 것을 막지 못한다"고도 명시합니다.

테스트 방법

  1. autoninja -C out/Default components_unittests 후 --gtest_filter='SecurePaymentConfirmation*'로 회귀가 없음을 확인했습니다.
  2. 헤드리스 SSH 서버라 --ozone-platform=headless 플래그가 필요합니다. 없으면 TestWebContents를 만드는 테스트들이 DeviceDataManager was not created로 전부 크래시합니다.
  3. 테스트를 삭제하면 그 테스트에서만 쓰던 헬퍼가 미사용으로 남아 -Werror에 걸릴 수 있습니다. 수정한 오브젝트 2개만 먼저 컴파일해 이를 확인하고, 심볼 테이블에서 삭제한 테스트가 사라지고 나머지 70개가 남아 있음을 검증했습니다.
  4. 전 플랫폼 검증은 CQ dry run으로 진행했습니다. 멘티 계정에는 tryjob 권한이 없어 멘토님께서 실행해 주셨고 통과했습니다.

배운 점

  • 크래시 리포트를 근거로 삼을 때는 스택 전체의 정합성을 먼저 의심해야 합니다. 무관한 프레임이 섞여 있으면 시그니처가 잘못 귀속된 것일 수 있고, 그 위에 쌓은 가설은 통째로 틀리게 됩니다.
  • "~일 수 있다"는 주석은 호출처를 전부 추적한 뒤에만 써야 합니다. 추측을 코드 주석으로 남기면 다음 사람을 오도합니다. 리뷰어가 -1을 준 실질적인 이유도 코드보다는 이 주석이었습니다.
  • 조용한 early return은 불변식 위반을 숨깁니다. 발생하면 안 되는 상황이라면 CHECK로 드러내는 편이 낫습니다. 특히 이번처럼 크래시 시그니처가 잘못 잡혀 혼란이 생겼던 코드에서는, CHECK가 남기는 깨끗한 스택이 다음 논쟁을 막아줍니다.
  • DCHECK가 실제로 꺼지는 것은 official 빌드뿐입니다. 로컬 out/Default는 is_debug=false여도 dcheck_always_on이 비-official 크로미움 빌드에서 기본 true라 DCHECK가 살아 있습니다.
  • 리뷰에서 방향이 바뀌는 것은 실패가 아니라 정상적인 과정입니다. 길게 사과하기보다 지적이 맞는지 코드로 검증하고, 반영한 뒤 다음으로 넘어가는 것이 낫다는 것을 배웠습니다.

참고 자료

모든 컨트리뷰션 보기