Backed out 2 changesets (bug 1744203) for causing bc test failures. CLOSED TREE

Backed out changeset 62c677d99313 (bug 1744203)
Backed out changeset 453b154f2cec (bug 1744203)

security/sandbox/common/test/PSandboxTesting.ipdl

@@ -8,7 +8,7 @@ namespace mozilla {
 
 async protocol PSandboxTesting {
 parent:
-  async ReportTestResults(nsCString testName, bool passed, nsCString message);
+  async ReportTestResults(nsCString testName, bool shouldSucceed, bool didSucceed, nsCString resultMessage);
   async TestCompleted();
 
 child:

security/sandbox/common/test/SandboxTestingChild.cpp

@@ -106,7 +106,8 @@ bool SandboxTestingChild::RecvShutDown() {
 }
 
 void SandboxTestingChild::ReportNoTests() {
-  SendReportTestResults("dummy_test"_ns, /* passed */ true,
+  SendReportTestResults("dummy_test"_ns, /* shouldSucceed */ true,
+                        /* didSucceed */ true,
                         "The test framework fails if there are no cases."_ns);
 }
 
@@ -120,48 +121,24 @@ void SandboxTestingChild::ErrnoTest(const nsCString& aName, bool aExpectSuccess,
 
 template <typename F>
 void SandboxTestingChild::ErrnoValueTest(const nsCString& aName,
-                                         int aExpectedErrno, F&& aFunction) {
+                                         bool aExpectEquals, int aExpectedErrno,
+                                         F&& aFunction) {
   int status = aFunction() >= 0 ? 0 : errno;
-  PosixTest(aName, aExpectedErrno == 0, status, Some(aExpectedErrno));
+  PosixTest(aName, aExpectEquals, status == aExpectedErrno);
 }
 
 void SandboxTestingChild::PosixTest(const nsCString& aName, bool aExpectSuccess,
-                                    int aStatus, Maybe<int> aExpectedError) {
+                                    int aStatus) {
+  bool succeeded = aStatus == 0;
   nsAutoCString message;
-  bool passed;
-
-  // The "expected" arguments are a little redundant.
-  MOZ_ASSERT(!aExpectedError || aExpectSuccess == (*aExpectedError == 0));
-
-  // Decide whether the test passed, and stringify the actual result.
-  if (aStatus == 0) {
+  if (succeeded) {
     message = "Succeeded"_ns;
-    passed = aExpectSuccess;
   } else {
     message = "Error: "_ns;
     message += strerror(aStatus);
-    if (aExpectedError) {
-      passed = aStatus == *aExpectedError;
-    } else {
-      passed = !aExpectSuccess;
-    }
   }
 
-  // If something unexpected happened, mention the expected result.
-  if (!passed) {
-    message += "; expected ";
-    if (aExpectSuccess) {
-      message += "success";
-    } else {
-      message += "error";
-      if (aExpectedError) {
-        message += ": ";
-        message += strerror(*aExpectedError);
-      }
-    }
-  }
-
-  SendReportTestResults(aName, passed, message);
+  SendReportTestResults(aName, aExpectSuccess, succeeded, message);
 }
 #endif  // XP_UNIX
 

security/sandbox/common/test/SandboxTestingChild.h

@@ -8,7 +8,6 @@
 #define mozilla_SandboxTestingChild_h
 
 #include "mozilla/PSandboxTestingChild.h"
-#include "mozilla/Maybe.h"
 #include "mozilla/Monitor.h"
 #include "mozilla/UniquePtr.h"
 
@@ -47,12 +46,8 @@ class SandboxTestingChild : public PSandboxTestingChild {
   inline void ReportNoTests();
 
 #ifdef XP_UNIX
-  // For test cases that return an error number or 0, like newer POSIX
-  // APIs.  If `aExpectSuccess` is true, the test passes if the status is
-  // 0; otherwise, the test requires a specific error if `aExpectedError`
-  // is `Some(n)` or any nonzero status if it's `Nothing()`.
-  void PosixTest(const nsCString& aName, bool aExpectSuccess, int aStatus,
-                 Maybe<int> aExpectedError = Nothing());
+  // For test cases that return an error number or 0, like newer POSIX APIs.
+  void PosixTest(const nsCString& aName, bool aExpectSuccess, int aStatus);
 
   // For test cases that return a negative number and set `errno` to
   // indicate error, like classical Unix APIs; takes a callable, which
@@ -61,10 +56,10 @@ class SandboxTestingChild : public PSandboxTestingChild {
   void ErrnoTest(const nsCString& aName, bool aExpectSuccess, F&& aFunction);
 
   // Similar to ErrnoTest, except that we want to compare a specific `errno`
-  // being returned.
+  // being returned (or not).
   template <typename F>
-  void ErrnoValueTest(const nsCString& aName, int aExpectedErrno,
-                      F&& aFunction);
+  void ErrnoValueTest(const nsCString& aName, bool aExpectEquals,
+                      int aExpectedErrno, F&& aFunction);
 #endif
 
  private:

security/sandbox/common/test/SandboxTestingChildTests.h

@@ -6,7 +6,6 @@
 
 #include "SandboxTestingChild.h"
 
-#include "mozilla/StaticPrefs_security.h"
 #include "nsXULAppAPI.h"
 
 #ifdef XP_UNIX
@@ -65,7 +64,7 @@ void RunTestsContent(SandboxTestingChild* child) {
   // work.
   // Checking ENETUNREACH should be thrown by SandboxBrokerClient::Connect()
   // when it detects it does not starts with a '/'
-  child->ErrnoValueTest("connect_abstract_blocked"_ns, ENETUNREACH, [&] {
+  child->ErrnoValueTest("connect_abstract_blocked"_ns, false, ENETUNREACH, [&] {
     int sockfd;
     struct sockaddr_un addr;
     char str[] = "\0xyz";  // Abstract socket requires first byte to be NULL
@@ -86,23 +85,15 @@ void RunTestsContent(SandboxTestingChild* child) {
   });
 
   // An abstract socket that does starts with /, so we do want it to work.
-  // Checking ECONNREFUSED because this is what the broker should get
-  // when trying to establish the connect call for us if it's allowed;
-  // otherwise we get EACCES, meaning that it was passed to the broker
-  // (unlike the previous test) but rejected.
-  const int errorForX =
-      StaticPrefs::security_sandbox_content_headless_AtStartup() ? EACCES
-                                                                 : ECONNREFUSED;
-  child->ErrnoValueTest("connect_abstract_permit"_ns, errorForX, [&] {
+  // Checking ECONNREFUSED because this is what the broker should get when
+  // trying to establish the connect call for us.
+  child->ErrnoValueTest("connect_abstract_permit"_ns, false, ECONNREFUSED, [&] {
     int sockfd;
     struct sockaddr_un addr;
     // we re-use actual X path, because this is what is allowed within
     // SandboxBrokerPolicyFactory::InitContentPolicy()
     // We can't just use any random path allowed, but one with CONNECT allowed.
 
-    // (Note that the real X11 sockets have names like `X0` for
-    // display `:0`; there shouldn't be anything named just `X`.)
-
     // Abstract socket requires first byte to be NULL
     char str[] = "\0/tmp/.X11-unix/X";
     size_t str_size = 17;
@@ -149,7 +140,7 @@ void RunTestsContent(SandboxTestingChild* child) {
   CFDictionaryRef windowServerDict = CGSessionCopyCurrentDictionary();
   bool gotWindowServerDetails = (windowServerDict != nullptr);
   child->SendReportTestResults(
-      "CGSessionCopyCurrentDictionary"_ns, !gotWindowServerDetails,
+      "CGSessionCopyCurrentDictionary"_ns, false, gotWindowServerDetails,
       gotWindowServerDetails ? "Failed: dictionary unexpectedly returned"_ns
                              : "Succeeded: no dictionary returned"_ns);
   if (windowServerDict != nullptr) {
@@ -218,7 +209,7 @@ void RunTestsRDD(SandboxTestingChild* child) {
 
 #ifdef XP_UNIX
 #  ifdef XP_LINUX
-  child->ErrnoValueTest("ioctl_tiocsti"_ns, ENOSYS, [&] {
+  child->ErrnoValueTest("ioctl_tiocsti"_ns, false, ENOSYS, [&] {
     int rv = ioctl(1, TIOCSTI, "x");
     return rv;
   });
@@ -229,12 +220,12 @@ void RunTestsRDD(SandboxTestingChild* child) {
     return rv;
   });
 
-  child->ErrnoValueTest("unlink"_ns, ENOENT, [&] {
+  child->ErrnoValueTest("unlink"_ns, false, ENOENT, [&] {
     int rv = unlink("");
     return rv;
   });
 
-  child->ErrnoValueTest("unlinkat"_ns, ENOENT, [&] {
+  child->ErrnoValueTest("unlinkat"_ns, false, ENOENT, [&] {
     int rv = unlinkat(AT_FDCWD, "", 0);
     return rv;
   });

security/sandbox/common/test/SandboxTestingParent.cpp

@@ -76,17 +76,21 @@ void SandboxTestingParent::ActorDestroy(ActorDestroyReason aWhy) {
 }
 
 mozilla::ipc::IPCResult SandboxTestingParent::RecvReportTestResults(
-    const nsCString& testName, bool passed, const nsCString& resultMessage) {
+    const nsCString& testName, bool shouldSucceed, bool didSucceed,
+    const nsCString& resultMessage) {
   NS_DispatchToMainThread(
       NS_NewRunnableFunction("SandboxReportTestResults", [=]() {
         nsCOMPtr<nsIObserverService> observerService =
             mozilla::services::GetObserverService();
         MOZ_RELEASE_ASSERT(observerService);
-        nsCString passedStr(passed ? "true"_ns : "false"_ns);
+        nsCString shouldPermit(shouldSucceed ? "true"_ns : "false"_ns);
+        nsCString wasPermitted(didSucceed ? "true"_ns : "false"_ns);
         nsString json;
         json += u"{ \"testid\" : \""_ns + NS_ConvertUTF8toUTF16(testName) +
-                u"\", \"passed\" : "_ns + NS_ConvertUTF8toUTF16(passedStr) +
-                u", \"message\" : \""_ns +
+                u"\", \"shouldPermit\" : "_ns +
+                NS_ConvertUTF8toUTF16(shouldPermit) +
+                u", \"wasPermitted\" : "_ns +
+                NS_ConvertUTF8toUTF16(wasPermitted) + u", \"message\" : \""_ns +
                 NS_ConvertUTF8toUTF16(resultMessage) + u"\" }"_ns;
         observerService->NotifyObservers(nullptr, "sandbox-test-result",
                                          json.BeginReading());

security/sandbox/common/test/SandboxTestingParent.h

@@ -28,7 +28,8 @@ class SandboxTestingParent : public PSandboxTestingParent {
   void ActorDestroy(ActorDestroyReason aWhy) override;
 
   mozilla::ipc::IPCResult RecvReportTestResults(const nsCString& testName,
-                                                bool passed,
+                                                bool shouldSucceed,
+                                                bool didSucceed,
                                                 const nsCString& resultMessage);
   mozilla::ipc::IPCResult RecvTestCompleted();
 

security/sandbox/test/browser_sandbox_test.js

@@ -22,10 +22,15 @@ function test() {
 
   // A callback called after each test-result.
   let sandboxTestResult = (subject, topic, data) => {
-    let { testid, passed, message } = JSON.parse(data);
+    let { testid, shouldPermit, wasPermitted, message } = JSON.parse(data);
     ok(
-      passed,
-      "Test " + testid + (passed ? " passed: " : " failed: ") + message
+      shouldPermit == wasPermitted,
+      "Test " +
+        testid +
+        " was " +
+        (wasPermitted ? "" : "not ") +
+        "permitted.  | " +
+        message
     );
   };
   Services.obs.addObserver(sandboxTestResult, "sandbox-test-result");