Bug 1534882 - Don't crash when synchronously constructing actor during content shutdown, r=nika draft
authorJan Varga <jan.varga@gmail.com>
Thu, 16 May 2019 21:09:18 +0200
changeset 2008610 3ce1b3eb8cdf0eec839042d648280dfcafaabaf1
parent 2008609 309c90d2fdeafa75e5e75a209f4a9b06e0bd7c16
child 2008611 b4cb17cc1280ed17b454d5bd10a756868b765fd9
push id363926
push userjvarga@mozilla.com
push dateSat, 18 May 2019 08:19:45 +0000
treeherdertry@e89baae86dd1 [default view] [failures only]
reviewersnika
bugs1534882, 1509362
milestone68.0a1
Bug 1534882 - Don't crash when synchronously constructing actor during content shutdown, r=nika This patch changes the way how we handle sync ctor send errors. They are now ignored and treated like messages which successfully were queued to send, but got lost due to the other side hanging up. For more details, see bug 1509362 which originally did it for async ctors. The main differences here are that we destroy the actor and we return null when the send fails. Differential Revision: https://phabricator.services.mozilla.com/D31517
dom/plugins/ipc/PluginModuleChild.cpp
ipc/ipdl/ipdl/lower.py
--- a/dom/plugins/ipc/PluginModuleChild.cpp
+++ b/dom/plugins/ipc/PluginModuleChild.cpp
@@ -973,18 +973,20 @@ NPError _geturlnotify(NPP aNPP, const ch
 
   if (!aNPP)  // nullptr check for nspluginwrapper (bug 561690)
     return NPERR_INVALID_INSTANCE_ERROR;
 
   nsCString url = NullableString(aRelativeURL);
   auto* sn = new StreamNotifyChild(url);
 
   NPError err;
-  InstCast(aNPP)->CallPStreamNotifyConstructor(sn, url, NullableString(aTarget),
-                                               false, nsCString(), false, &err);
+  if (!InstCast(aNPP)->CallPStreamNotifyConstructor(
+          sn, url, NullableString(aTarget), false, nsCString(), false, &err)) {
+    return NPERR_GENERIC_ERROR;
+  }
 
   if (NPERR_NO_ERROR == err) {
     // If NPN_PostURLNotify fails, the parent will immediately send us
     // a PStreamNotifyDestructor, which should not call NPP_URLNotify.
     sn->SetValid(aNotifyData);
   }
 
   return err;
@@ -1076,19 +1078,21 @@ NPError _posturlnotify(NPP aNPP, const c
         ("NPN_PostURLNotify with file=true is no longer supported"));
     return NPERR_GENERIC_ERROR;
   }
 
   nsCString url = NullableString(aRelativeURL);
   auto* sn = new StreamNotifyChild(url);
 
   NPError err;
-  InstCast(aNPP)->CallPStreamNotifyConstructor(
-      sn, url, NullableString(aTarget), true, nsCString(aBuffer, aLength),
-      aIsFile, &err);
+  if (!InstCast(aNPP)->CallPStreamNotifyConstructor(
+          sn, url, NullableString(aTarget), true, nsCString(aBuffer, aLength),
+          aIsFile, &err)) {
+    return NPERR_GENERIC_ERROR;
+  }
 
   if (NPERR_NO_ERROR == err) {
     // If NPN_PostURLNotify fails, the parent will immediately send us
     // a PStreamNotifyDestructor, which should not call NPP_URLNotify.
     sn->SetValid(aNotifyData);
   }
 
   return err;
--- a/ipc/ipdl/ipdl/lower.py
+++ b/ipc/ipdl/ipdl/lower.py
@@ -4128,30 +4128,48 @@ class _GenerateProtocolActorCode(ipdl.as
         actor = md.actorDecl()
         method = MethodDefn(self.makeSendMethodDecl(md))
         method.addstmts(self.bindManagedActor(actor) + [Whitespace.NL])
 
         msgvar, stmts = self.makeMessage(md, errfnSendCtor)
 
         replyvar = self.replyvar
         sendok, sendstmts = self.sendBlocking(md, msgvar, replyvar)
+
+        failIf = StmtIf(ExprNot(sendok))
+        failIf.addifstmt(_printWarningMessage('Error sending constructor'))
+        failIf.addifstmts(self.destroyActor(md, actor.var(),
+                                            why=_DestroyReason.FailedConstructor))
+        failIf.addifstmt(StmtReturn(ExprLiteral.NULL))
+
         method.addstmts(
+            # Build our constructor message & verify it.
             stmts
             + [Whitespace.NL,
                 StmtDecl(Decl(Type('Message'), replyvar.name))]
             + self.genVerifyMessage(md.decl.type.verify, md.params,
                                     errfnSendCtor, ExprVar('msg__'))
+
+            # Synchronously send the constructor message to the other side.
+            #
+            # If the MessageChannel is closing, and we haven't been told yet,
+            # this send may fail. This error is ignored to treat it like a
+            # message being lost due to the other side shutting down before
+            # processing it.
+            #
+            # NOTE: We also free the actor here.
             + sendstmts
-            + self.failCtorIf(md, ExprNot(sendok)))
-
-        def errfnCleanupCtor(msg):
-            return self.failCtorIf(md, ExprLiteral.TRUE)
+
+            # Warn, destroy the actor and return null if the message failed to
+            # send.
+            + [failIf])
+
         stmts = self.deserializeReply(
             md, ExprAddrOf(replyvar), self.side,
-            errfnCleanupCtor, errfnSentinel(ExprLiteral.NULL))
+            errfnSendCtor, errfnSentinel(ExprLiteral.NULL))
         method.addstmts(stmts + [StmtReturn(actor.var())])
 
         return method
 
     def bindManagedActor(self, actordecl, errfn=ExprLiteral.NULL, idexpr=None):
         actorvar = actordecl.var()
         actorproto = actordecl.ipdltype.protocol
         actortype = ipdl.type.ActorType(actorproto)
@@ -4172,30 +4190,16 @@ class _GenerateProtocolActorCode(ipdl.as
             # mManagedPFoo.PutEntry(actor);
             StmtExpr(_callInsertManagedActor(
                 self.protocol.managedVar(actorproto, self.side), actorvar)),
             # actor->mLivenessState = START;
             StmtExpr(ExprAssn(_actorState(actorvar),
                               _startState(actorproto.hasReentrantDelete))),
         ]
 
-    def failCtorIf(self, md, cond):
-        actorvar = md.actorDecl().var()
-        failif = StmtIf(cond)
-
-        if self.side == 'child':
-            # in the child process this should not fail
-            failif.addifstmt(_fatalError('constructor for actor failed'))
-        else:
-            failif.addifstmts(self.destroyActor(md, actorvar,
-                                                why=_DestroyReason.FailedConstructor))
-
-        failif.addifstmt(StmtReturn(ExprLiteral.NULL))
-        return [failif]
-
     def genHelperCtor(self, md):
         helperdecl = self.makeSendMethodDecl(md)
         helperdecl.params = helperdecl.params[1:]
         helper = MethodDefn(helperdecl)
 
         callctor = self.callAllocActor(md, retsems='out', side=self.side)
         helper.addstmt(StmtReturn(ExprCall(
             ExprVar(helperdecl.name), args=[callctor] + callctor.args)))
@@ -4938,17 +4942,18 @@ class _GenerateProtocolActorCode(ipdl.as
         else:
             assert not promise
             returnsems = 'out'
             rettype = Type.BOOL
         decl = MethodDecl(
             md.sendMethod(),
             params=md.makeCxxParams(paramsems, returnsems=returnsems,
                                     side=self.side, implicit=implicit),
-            warn_unused=(self.side == 'parent' and returnsems != 'callback'),
+            warn_unused=((self.side == 'parent' and returnsems != 'callback') or
+                         (md.decl.type.isCtor() and not md.decl.type.isAsync())),
             ret=rettype)
         if md.decl.type.isCtor():
             decl.ret = md.actorDecl().bareType(self.side)
         return decl
 
     def logMessage(self, md, msgptr, pfx, actor=None, receiving=False):
         actorname = _actorName(self.protocol.name, self.side)