Bug 1534421. Fix the CAN_RUN_SCRIPT analysis to treat a deref of an arg as live if it would treat the arg as live. r=andi
authorBoris Zbarsky <bzbarsky@mit.edu>
Wed, 13 Mar 2019 12:13:08 +0000
changeset 521707 f65aaa2a97bb
parent 521706 b6d239de820f
child 521708 a710a25b6c5d
push id10867
push userdvarga@mozilla.com
push dateThu, 14 Mar 2019 15:20:45 +0000
treeherdermozilla-beta@abad13547875 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersandi
bugs1534421
milestone67.0a1
first release with
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
last release without
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
Bug 1534421. Fix the CAN_RUN_SCRIPT analysis to treat a deref of an arg as live if it would treat the arg as live. r=andi Differential Revision: https://phabricator.services.mozilla.com/D23071
build/clang-plugin/CanRunScriptChecker.cpp
build/clang-plugin/CustomMatchers.h
build/clang-plugin/tests/TestCanRunScript.cpp
dom/events/EventListenerManager.cpp
--- a/build/clang-plugin/CanRunScriptChecker.cpp
+++ b/build/clang-plugin/CanRunScriptChecker.cpp
@@ -40,16 +40,36 @@ void CanRunScriptChecker::registerMatche
               ))
             )
           ),
           // and which is not a parameter of the parent function,
           unless(declRefExpr(to(parmVarDecl()))),
           // and which is not a default arg with value nullptr, since those are
           // always safe.
           unless(cxxDefaultArgExpr(isNullDefaultArg())),
+          // and which is not a dereference of a parameter of the parent
+          // function (including "this"),
+          unless(
+            unaryOperator(
+              unaryDereferenceOperator(),
+              hasUnaryOperand(
+                anyOf(
+                  // If we're doing *someArg, the argument of the dereference is
+                  // an ImplicitCastExpr LValueToRValue which has the
+                  // DeclRefExpr as an argument.  We could try to match that
+                  // explicitly with a custom matcher (none of the built-in
+                  // matchers seem to match on the thing being cast for an
+                  // implicitCastExpr), but it's simpler to just use
+                  // ignoreTrivials to strip off the cast.
+                  ignoreTrivials(declRefExpr(to(parmVarDecl()))),
+                  cxxThisExpr()
+                )
+              )
+            )
+          ),
           // and which is not a MOZ_KnownLive wrapped value.
           unless(
             anyOf(
               MozKnownLiveCall,
               // MOZ_KnownLive applied to a RefPtr or nsCOMPtr just returns that
               // same RefPtr/nsCOMPtr type which causes us to have a conversion
               // operator applied after the MOZ_KnownLive.
               cxxMemberCallExpr(on(allOf(hasType(isSmartPtrToRefCounted()),
--- a/build/clang-plugin/CustomMatchers.h
+++ b/build/clang-plugin/CustomMatchers.h
@@ -91,16 +91,22 @@ AST_MATCHER(BinaryOperator, binaryArithm
 /// This matcher will match all arithmetic unary operators.
 AST_MATCHER(UnaryOperator, unaryArithmeticOperator) {
   UnaryOperatorKind OpCode = Node.getOpcode();
   return OpCode == UO_PostInc || OpCode == UO_PostDec || OpCode == UO_PreInc ||
          OpCode == UO_PreDec || OpCode == UO_Plus || OpCode == UO_Minus ||
          OpCode == UO_Not;
 }
 
+/// This matcher will match the unary dereference operator
+AST_MATCHER(UnaryOperator, unaryDereferenceOperator) {
+  UnaryOperatorKind OpCode = Node.getOpcode();
+  return OpCode == UO_Deref;
+}
+
 /// This matcher will match == and != binary operators.
 AST_MATCHER(BinaryOperator, binaryEqualityOperator) {
   BinaryOperatorKind OpCode = Node.getOpcode();
   return OpCode == BO_EQ || OpCode == BO_NE;
 }
 
 /// This matcher will match comma operator.
 AST_MATCHER(BinaryOperator, binaryCommaOperator) {
--- a/build/clang-plugin/tests/TestCanRunScript.cpp
+++ b/build/clang-plugin/tests/TestCanRunScript.cpp
@@ -235,16 +235,29 @@ MOZ_CAN_RUN_SCRIPT void test_defaults_5(
   test_defaults_helper_2(t); // 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 test_defaults_6() {
   RefPtr<RefCountedBase> t = new RefCountedBase;
   test_defaults_helper_2(t);
 }
 
+MOZ_CAN_RUN_SCRIPT void test_arg_deref_helper(RefCountedBase&) {
+}
+
+MOZ_CAN_RUN_SCRIPT void test_arg_deref(RefCountedBase* arg) {
+  test_arg_deref_helper(*arg);
+}
+
+struct RefCountedDerefTester : public RefCountedBase {
+  MOZ_CAN_RUN_SCRIPT void foo() {
+    test_arg_deref_helper(*this);
+  }
+};
+
 struct DisallowMemberArgs {
   RefPtr<RefCountedBase> 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)}}
   }
--- a/dom/events/EventListenerManager.cpp
+++ b/dom/events/EventListenerManager.cpp
@@ -1035,17 +1035,18 @@ nsresult EventListenerManager::HandleEve
     // Event::currentTarget is set in EventDispatcher.
     if (listenerHolder.HasWebIDLCallback()) {
       ErrorResult rv;
       listenerHolder.GetWebIDLCallback()->HandleEvent(aCurrentTarget,
                                                       *aDOMEvent, rv);
       result = rv.StealNSResult();
     } else {
       // listenerHolder is holding a stack ref here.
-      result = MOZ_KnownLive(listenerHolder.GetXPCOMCallback())->HandleEvent(aDOMEvent);
+      result = MOZ_KnownLive(listenerHolder.GetXPCOMCallback())
+                   ->HandleEvent(aDOMEvent);
     }
   }
 
   return result;
 }
 
 EventMessage EventListenerManager::GetLegacyEventMessage(
     EventMessage aEventMessage) const {
@@ -1229,20 +1230,18 @@ void EventListenerManager::HandleEventIn
 
             nsCOMPtr<nsPIDOMWindowInner> innerWindow =
                 WindowFromListener(listener, aItemInShadowTree);
             mozilla::dom::Event* oldWindowEvent = nullptr;
             if (innerWindow) {
               oldWindowEvent = innerWindow->SetEvent(*aDOMEvent);
             }
 
-            // MOZ_KnownLive should not be needed here, but bug 1534421 requires
-            // it for the moment.
             nsresult rv =
-                HandleEventSubType(listener, MOZ_KnownLive(*aDOMEvent), aCurrentTarget);
+                HandleEventSubType(listener, *aDOMEvent, aCurrentTarget);
 
             if (innerWindow) {
               Unused << innerWindow->SetEvent(oldWindowEvent);
             }
 
             if (NS_FAILED(rv)) {
               aEvent->mFlags.mExceptionWasRaised = true;
             }