Closed
Bug 1097998
Opened 10 years ago
Closed 10 years ago
[e10s] Warn when using CPOWs when content process isn't in a safe state
Categories
(Core :: DOM: Content Processes, defect)
Core
DOM: Content Processes
Tracking
()
RESOLVED
FIXED
mozilla37
Tracking | Status | |
---|---|---|
e10s | m4+ | --- |
People
(Reporter: billm, Assigned: blassey)
References
Details
Attachments
(1 file, 3 obsolete files)
4.47 KB,
patch
|
billm
:
review+
bholley
:
review+
|
Details | Diff | Splinter Review |
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)
Assignee | ||
Updated•10 years ago
|
tracking-e10s:
--- → m4+
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?
Assignee: wmccloskey → blassey.bugs
Attachment #8540903 -
Flags: review?(wmccloskey)
Assignee | ||
Comment 2•10 years ago
|
||
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)
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.
Attachment #8540919 -
Attachment is obsolete: true
Attachment #8540919 -
Flags: review?(wmccloskey)
Attachment #8540978 -
Flags: review?(wmccloskey)
Reporter | ||
Comment 4•10 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.
Attachment #8540978 -
Flags: review?(wmccloskey)
Assignee | ||
Comment 5•10 years ago
|
||
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)
Reporter | ||
Comment 6•10 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)."
Attachment #8543340 -
Flags: review?(wmccloskey) → review+
Comment 7•10 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.
Attachment #8543340 -
Flags: review?(mrbkap)
Attachment #8543340 -
Flags: review?(bobbyholley)
Attachment #8543340 -
Flags: review+
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla37
You need to log in
before you can comment on or make changes to this bug.
Description
•