bug 1483905 - ensure the WebAuthnManager stays alive while WebAuthnTransactionChild is using it r=qdot

mozkeeler · mozkeeler · commit 3d31cfbfef44 · 2018-09-07T09:17:19.000-07:00
Differential Revision: https://phabricator.services.mozilla.com/D5305 --HG-- extra : rebase_source : 1c05f0cd33954fe0127e295b4c76eed40f75e6ef
diff --git a/dom/webauthn/WebAuthnTransactionChild.cpp b/dom/webauthn/WebAuthnTransactionChild.cpp
@@ -28,7 +28,12 @@ WebAuthnTransactionChild::RecvConfirmRegister(const uint64_t& aTransactionId,
     return IPC_FAIL_NO_REASON(this);
   }
 
-  mManager->FinishMakeCredential(aTransactionId, aResult);
+  // We don't own the reference to mManager. We need to prevent its refcount
+  // going to 0 while we call anything that can reach the call to
+  // StopListeningForVisibilityEvents in WebAuthnManager::ClearTransaction
+  // (often via WebAuthnManager::RejectTransaction).
+  RefPtr<WebAuthnManagerBase> kungFuDeathGrip(mManager);
+  kungFuDeathGrip->FinishMakeCredential(aTransactionId, aResult);
   return IPC_OK();
 }
 
@@ -40,7 +45,12 @@ WebAuthnTransactionChild::RecvConfirmSign(const uint64_t& aTransactionId,
     return IPC_FAIL_NO_REASON(this);
   }
 
-  mManager->FinishGetAssertion(aTransactionId, aResult);
+  // We don't own the reference to mManager. We need to prevent its refcount
+  // going to 0 while we call anything that can reach the call to
+  // StopListeningForVisibilityEvents in WebAuthnManager::ClearTransaction
+  // (often via WebAuthnManager::RejectTransaction).
+  RefPtr<WebAuthnManagerBase> kungFuDeathGrip(mManager);
+  kungFuDeathGrip->FinishGetAssertion(aTransactionId, aResult);
   return IPC_OK();
 }
 
@@ -52,7 +62,12 @@ WebAuthnTransactionChild::RecvAbort(const uint64_t& aTransactionId,
     return IPC_FAIL_NO_REASON(this);
   }
 
-  mManager->RequestAborted(aTransactionId, aError);
+  // We don't own the reference to mManager. We need to prevent its refcount
+  // going to 0 while we call anything that can reach the call to
+  // StopListeningForVisibilityEvents in WebAuthnManager::ClearTransaction
+  // (often via WebAuthnManager::RejectTransaction).
+  RefPtr<WebAuthnManagerBase> kungFuDeathGrip(mManager);
+  kungFuDeathGrip->RequestAborted(aTransactionId, aError);
   return IPC_OK();
 }
 
diff --git a/dom/webauthn/tests/get_assertion_dead_object.html b/dom/webauthn/tests/get_assertion_dead_object.html
@@ -0,0 +1,21 @@
+<!DOCTYPE html>
+<html>
+<head>
+<meta charset=utf-8>
+</head>
+<body>
+<script type="text/javascript">
+  window.addEventListener('load', function() {
+    let o = [];
+    o[0] = window.navigator;
+    document.writeln('');
+    // Since the USB token is enabled by default, this will pop up a notification that the
+    // user can insert/interact with it. Since this is just a test, this won't happen. The
+    // request will eventually time out.
+    // Unfortunately the minimum timeout is 15 seconds.
+    o[0].credentials.get({ publicKey: { challenge: new Uint8Array(128), timeout: 15000 } });
+    o.forEach((n, i) => o[i] = null);
+  });
+</script>
+</body>
+</html>
diff --git a/dom/webauthn/tests/mochitest.ini b/dom/webauthn/tests/mochitest.ini
@@ -3,6 +3,7 @@ support-files =
   cbor.js
   u2futil.js
   pkijs/*
+  get_assertion_dead_object.html
 skip-if = !e10s
 scheme = https
 
@@ -14,6 +15,7 @@ scheme = https
 [test_webauthn_no_token.html]
 [test_webauthn_make_credential.html]
 [test_webauthn_get_assertion.html]
+[test_webauthn_get_assertion_dead_object.html]
 [test_webauthn_override_request.html]
 [test_webauthn_store_credential.html]
 [test_webauthn_sameorigin.html]
diff --git a/dom/webauthn/tests/test_webauthn_get_assertion_dead_object.html b/dom/webauthn/tests/test_webauthn_get_assertion_dead_object.html
@@ -0,0 +1,29 @@
+<!DOCTYPE html>
+<meta charset=utf-8>
+<head>
+  <title>Test for GetAssertion on dead object</title>
+  <script type="text/javascript" src="/tests/SimpleTest/SimpleTest.js"></script>
+  <script type="text/javascript" src="/tests/SimpleTest/AddTask.js"></script>
+  <link rel="stylesheet" type="text/css" href="/tests/SimpleTest/test.css" />
+</head>
+<body>
+
+  <h1>Test for GetAssertion on dead object</h1>
+  <a target="_blank" href="https://bugzilla.mozilla.org/show_bug.cgi?id=1483905">Mozilla Bug 1483905</a>
+
+  <script class="testbody" type="text/javascript">
+    "use strict";
+    SimpleTest.waitForExplicitFinish();
+    SimpleTest.requestFlakyTimeout(
+      "Due to the nature of this test, there's no way for the window we're opening to signal " +
+      "that it's done (the `document.writeln('')` is essential and basically clears any state " +
+      "we could use). So, we have to wait at least 15 seconds for the webauthn call to time out.");
+    let win = window.open("https://example.com/tests/dom/webauthn/tests/get_assertion_dead_object.html");
+    setTimeout(() => {
+      win.close();
+      SimpleTest.finish();
+    }, 20000);
+  </script>
+
+</body>
+</html>

Original file line number	Diff line number	Diff line change
`@@ -28,7 +28,12 @@ WebAuthnTransactionChild::RecvConfirmRegister(const uint64_t& aTransactionId,`
`28`	`28`	`return IPC_FAIL_NO_REASON(this);`
`29`	`29`	`}`
`30`	`30`
`31`		`- mManager->FinishMakeCredential(aTransactionId, aResult);`
	`31`	`+ // We don't own the reference to mManager. We need to prevent its refcount`
	`32`	`+ // going to 0 while we call anything that can reach the call to`
	`33`	`+ // StopListeningForVisibilityEvents in WebAuthnManager::ClearTransaction`
	`34`	`+ // (often via WebAuthnManager::RejectTransaction).`
	`35`	`+ RefPtr<WebAuthnManagerBase> kungFuDeathGrip(mManager);`
	`36`	`+ kungFuDeathGrip->FinishMakeCredential(aTransactionId, aResult);`
`32`	`37`	`return IPC_OK();`
`33`	`38`	`}`
`34`	`39`
`@@ -40,7 +45,12 @@ WebAuthnTransactionChild::RecvConfirmSign(const uint64_t& aTransactionId,`
`40`	`45`	`return IPC_FAIL_NO_REASON(this);`
`41`	`46`	`}`
`42`	`47`
`43`		`- mManager->FinishGetAssertion(aTransactionId, aResult);`
	`48`	`+ // We don't own the reference to mManager. We need to prevent its refcount`
	`49`	`+ // going to 0 while we call anything that can reach the call to`
	`50`	`+ // StopListeningForVisibilityEvents in WebAuthnManager::ClearTransaction`
	`51`	`+ // (often via WebAuthnManager::RejectTransaction).`
	`52`	`+ RefPtr<WebAuthnManagerBase> kungFuDeathGrip(mManager);`
	`53`	`+ kungFuDeathGrip->FinishGetAssertion(aTransactionId, aResult);`
`44`	`54`	`return IPC_OK();`
`45`	`55`	`}`
`46`	`56`
`@@ -52,7 +62,12 @@ WebAuthnTransactionChild::RecvAbort(const uint64_t& aTransactionId,`
`52`	`62`	`return IPC_FAIL_NO_REASON(this);`
`53`	`63`	`}`
`54`	`64`
`55`		`- mManager->RequestAborted(aTransactionId, aError);`
	`65`	`+ // We don't own the reference to mManager. We need to prevent its refcount`
	`66`	`+ // going to 0 while we call anything that can reach the call to`
	`67`	`+ // StopListeningForVisibilityEvents in WebAuthnManager::ClearTransaction`
	`68`	`+ // (often via WebAuthnManager::RejectTransaction).`
	`69`	`+ RefPtr<WebAuthnManagerBase> kungFuDeathGrip(mManager);`
	`70`	`+ kungFuDeathGrip->RequestAborted(aTransactionId, aError);`
`56`	`71`	`return IPC_OK();`
`57`	`72`	`}`
`58`	`73`