[WebOTP] Make service unconditionally unsubscribe from fetcher

It is important to unsubscribe WebOTPService from SMS fetcher when
it gets destructed to avoid use-after-free bugs.

This currently happens in CompleteRequest() but we only call it if there
is a callback. At the moment using the existence of a callback as a
proxy for the existence of a subscription is safe because we only
subscribe when there is a callback. But it is more robust and safer to
unsubscribe unconditionally and not rely on the existence of callback_
as an indicator for a subscription.

R=kenrb@chromium.org

BUG: 1138161
Change-Id: Ic69191fa7d895b211508b84f1b90c973f0c386a7
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2490260Reviewed-by: Majid Valipour <majidvp@chromium.org>
Reviewed-by: Ken Buchanan <kenrb@chromium.org>
Commit-Queue: Majid Valipour <majidvp@chromium.org>
Cr-Commit-Position: refs/heads/master@{#819520}

content/browser/sms/webotp_service.cc

@@ -69,8 +69,10 @@ WebOTPService::WebOTPService(
 }
 
 WebOTPService::~WebOTPService() {
-  if (callback_)
-    CompleteRequest(SmsStatus::kUnhandledRequest);
+  // Resolve any pending callback and invoke clean up to unsubscribe this
+  // service from fetcher.
+  CompleteRequest(SmsStatus::kUnhandledRequest);
+
   DCHECK(!callback_);
 }
 

content/browser/sms/webotp_service_unittest.cc

@@ -371,6 +371,10 @@ TEST_F(WebOTPServiceTest, CancelForNoDelegate) {
   loop.Run();
 
   ASSERT_FALSE(fetcher.HasSubscribers());
+
+  // Explicitly delete contents to ensure the invariant that the fetcher
+  // lifetime is longer than service.
+  DeleteContents();
 }
 
 TEST_F(WebOTPServiceTest, Abort) {