The typical use of CPOWs is pretty safe: 1. Content process gets an event and sends a sync message to chrome. 2. Chrome process sends down a bunch of CPOW messages querying the DOM. 3. Chrome process finishes handling the sync message and content process is allowed to continue. We are much less likely to deadlock in this situation because the content process is known to be ready to handle these CPOWs. Let's call this case 1. Deadlocks are much more likely to happen if the content process is unprepared to receive a CPOW. This is case 2. Most uses of CPOWs by plugins and tests fall into case 1. Most uses of CPOWs by Firefox fall into case 2. We should warn when case 2 happens and try to eliminate these issues. The main ones that I know of are: - Session store data collection uses case 2 CPOWs when a tab or window is closed. - Any use of the sessionHistory object (via the back button or isTabEmpty)

It would be nice to get the file name and line number of any JS that triggered this. Thoughts?

Here's a best effort to get file names and line numbers

If we don't have a current js context, I assume it can't be a CPOW, so use NS_WARNING rather than logging to the js console.

Comment on attachment 8540978 [details] [diff] [review] warn_unsafe_CPOW.patch Review of attachment 8540978 [details] [diff] [review]: ----------------------------------------------------------------- ::: ipc/glue/MessageChannel.cpp @@ +707,5 @@ > > bool > MessageChannel::Send(Message* aMsg, Message* aReply) > { > + if (mCurrentTransaction == 0 && XRE_GetProcessType() == GeckoProcessType_Default) { This looks good, but this code doesn't really belong here: it's too specific to CPOWs and PContent. I would suggest adding a virtual method to MessageListener (defined in MessageLink.h). You could call it OnBeginSyncTransaction or something. The default implementation would do nothing. The code in MessageChannel would just do: if (mCurrentTransaction == 0) mListener->OnBeginSyncTransaction(); Then you would override this method in ContentParent (which indirectly inherits from MessageListener), putting the code below there. @@ +711,5 @@ > + if (mCurrentTransaction == 0 && XRE_GetProcessType() == GeckoProcessType_Default) { > + nsCOMPtr<nsIConsoleService> console(do_GetService(NS_CONSOLESERVICE_CONTRACTID)); > + JSContext *cx = nsContentUtils::GetCurrentJSContext(); > + if (console && cx) { > + const char* filename; I'd recommend initializing filename to "" here sunce GetCallingLocation doesn't always fill it in. @@ +717,5 @@ > + nsJSUtils::GetCallingLocation(cx, &filename, &lineno); > + nsCOMPtr<nsIScriptError> error(do_CreateInstance(NS_SCRIPTERROR_CONTRACTID)); > + error->Init(NS_LITERAL_STRING("unsafe CPOW usage"), NS_ConvertUTF8toUTF16(filename), > + EmptyString(), lineno, 0, nsIScriptError::warningFlag, "CPOW"); > + console->LogMessage(error); bholley or mrbkap are better reviewers for this code. Maybe give one of them the ContentParent review and I'll review the MessageChannel part. @@ +719,5 @@ > + error->Init(NS_LITERAL_STRING("unsafe CPOW usage"), NS_ConvertUTF8toUTF16(filename), > + EmptyString(), lineno, 0, nsIScriptError::warningFlag, "CPOW"); > + console->LogMessage(error); > + } else { > + NS_WARNING("Unsafe synchronouse IPC message"); No 'e' in synchronous.

Need one of mrbkap or bholley for the ContentParent bit (as billm suggested) and billm for MessageChannel and MessageList

Comment on attachment 8543340 [details] [diff] [review] warn_unsafe_CPOW.patch Review of attachment 8543340 [details] [diff] [review]: ----------------------------------------------------------------- ::: ipc/glue/MessageChannel.cpp @@ +16,5 @@ > #include "nsDebug.h" > #include "nsISupportsImpl.h" > #include "nsContentUtils.h" > +#include "nsIConsoleService.h" > +#include "nsIScriptError.h" Please remove these two. @@ +708,5 @@ > bool > MessageChannel::Send(Message* aMsg, Message* aReply) > { > + if (mCurrentTransaction == 0) > + mListener->OnBeginSyncTransaction(); Let's move this after the assertions (so right after AssertNotCurrentThreadOwns). ::: ipc/glue/MessageLink.h @@ +87,5 @@ > } > virtual void OnExitedCall() { > NS_RUNTIMEABORT("default impl shouldn't be invoked"); > } > + virtual void OnBeginSyncTransaction() { Please add a comment: "This callback is called when a sync message is sent that begins a new IPC transaction (i.e., when it is not part of an existing sequence of nested messages)."

Comment on attachment 8543340 [details] [diff] [review] warn_unsafe_CPOW.patch Review of attachment 8543340 [details] [diff] [review]: ----------------------------------------------------------------- Please base this patch on top of bug 1117851. r=me with those two things. ::: dom/ipc/ContentParent.cpp @@ +1616,5 @@ > + uint32_t lineno = 0; > + nsJSUtils::GetCallingLocation(cx, &filename, &lineno); > + nsCOMPtr<nsIScriptError> error(do_CreateInstance(NS_SCRIPTERROR_CONTRACTID)); > + error->Init(NS_LITERAL_STRING("unsafe CPOW usage"), NS_ConvertUTF8toUTF16(filename), > + EmptyString(), lineno, 0, nsIScriptError::warningFlag, "CPOW"); I think you want the category to be "chrome javascript" (which devtools know about) unless you have reason to do otherwise.