Bug 1261094 - Improve how MessageChannel::mInterruptStack is used in IPC code, r=jld a=lizzard
authorAndrea Marchesini <amarchesini@mozilla.com>
Thu, 28 Apr 2016 07:21:49 +0800
changeset 332783 afe313884a55cfa3f3029fc7cee3e0e5d6ff37f6
parent 332782 0e7f650568bbd910b20bdf7c8c2f1d193475cf2a
child 332784 ed90d8e648124ef21e0c616fa1c8cc379b91da90
push id6048
push userkmoir@mozilla.com
push dateMon, 06 Jun 2016 19:02:08 +0000
treeherdermozilla-beta@46d72a56c57d [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersjld, lizzard
bugs1261094
milestone48.0a2
Bug 1261094 - Improve how MessageChannel::mInterruptStack is used in IPC code, r=jld a=lizzard
dom/plugins/ipc/PluginMessageUtils.cpp
dom/plugins/ipc/PluginMessageUtils.h
dom/plugins/ipc/PluginModuleChild.h
dom/plugins/ipc/PluginModuleParent.h
ipc/chromium/src/chrome/common/ipc_message.h
ipc/glue/MessageChannel.cpp
ipc/glue/MessageChannel.h
ipc/glue/MessageLink.h
ipc/ipdl/test/cxx/TestInterruptRaces.cpp
ipc/ipdl/test/cxx/TestInterruptRaces.h
ipc/ipdl/test/cxx/TestRaceDeadlock.cpp
ipc/ipdl/test/cxx/TestRaceDeadlock.h
ipc/ipdl/test/cxx/TestRaceDeferral.cpp
ipc/ipdl/test/cxx/TestRaceDeferral.h
--- a/dom/plugins/ipc/PluginMessageUtils.cpp
+++ b/dom/plugins/ipc/PluginMessageUtils.cpp
@@ -60,18 +60,18 @@ NPRemoteWindow::NPRemoteWindow() :
 {
   clipRect.top = 0;
   clipRect.left = 0;
   clipRect.bottom = 0;
   clipRect.right = 0;
 }
 
 ipc::RacyInterruptPolicy
-MediateRace(const MessageChannel::Message& parent,
-            const MessageChannel::Message& child)
+MediateRace(const MessageChannel::MessageInfo& parent,
+            const MessageChannel::MessageInfo& child)
 {
   switch (parent.type()) {
   case PPluginInstance::Msg_Paint__ID:
   case PPluginInstance::Msg_NPP_SetWindow__ID:
   case PPluginInstance::Msg_NPP_HandleEvent_Shmem__ID:
   case PPluginInstance::Msg_NPP_HandleEvent_IOSurface__ID:
     // our code relies on the frame list not changing during paints and
     // reflows
--- a/dom/plugins/ipc/PluginMessageUtils.h
+++ b/dom/plugins/ipc/PluginMessageUtils.h
@@ -40,18 +40,18 @@ using layers::SurfaceDescriptorX11;
 
 enum ScriptableObjectType
 {
   LocalObject,
   Proxy
 };
 
 mozilla::ipc::RacyInterruptPolicy
-MediateRace(const mozilla::ipc::MessageChannel::Message& parent,
-            const mozilla::ipc::MessageChannel::Message& child);
+MediateRace(const mozilla::ipc::MessageChannel::MessageInfo& parent,
+            const mozilla::ipc::MessageChannel::MessageInfo& child);
 
 std::string
 MungePluginDsoPath(const std::string& path);
 std::string
 UnmungePluginDsoPath(const std::string& munged);
 
 extern mozilla::LogModule* GetPluginLog();
 
--- a/dom/plugins/ipc/PluginModuleChild.h
+++ b/dom/plugins/ipc/PluginModuleChild.h
@@ -60,17 +60,18 @@ static const int kNestedLoopDetectorInte
 
 class PluginInstanceChild;
 
 class PluginModuleChild : public PPluginModuleChild
 {
     typedef mozilla::dom::PCrashReporterChild PCrashReporterChild;
 protected:
     virtual mozilla::ipc::RacyInterruptPolicy
-    MediateInterruptRace(const Message& parent, const Message& child) override
+    MediateInterruptRace(const MessageInfo& parent,
+                         const MessageInfo& child) override
     {
         return MediateRace(parent, child);
     }
 
     virtual bool ShouldContinueFromReplyTimeout() override;
 
     virtual bool RecvSettingChanged(const PluginSettings& aSettings) override;
 
--- a/dom/plugins/ipc/PluginModuleParent.h
+++ b/dom/plugins/ipc/PluginModuleParent.h
@@ -142,17 +142,18 @@ public:
     virtual void SetHasLocalInstance() override {
         mHadLocalInstance = true;
     }
 
     int GetQuirks() { return mQuirks; }
 
 protected:
     virtual mozilla::ipc::RacyInterruptPolicy
-    MediateInterruptRace(const Message& parent, const Message& child) override
+    MediateInterruptRace(const MessageInfo& parent,
+                         const MessageInfo& child) override
     {
         return MediateRace(parent, child);
     }
 
     virtual bool
     RecvBackUpXResources(const FileDescriptor& aXSocketFd) override;
 
     virtual bool AnswerProcessSomeEvents() override;
--- a/ipc/chromium/src/chrome/common/ipc_message.h
+++ b/ipc/chromium/src/chrome/common/ipc_message.h
@@ -353,16 +353,31 @@ class Message : public Pickle {
     return file_descriptor_set_.get();
   }
 #endif
 
   const char* name_;
 
 };
 
+class MessageInfo {
+public:
+    typedef uint32_t msgid_t;
+
+    explicit MessageInfo(const Message& aMsg)
+        : mSeqno(aMsg.seqno()), mType(aMsg.type()) {}
+
+    int32_t seqno() const { return mSeqno; }
+    msgid_t type() const { return mType; }
+
+private:
+    int32_t mSeqno;
+    msgid_t mType;
+};
+
 //------------------------------------------------------------------------------
 
 }  // namespace IPC
 
 enum SpecialRoutingIDs {
   // indicates that we don't have a routing ID yet.
   MSG_ROUTING_NONE = kint32min,
 
--- a/ipc/glue/MessageChannel.cpp
+++ b/ipc/glue/MessageChannel.cpp
@@ -1280,17 +1280,17 @@ MessageChannel::Call(Message* aMsg, Mess
                "cannot issue Interrupt call while blocked on sync request");
     IPC_ASSERT(!DispatchingSyncMessage(),
                "violation of sync handler invariant");
     IPC_ASSERT(msg->is_interrupt(), "can only Call() Interrupt messages here");
 
     msg->set_seqno(NextSeqno());
     msg->set_interrupt_remote_stack_depth_guess(mRemoteStackDepthGuess);
     msg->set_interrupt_local_stack_depth(1 + InterruptStackDepth());
-    mInterruptStack.push(*msg);
+    mInterruptStack.push(MessageInfo(*msg));
     mLink->SendMessage(msg.forget());
 
     while (true) {
         // if a handler invoked by *Dispatch*() spun a nested event
         // loop, and the connection was broken during that loop, we
         // might have already processed the OnError event. if so,
         // trying another loop iteration will be futile because
         // channel state will have been cleared
@@ -1365,17 +1365,17 @@ MessageChannel::Call(Message* aMsg, Mess
         // If the message is an Interrupt reply, either process it as a reply to our
         // call, or add it to the list of out-of-turn replies we've received.
         if (recvd.is_reply()) {
             IPC_ASSERT(!mInterruptStack.empty(), "invalid Interrupt stack");
 
             // If this is not a reply the call we've initiated, add it to our
             // out-of-turn replies and keep polling for events.
             {
-                const Message &outcall = mInterruptStack.top();
+                const MessageInfo &outcall = mInterruptStack.top();
 
                 // Note, In the parent, sequence numbers increase from 0, and
                 // in the child, they decrease from 0.
                 if ((mSide == ChildSide && recvd.seqno() > outcall.seqno()) ||
                     (mSide != ChildSide && recvd.seqno() < outcall.seqno()))
                 {
                     mOutOfTurnReplies[recvd.seqno()] = Move(recvd);
                     continue;
@@ -1667,19 +1667,21 @@ MessageChannel::DispatchInterruptMessage
     // Race detection: see the long comment near mRemoteStackDepthGuess in
     // MessageChannel.h. "Remote" stack depth means our side, and "local" means
     // the other side.
     if (aMsg.interrupt_remote_stack_depth_guess() != RemoteViewOfStackDepth(stackDepth)) {
         // Interrupt in-calls have raced. The winner, if there is one, gets to defer
         // processing of the other side's in-call.
         bool defer;
         const char* winner;
-        const Message& parentMsg = (mSide == ChildSide) ? aMsg : mInterruptStack.top();
-        const Message& childMsg = (mSide == ChildSide) ? mInterruptStack.top() : aMsg;
-        switch (mListener->MediateInterruptRace(parentMsg, childMsg))
+        const MessageInfo parentMsgInfo =
+          (mSide == ChildSide) ? MessageInfo(aMsg) : mInterruptStack.top();
+        const MessageInfo childMsgInfo =
+          (mSide == ChildSide) ? mInterruptStack.top() : MessageInfo(aMsg);
+        switch (mListener->MediateInterruptRace(parentMsgInfo, childMsgInfo))
         {
           case RIPChildWins:
             winner = "child";
             defer = (mSide == ChildSide);
             break;
           case RIPParentWins:
             winner = "parent";
             defer = (mSide != ChildSide);
--- a/ipc/glue/MessageChannel.h
+++ b/ipc/glue/MessageChannel.h
@@ -69,16 +69,17 @@ class MessageChannel : HasResultCodes
     class InterruptFrame;
 
     typedef mozilla::Monitor Monitor;
 
   public:
     static const int32_t kNoTimeout;
 
     typedef IPC::Message Message;
+    typedef IPC::MessageInfo MessageInfo;
     typedef mozilla::ipc::Transport Transport;
 
     explicit MessageChannel(MessageListener *aListener);
     ~MessageChannel();
 
     // "Open" from the perspective of the transport layer; the underlying
     // socketpair/pipe should already be created.
     //
@@ -634,17 +635,17 @@ class MessageChannel : HasResultCodes
     // another blocking message, because it's blocked on a reply from us.
     //
     MessageQueue mPending;
 
     // Stack of all the out-calls on which this channel is awaiting responses.
     // Each stack refers to a different protocol and the stacks are mutually
     // exclusive: multiple outcalls of the same kind cannot be initiated while
     // another is active.
-    std::stack<Message> mInterruptStack;
+    std::stack<MessageInfo> mInterruptStack;
 
     // This is what we think the Interrupt stack depth is on the "other side" of this
     // Interrupt channel.  We maintain this variable so that we can detect racy Interrupt
     // calls.  With each Interrupt out-call sent, we send along what *we* think the
     // stack depth of the remote side is *before* it will receive the Interrupt call.
     //
     // After sending the out-call, our stack depth is "incremented" by pushing
     // that pending message onto mPending.
--- a/ipc/glue/MessageLink.h
+++ b/ipc/glue/MessageLink.h
@@ -57,16 +57,17 @@ enum RacyInterruptPolicy {
 
 class MessageListener
   : protected HasResultCodes,
     public mozilla::SupportsWeakPtr<MessageListener>
 {
   public:
     MOZ_DECLARE_WEAKREFERENCE_TYPENAME(MessageListener)
     typedef IPC::Message Message;
+    typedef IPC::MessageInfo MessageInfo;
 
     virtual ~MessageListener() { }
 
     virtual void OnChannelClose() = 0;
     virtual void OnChannelError() = 0;
     virtual Result OnMessageReceived(const Message& aMessage) = 0;
     virtual Result OnMessageReceived(const Message& aMessage, Message *& aReply) = 0;
     virtual Result OnCallReceived(const Message& aMessage, Message *& aReply) = 0;
@@ -115,18 +116,18 @@ class MessageListener
         NS_RUNTIMEABORT("default impl shouldn't be invoked");
     }
     virtual void OnEnteredCall() {
         NS_RUNTIMEABORT("default impl shouldn't be invoked");
     }
     virtual void OnExitedCall() {
         NS_RUNTIMEABORT("default impl shouldn't be invoked");
     }
-    virtual RacyInterruptPolicy MediateInterruptRace(const Message& parent,
-                                                     const Message& child)
+    virtual RacyInterruptPolicy MediateInterruptRace(const MessageInfo& parent,
+                                                     const MessageInfo& child)
     {
         return RIPChildWins;
     }
 
     /**
      * Return true if windows messages can be handled while waiting for a reply
      * to a sync IPDL message.
      */
--- a/ipc/ipdl/test/cxx/TestInterruptRaces.cpp
+++ b/ipc/ipdl/test/cxx/TestInterruptRaces.cpp
@@ -11,21 +11,21 @@ struct RunnableMethodTraits<mozilla::_ip
     static void ReleaseCallee(mozilla::_ipdltest::TestInterruptRacesParent* obj) { }
 };
 
 
 namespace mozilla {
 namespace _ipdltest {
 
 ipc::RacyInterruptPolicy
-MediateRace(const MessageChannel::Message& parent,
-            const MessageChannel::Message& child)
+MediateRace(const MessageChannel::MessageInfo& parent,
+            const MessageChannel::MessageInfo& child)
 {
     return (PTestInterruptRaces::Msg_Child__ID == parent.type()) ?
-        ipc::RIPParentWins : ipc::RIPChildWins;
+                ipc::RIPParentWins : ipc::RIPChildWins;
 }
 
 //-----------------------------------------------------------------------------
 // parent
 void
 TestInterruptRacesParent::Main()
 {
     if (!SendStart())
--- a/ipc/ipdl/test/cxx/TestInterruptRaces.h
+++ b/ipc/ipdl/test/cxx/TestInterruptRaces.h
@@ -5,18 +5,18 @@
 
 #include "mozilla/_ipdltest/PTestInterruptRacesParent.h"
 #include "mozilla/_ipdltest/PTestInterruptRacesChild.h"
 
 namespace mozilla {
 namespace _ipdltest {
 
 mozilla::ipc::RacyInterruptPolicy
-MediateRace(const mozilla::ipc::MessageChannel::Message& parent,
-            const mozilla::ipc::MessageChannel::Message& child);
+MediateRace(const mozilla::ipc::MessageChannel::MessageInfo& parent,
+            const mozilla::ipc::MessageChannel::MessageInfo& child);
 
 class TestInterruptRacesParent :
     public PTestInterruptRacesParent
 {
 public:
     TestInterruptRacesParent() : mHasReply(false),
                            mChildHasReply(false),
                            mAnsweredParent(false)
@@ -43,17 +43,18 @@ protected:
 
     virtual bool
     AnswerParent() override;
 
     virtual bool
     RecvGetAnsweredParent(bool* answeredParent) override;
 
     virtual mozilla::ipc::RacyInterruptPolicy
-    MediateInterruptRace(const Message& parent, const Message& child) override
+    MediateInterruptRace(const MessageInfo& parent,
+                         const MessageInfo& child) override
     {
         return MediateRace(parent, child);
     }
 
     virtual void ActorDestroy(ActorDestroyReason why) override
     {
         if (NormalShutdown != why)
             fail("unexpected destruction!");
@@ -100,17 +101,18 @@ protected:
 
     virtual bool
     RecvWakeup3() override;
 
     virtual bool
     AnswerChild() override;
 
     virtual mozilla::ipc::RacyInterruptPolicy
-    MediateInterruptRace(const Message& parent, const Message& child) override
+    MediateInterruptRace(const MessageInfo& parent,
+                         const MessageInfo& child) override
     {
         return MediateRace(parent, child);
     }
 
     virtual void ActorDestroy(ActorDestroyReason why) override
     {
         if (NormalShutdown != why)
             fail("unexpected destruction!");
--- a/ipc/ipdl/test/cxx/TestRaceDeadlock.cpp
+++ b/ipc/ipdl/test/cxx/TestRaceDeadlock.cpp
@@ -6,17 +6,17 @@
 
 using namespace mozilla::ipc;
 typedef mozilla::ipc::MessageChannel::Message Message;
 
 namespace mozilla {
 namespace _ipdltest {
 
 static RacyInterruptPolicy
-MediateRace(const Message& parent, const Message& child)
+MediateRace(const MessageInfo& parent, const MessageInfo& child)
 {
     return (PTestRaceDeadlock::Msg_Win__ID == parent.type()) ?
         RIPParentWins : RIPChildWins;
 }
 
 //-----------------------------------------------------------------------------
 // parent
 
@@ -62,18 +62,18 @@ TestRaceDeadlockParent::Test1()
 
 bool
 TestRaceDeadlockParent::AnswerLose()
 {
     return true;
 }
 
 RacyInterruptPolicy
-TestRaceDeadlockParent::MediateInterruptRace(const Message& parent,
-                                       const Message& child)
+TestRaceDeadlockParent::MediateInterruptRace(const MessageInfo& parent,
+                                             const MessageInfo& child)
 {
     return MediateRace(parent, child);
 }
 
 //-----------------------------------------------------------------------------
 // child
 
 TestRaceDeadlockChild::TestRaceDeadlockChild()
@@ -115,16 +115,16 @@ TestRaceDeadlockChild::AnswerWin()
 
 bool
 TestRaceDeadlockChild::AnswerRpc()
 {
     return true;
 }
 
 RacyInterruptPolicy
-TestRaceDeadlockChild::MediateInterruptRace(const Message& parent,
-                                      const Message& child)
+TestRaceDeadlockChild::MediateInterruptRace(const MessageInfo& parent,
+                                            const MessageInfo& child)
 {
     return MediateRace(parent, child);
 }
 
 } // namespace _ipdltest
 } // namespace mozilla
--- a/ipc/ipdl/test/cxx/TestRaceDeadlock.h
+++ b/ipc/ipdl/test/cxx/TestRaceDeadlock.h
@@ -25,17 +25,18 @@ protected:
     virtual bool ShouldContinueFromReplyTimeout() override;
 
     void Test1();
 
     virtual bool RecvStartRace() override;
     virtual bool AnswerLose() override;
 
     virtual mozilla::ipc::RacyInterruptPolicy
-    MediateInterruptRace(const Message& parent, const Message& child) override;
+    MediateInterruptRace(const MessageInfo& parent,
+                         const MessageInfo& child) override;
 
     virtual void ActorDestroy(ActorDestroyReason why) override
     {
         if (NormalShutdown != why)
             fail("unexpected destruction!");
         passed("ok");
         QuitParent();
     }
@@ -52,17 +53,18 @@ public:
 protected:
     virtual bool RecvStartRace() override;
 
     virtual bool AnswerWin() override;
 
     virtual bool AnswerRpc() override;
 
     virtual mozilla::ipc::RacyInterruptPolicy
-    MediateInterruptRace(const Message& parent, const Message& child) override;
+    MediateInterruptRace(const MessageInfo& parent,
+                         const MessageInfo& child) override;
 
     virtual void ActorDestroy(ActorDestroyReason why) override
     {
         if (NormalShutdown != why)
             fail("unexpected destruction!");
         QuitChild();
     }
 };
--- a/ipc/ipdl/test/cxx/TestRaceDeferral.cpp
+++ b/ipc/ipdl/test/cxx/TestRaceDeferral.cpp
@@ -4,17 +4,17 @@
 
 using namespace mozilla::ipc;
 typedef mozilla::ipc::MessageChannel::Message Message;
 
 namespace mozilla {
 namespace _ipdltest {
 
 static RacyInterruptPolicy
-MediateRace(const Message& parent, const Message& child)
+MediateRace(const MessageInfo& parent, const MessageInfo& child)
 {
     return (PTestRaceDeferral::Msg_Win__ID == parent.type()) ?
         RIPParentWins : RIPChildWins;
 }
 
 //-----------------------------------------------------------------------------
 // parent
 
@@ -62,18 +62,18 @@ TestRaceDeferralParent::AnswerLose()
 {
     if (mProcessedLose)
         fail("processed Lose twice");
     mProcessedLose = true;
     return true;
 }
 
 RacyInterruptPolicy
-TestRaceDeferralParent::MediateInterruptRace(const Message& parent,
-                                       const Message& child)
+TestRaceDeferralParent::MediateInterruptRace(const MessageInfo& parent,
+                                             const MessageInfo& child)
 {
     return MediateRace(parent, child);
 }
 
 //-----------------------------------------------------------------------------
 // child
 
 TestRaceDeferralChild::TestRaceDeferralChild()
@@ -102,16 +102,16 @@ TestRaceDeferralChild::AnswerWin()
 
 bool
 TestRaceDeferralChild::AnswerRpc()
 {
     return true;
 }
 
 RacyInterruptPolicy
-TestRaceDeferralChild::MediateInterruptRace(const Message& parent,
-                                      const Message& child)
+TestRaceDeferralChild::MediateInterruptRace(const MessageInfo& parent,
+                                            const MessageInfo& child)
 {
     return MediateRace(parent, child);
 }
 
 } // namespace _ipdltest
 } // namespace mozilla
--- a/ipc/ipdl/test/cxx/TestRaceDeferral.h
+++ b/ipc/ipdl/test/cxx/TestRaceDeferral.h
@@ -22,17 +22,18 @@ public:
     void Main();
 
 protected:
     void Test1();
 
     virtual bool AnswerLose() override;
 
     virtual mozilla::ipc::RacyInterruptPolicy
-    MediateInterruptRace(const Message& parent, const Message& child) override;
+    MediateInterruptRace(const MessageInfo& parent,
+                         const MessageInfo& child) override;
 
     virtual void ActorDestroy(ActorDestroyReason why) override
     {
         if (NormalShutdown != why)
             fail("unexpected destruction!");  
         passed("ok");
         QuitParent();
     }
@@ -51,17 +52,18 @@ public:
 protected:
     virtual bool RecvStartRace() override;
 
     virtual bool AnswerWin() override;
 
     virtual bool AnswerRpc() override;
 
     virtual mozilla::ipc::RacyInterruptPolicy
-    MediateInterruptRace(const Message& parent, const Message& child) override;
+    MediateInterruptRace(const MessageInfo& parent,
+                         const MessageInfo& child) override;
 
     virtual void ActorDestroy(ActorDestroyReason why) override
     {
         if (NormalShutdown != why)
             fail("unexpected destruction!");
         QuitChild();
     }
 };