Bug 1536336. Change MOZ_CAN_RUN_SCRIPT analysis to allow const members of "this" in addition to stack refptrs. r=andi

"this" is guaranteed to stay alive as long as other MOZ_CAN_RUN_SCRIPT
conditions hold, and its const members can't change value and drop
their refs.

Differential Revision: https://phabricator.services.mozilla.com/D23997

--HG--
extra : moz-landing-system : lando

build/clang-plugin/CanRunScriptChecker.cpp

@@ -34,12 +34,15 @@
  *    https://bugzilla.mozilla.org/show_bug.cgi?id=1535523 tracks this.
  *
  * Given those invariants we then require that when calling a MOZ_CAN_RUN_SCRIPT
- * function all refcounted arguments (including "this") satisfy one of three
+ * function all refcounted arguments (including "this") satisfy one of four
  * conditions:
  *  a) The argument is held via a strong pointer on the stack.
- *  b) The argument is an argument of the caller (and hence held by a strong
+ *  b) The argument is a const strong pointer member of "this".  We know "this"
+ *     is being kept alive, and a const strong pointer member can't drop its ref
+ *     until "this" dies.
+ *  c) The argument is an argument of the caller (and hence held by a strong
  *     pointer somewhere higher up the callstack).
- *  c) The argument is explicitly annotated with MOZ_KnownLive, which indicates
+ *  d) The argument is explicitly annotated with MOZ_KnownLive, which indicates
  *     that something is guaranteed to keep it alive (e.g. it's rooted via a JS
  *     reflector).
  */
@@ -53,6 +56,16 @@ void CanRunScriptChecker::registerMatchers(MatchFinder *AstMatcher) {
     ignoreTrivials(
       declRefExpr(to(varDecl(hasAutomaticStorageDuration())),
                   hasType(isSmartPtrToRefCounted())));
+  auto ConstMemberOfThisSmartPtr =
+    memberExpr(hasType(isSmartPtrToRefCounted()),
+               hasType(isConstQualified()),
+               hasObjectExpression(cxxThisExpr()));
+  // A smartptr can be known-live for two reasons:
+  // 1) It's declared on the stack.
+  // 2) It's a const member of "this".  We know "this" is alive (recursively)
+  //    and const members can't change their value hence can't drop their
+  //    reference until "this" gets destroyed.
+  auto KnownLiveSmartPtr = anyOf(StackSmartPtr, ConstMemberOfThisSmartPtr);
   auto MozKnownLiveCall =
     callExpr(callee(functionDecl(hasName("MOZ_KnownLive"))));
 
@@ -69,15 +82,15 @@ void CanRunScriptChecker::registerMatchers(MatchFinder *AstMatcher) {
           // and which is not this,
           unless(cxxThisExpr()),
           // and which is not a stack smart ptr
-          unless(StackSmartPtr),
+          unless(KnownLiveSmartPtr),
           // and which is not a method call on a stack smart ptr,
-          unless(cxxMemberCallExpr(on(StackSmartPtr))),
+          unless(cxxMemberCallExpr(on(KnownLiveSmartPtr))),
           // and which is not calling operator* on a stack smart ptr.
           unless(
             allOf(
               cxxOperatorCallExpr(hasOverloadedOperatorName("*")),
               callExpr(allOf(
-                hasAnyArgument(StackSmartPtr),
+                hasAnyArgument(KnownLiveSmartPtr),
                 argumentCountIs(1)
               ))
             )

build/clang-plugin/tests/TestCanRunScript.cpp

@@ -308,3 +308,42 @@ struct DisallowMemberCallsOnRandomKnownLive {
   }
 };
 
+struct AllowConstMemberArgs {
+  const RefPtr<RefCountedBase> mRefCounted;
+  MOZ_CAN_RUN_SCRIPT void foo() {
+    mRefCounted->method_test();
+  }
+  MOZ_CAN_RUN_SCRIPT void bar() {
+    test2(mRefCounted);
+  }
+};
+
+struct AllowConstMemberArgsWithExplicitThis {
+  const RefPtr<RefCountedBase> mRefCounted;
+  MOZ_CAN_RUN_SCRIPT void foo() {
+    this->mRefCounted->method_test();
+  }
+  MOZ_CAN_RUN_SCRIPT void bar() {
+    test2(this->mRefCounted);
+  }
+};
+
+struct DisallowConstMemberArgsOfMembers {
+  RefPtr<AllowConstMemberArgs> mMember;
+  MOZ_CAN_RUN_SCRIPT void foo() {
+    mMember->mRefCounted->method_test(); // expected-error {{arguments must all be strong refs or parent parameters when calling a function marked as MOZ_CAN_RUN_SCRIPT (including the implicit object argument)}}
+  }
+  MOZ_CAN_RUN_SCRIPT void bar() {
+    test2(mMember->mRefCounted); // expected-error {{arguments must all be strong refs or parent parameters when calling a function marked as MOZ_CAN_RUN_SCRIPT (including the implicit object argument)}}
+  }
+};
+
+struct DisallowConstNonRefPtrMemberArgs {
+  RefCountedBase* const mRefCounted;
+  MOZ_CAN_RUN_SCRIPT void foo() {
+    mRefCounted->method_test(); // expected-error {{arguments must all be strong refs or parent parameters when calling a function marked as MOZ_CAN_RUN_SCRIPT (including the implicit object argument)}}
+  }
+  MOZ_CAN_RUN_SCRIPT void bar() {
+    test2(mRefCounted); // expected-error {{arguments must all be strong refs or parent parameters when calling a function marked as MOZ_CAN_RUN_SCRIPT (including the implicit object argument)}}
+  }
+};

dom/events/DataTransferItem.cpp

@@ -436,8 +436,7 @@ void DataTransferItem::GetAsString(FunctionStringCallback* aCallback,
     MOZ_CAN_RUN_SCRIPT_BOUNDARY
     NS_IMETHOD Run() override {
       ErrorResult rv;
-      // mCallback is const, so we never null it out until our destructor.
-      MOZ_KnownLive(mCallback)->Call(mStringData, rv);
+      mCallback->Call(mStringData, rv);
       NS_WARNING_ASSERTION(!rv.Failed(), "callback failed");
       return rv.StealNSResult();
     }

dom/filesystem/compat/CallbackRunnables.cpp

@@ -33,9 +33,7 @@ EntryCallbackRunnable::EntryCallbackRunnable(FileSystemEntryCallback* aCallback,
 
 NS_IMETHODIMP
 EntryCallbackRunnable::Run() {
-  // Both of our strongly held members never change (they're const), so we can
-  // use MOZ_KnownLive on them.
-  MOZ_KnownLive(mCallback)->Call(MOZ_KnownLive(*mEntry));
+  mCallback->Call(*mEntry);
   return NS_OK;
 }
 
@@ -54,8 +52,7 @@ ErrorCallbackRunnable::ErrorCallbackRunnable(nsIGlobalObject* aGlobalObject,
 NS_IMETHODIMP
 ErrorCallbackRunnable::Run() {
   RefPtr<DOMException> exception = DOMException::Create(mError);
-  // mCallback never changes (it's const) so MOZ_KnownLive is ok.
-  MOZ_KnownLive(mCallback)->Call(*exception);
+  mCallback->Call(*exception);
   return NS_OK;
 }
 
@@ -68,8 +65,7 @@ EmptyEntriesCallbackRunnable::EmptyEntriesCallbackRunnable(
 NS_IMETHODIMP
 EmptyEntriesCallbackRunnable::Run() {
   Sequence<OwningNonNull<FileSystemEntry>> sequence;
-  // mCallback never changes (it's const), so MOZ_KnownLive is ok.
-  MOZ_KnownLive(mCallback)->Call(sequence);
+  mCallback->Call(sequence);
   return NS_OK;
 }
 
@@ -192,8 +188,7 @@ void GetEntryHelper::CompleteOperation(JSObject* aObj) {
 
     RefPtr<FileSystemFileEntry> entry = new FileSystemFileEntry(
         mParentEntry->GetParentObject(), file, mParentEntry, mFileSystem);
-    // mSuccessCallback never changes (it's const), so MOZ_KnownLive is ok.
-    MOZ_KnownLive(mSuccessCallback)->Call(*entry);
+    mSuccessCallback->Call(*entry);
     return;
   }
 
@@ -207,8 +202,7 @@ void GetEntryHelper::CompleteOperation(JSObject* aObj) {
 
   RefPtr<FileSystemDirectoryEntry> entry = new FileSystemDirectoryEntry(
       mParentEntry->GetParentObject(), directory, mParentEntry, mFileSystem);
-  // mSuccessCallback never changes (it's const), so MOZ_KnownLive is ok.
-  MOZ_KnownLive(mSuccessCallback)->Call(*entry);
+  mSuccessCallback->Call(*entry);
 }
 
 void GetEntryHelper::ContinueRunning(JSObject* aObj) {

dom/filesystem/compat/FileSystemDirectoryReader.cpp

@@ -87,8 +87,7 @@ class PromiseHandler final : public PromiseNativeHandler {
       sequence[i] = entry;
     }
 
-    // mSuccessCallback never changes (it's const), so MOZ_KnownLive is ok.
-    MOZ_KnownLive(mSuccessCallback)->Call(sequence);
+    mSuccessCallback->Call(sequence);
   }
 
   virtual void RejectedCallback(JSContext* aCx,

dom/filesystem/compat/FileSystemFileEntry.cpp

@@ -32,8 +32,7 @@ class FileCallbackRunnable final : public Runnable {
     RefPtr<File> file = File::Create(mFile->GetParentObject(), mFile->Impl());
     MOZ_ASSERT(file);
 
-    // mCallback never changes (it's const) so MOZ_KnownLive is ok.
-    MOZ_KnownLive(mCallback)->Call(*file);
+    mCallback->Call(*file);
     return NS_OK;
   }
 

dom/filesystem/compat/FileSystemRootDirectoryReader.cpp

@@ -34,8 +34,7 @@ class EntriesCallbackRunnable final : public Runnable {
       }
     }
 
-    // mCallback never changes (it's const), so MOZ_KnownLive is ok.
-    MOZ_KnownLive(mCallback)->Call(entries);
+    mCallback->Call(entries);
     return NS_OK;
   }
 

xpcom/base/CycleCollectedJSContext.cpp

@@ -233,8 +233,7 @@ class PromiseJobRunnable final : public MicroTaskRunnable {
       AutoHandlingUserInputStatePusher userInpStatePusher(
           mPropagateUserInputEventHandling, nullptr, doc);
 
-      // mCallback is const, so can't suddenly become null.
-      MOZ_KnownLive(mCallback)->Call("promise callback");
+      mCallback->Call("promise callback");
       aAso.CheckForInterrupt();
     }
     // Now that mCallback is no longer needed, clear any pointers it contains to