Bill McCloskey [inactive unless it's an emergency] (:billm) Reporter
	Description • 10 years ago

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)

	Brad Lassey [:blassey] (use needinfo?) Assignee
	Comment 1 • 10 years ago

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

	Brad Lassey [:blassey] (use needinfo?) Assignee
	Comment 2 • 10 years ago

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

	Brad Lassey [:blassey] (use needinfo?) Assignee
	Comment 3 • 10 years ago

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.

	Bill McCloskey [inactive unless it's an emergency] (:billm) Reporter
	Comment 4 • 9 years ago

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.

	Brad Lassey [:blassey] (use needinfo?) Assignee
	Comment 5 • 9 years ago

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

	Bill McCloskey [inactive unless it's an emergency] (:billm) Reporter
	Comment 6 • 9 years ago

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)."

	Bobby Holley (:bholley)
	Comment 7 • 9 years ago

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.

	Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)
	Comment 8 • 9 years ago

https://hg.mozilla.org/mozilla-central/rev/d4b0aadb4e44