From c00170878902a892a5ff10bbf327b99a88021246 Mon Sep 17 00:00:00 2001 From: David Anderson Date: Mon, 26 Aug 2013 21:56:57 -0700 Subject: [PATCH] Don't deadlock on a child process for a CPOW (bug 905896, r=cjones). --- dom/ipc/ContentParent.cpp | 13 +++++++++ dom/ipc/ContentParent.h | 2 ++ ipc/glue/SyncChannel.cpp | 9 ++++++ ipc/ipdl/test/cxx/PTestDataStructures.ipdl | 2 ++ ipc/ipdl/test/cxx/PTestUrgency.ipdl | 2 ++ ipc/ipdl/test/cxx/TestUrgency.cpp | 33 ++++++++++++++++++++++ ipc/ipdl/test/cxx/TestUrgency.h | 11 ++++++-- js/ipc/JavaScriptParent.cpp | 2 +- 8 files changed, 71 insertions(+), 3 deletions(-) diff --git a/dom/ipc/ContentParent.cpp b/dom/ipc/ContentParent.cpp index 8d828b506814..a7103c9e4dab 100644 --- a/dom/ipc/ContentParent.cpp +++ b/dom/ipc/ContentParent.cpp @@ -939,6 +939,10 @@ ContentParent::OnChannelConnected(int32_t pid) } #endif } + + // Set a reply timeout. The only time the parent process will actually + // timeout is through urgent messages (which are used by CPOWs). + SetReplyTimeoutMs(Preferences::GetInt("dom.ipc.cpow.timeout", 3000)); } void @@ -2797,5 +2801,14 @@ ContentParent::RecvKeywordToURI(const nsCString& aKeyword, OptionalInputStreamPa return true; } +bool +ContentParent::ShouldContinueFromReplyTimeout() +{ + // The only time ContentParent sends blocking messages is for CPOWs, so + // timeouts should only ever occur in electrolysis-enabled sessions. + MOZ_ASSERT(Preferences::GetBool("browser.tabs.remote", false)); + return false; +} + } // namespace dom } // namespace mozilla diff --git a/dom/ipc/ContentParent.h b/dom/ipc/ContentParent.h index e87089fb0d19..2159b5d37dc5 100644 --- a/dom/ipc/ContentParent.h +++ b/dom/ipc/ContentParent.h @@ -180,6 +180,8 @@ protected: void OnChannelConnected(int32_t pid) MOZ_OVERRIDE; virtual void ActorDestroy(ActorDestroyReason why); + bool ShouldContinueFromReplyTimeout() MOZ_OVERRIDE; + private: static nsDataHashtable *sAppContentParents; static nsTArray* sNonAppContentParents; diff --git a/ipc/glue/SyncChannel.cpp b/ipc/glue/SyncChannel.cpp index 0bd91e90b441..a2c42dc7fac8 100644 --- a/ipc/glue/SyncChannel.cpp +++ b/ipc/glue/SyncChannel.cpp @@ -322,6 +322,15 @@ SyncChannel::ShouldContinueFromTimeout() cont = static_cast(mListener.get())->OnReplyTimeout(); } + static enum { UNKNOWN, NOT_DEBUGGING, DEBUGGING } sDebuggingChildren = UNKNOWN; + + if (sDebuggingChildren == UNKNOWN) { + sDebuggingChildren = getenv("MOZ_DEBUG_CHILD_PROCESS") ? DEBUGGING : NOT_DEBUGGING; + } + if (sDebuggingChildren == DEBUGGING) { + return true; + } + if (!cont) { // NB: there's a sublety here. If parents were allowed to // send sync messages to children, then it would be possible diff --git a/ipc/ipdl/test/cxx/PTestDataStructures.ipdl b/ipc/ipdl/test/cxx/PTestDataStructures.ipdl index ae8d7d913cd5..b0aacb99138e 100644 --- a/ipc/ipdl/test/cxx/PTestDataStructures.ipdl +++ b/ipc/ipdl/test/cxx/PTestDataStructures.ipdl @@ -1,6 +1,8 @@ include protocol PTestDataStructuresSub; include PTestDataStructuresCommon; +include "mozilla/GfxMessageUtils.h"; + namespace mozilla { namespace _ipdltest { diff --git a/ipc/ipdl/test/cxx/PTestUrgency.ipdl b/ipc/ipdl/test/cxx/PTestUrgency.ipdl index 989e59cdc97d..54021e1db341 100644 --- a/ipc/ipdl/test/cxx/PTestUrgency.ipdl +++ b/ipc/ipdl/test/cxx/PTestUrgency.ipdl @@ -9,12 +9,14 @@ parent: sync Test3() returns (uint32_t result); sync Test4_Begin(); sync Test4_NestedSync(); + sync FinalTest_Begin(); child: async Start(); urgent Reply1() returns (uint32_t result); urgent Reply2() returns (uint32_t result); urgent Test4_Reenter(); + urgent FinalTest_Hang(); }; } // namespace _ipdltest diff --git a/ipc/ipdl/test/cxx/TestUrgency.cpp b/ipc/ipdl/test/cxx/TestUrgency.cpp index bd1b2ce5a7ed..2ae7ba5de21b 100644 --- a/ipc/ipdl/test/cxx/TestUrgency.cpp +++ b/ipc/ipdl/test/cxx/TestUrgency.cpp @@ -3,6 +3,13 @@ #include "IPDLUnitTests.h" // fail etc. #include +template<> +struct RunnableMethodTraits +{ + static void RetainCallee(mozilla::_ipdltest::TestUrgencyParent* obj) { } + static void ReleaseCallee(mozilla::_ipdltest::TestUrgencyParent* obj) { } +}; + namespace mozilla { namespace _ipdltest { @@ -74,6 +81,21 @@ TestUrgencyParent::RecvTest4_NestedSync() return false; } +bool +TestUrgencyParent::RecvFinalTest_Begin() +{ + SetReplyTimeoutMs(2000); + if (CallFinalTest_Hang()) + fail("should have failed due to timeout"); + if (!GetIPCChannel()->Unsound_IsClosed()) + fail("channel should have closed"); + + MessageLoop::current()->PostTask( + FROM_HERE, + NewRunnableMethod(this, &TestUrgencyParent::Close)); + return false; +} + //----------------------------------------------------------------------------- // child @@ -115,6 +137,10 @@ TestUrgencyChild::RecvStart() if (!SendTest4_Begin()) fail("calling SendTest4_Begin"); + // This must be the last test, since the child process may die. + if (SendFinalTest_Begin()) + fail("Final test should not have succeeded"); + Close(); return true; @@ -153,6 +179,13 @@ TestUrgencyChild::AnswerTest4_Reenter() return true; } +bool +TestUrgencyChild::AnswerFinalTest_Hang() +{ + sleep(10); + return true; +} + TestUrgencyChild::TestUrgencyChild() : test_(0) { diff --git a/ipc/ipdl/test/cxx/TestUrgency.h b/ipc/ipdl/test/cxx/TestUrgency.h index 726c4f389b2a..5ed3adcaa073 100644 --- a/ipc/ipdl/test/cxx/TestUrgency.h +++ b/ipc/ipdl/test/cxx/TestUrgency.h @@ -27,14 +27,20 @@ public: bool RecvTest3(uint32_t *value); bool RecvTest4_Begin(); bool RecvTest4_NestedSync(); + bool RecvFinalTest_Begin(); + bool ShouldContinueFromReplyTimeout() MOZ_OVERRIDE + { + return false; + } virtual void ActorDestroy(ActorDestroyReason why) MOZ_OVERRIDE { - if (NormalShutdown != why) + if (AbnormalShutdown != why) fail("unexpected destruction!"); passed("ok"); QuitParent(); } + private: bool inreply_; }; @@ -51,10 +57,11 @@ public: bool AnswerReply1(uint32_t *reply); bool AnswerReply2(uint32_t *reply); bool AnswerTest4_Reenter(); + bool AnswerFinalTest_Hang(); virtual void ActorDestroy(ActorDestroyReason why) MOZ_OVERRIDE { - if (NormalShutdown != why) + if (AbnormalShutdown != why) fail("unexpected destruction!"); QuitChild(); } diff --git a/js/ipc/JavaScriptParent.cpp b/js/ipc/JavaScriptParent.cpp index 8db76a122705..bfb681798d00 100644 --- a/js/ipc/JavaScriptParent.cpp +++ b/js/ipc/JavaScriptParent.cpp @@ -594,7 +594,7 @@ JavaScriptParent::unwrap(JSContext *cx, ObjectId objId) bool JavaScriptParent::ipcfail(JSContext *cx) { - JS_ReportError(cx, "catastrophic IPC failure"); + JS_ReportError(cx, "child process crashed or timedout"); return false; }