[e10s] Warn when using CPOWs when content process isn't in a safe state

RESOLVED FIXED in mozilla37

Status

()

defect
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: billm, Assigned: blassey)

Tracking

(Blocks 1 bug)

unspecified
mozilla37
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(e10sm4+)

Details

Attachments

(1 attachment, 3 obsolete attachments)

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)
Blocks: 1109869
Posted patch warn_unsafe_CPOW.patch (obsolete) — Splinter Review
It would be nice to get the file name and line number of any JS that triggered this. Thoughts?
Assignee: wmccloskey → blassey.bugs
Attachment #8540903 - Flags: review?(wmccloskey)
Posted patch warn_unsafe_CPOW.patch (obsolete) — Splinter Review
Here's a best effort to get file names and line numbers
Attachment #8540903 - Attachment is obsolete: true
Attachment #8540903 - Flags: review?(wmccloskey)
Attachment #8540919 - Flags: review?(wmccloskey)
Posted patch warn_unsafe_CPOW.patch (obsolete) — Splinter Review
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.
Attachment #8540919 - Attachment is obsolete: true
Attachment #8540919 - Flags: review?(wmccloskey)
Attachment #8540978 - Flags: review?(wmccloskey)
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.
Attachment #8540978 - Flags: review?(wmccloskey)
Need one of mrbkap or bholley for the ContentParent bit (as billm suggested) and billm for MessageChannel and MessageList
Attachment #8540978 - Attachment is obsolete: true
Attachment #8543340 - Flags: review?(wmccloskey)
Attachment #8543340 - Flags: review?(mrbkap)
Attachment #8543340 - Flags: review?(bobbyholley)
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)."
Attachment #8543340 - Flags: review?(wmccloskey) → review+
Depends on: 1117851
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.
Attachment #8543340 - Flags: review?(mrbkap)
Attachment #8543340 - Flags: review?(bobbyholley)
Attachment #8543340 - Flags: review+
https://hg.mozilla.org/mozilla-central/rev/d4b0aadb4e44
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla37
You need to log in before you can comment on or make changes to this bug.