TestShell environments in content children cannot register nsIMessageListenerManager listeners

RESOLVED WONTFIX

Status

()

Core
IPC
RESOLVED WONTFIX
5 years ago
5 years ago

People

(Reporter: jimb, Assigned: jimb)

Tracking

Trunk
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 2 obsolete attachments)

(Assignee)

Description

5 years ago
Created attachment 729880 [details]
Test case. Run with xpcshell; child process crashes

xpcshell has a function for creating content child processes and communicating with them, called 'sendCommand'. This function sends an IPDL PTestShell constructor message to the child, which creates an XPCShellEnvironment in which to evaluate JavaScript sent from the parent. sendCommand then sends a PTestShell::ExecuteCommand message to actually evaluate some code in the child.

The 'sendCommand' function is the only way to exercise child processes in xpcshell tests. However, if code run in the child process using 'sendCommand' registers listeners with nsIMessageListenerManager instances, and those listeners use the global functions defined by XPCShellEnvironment, the child process will crash.

XPCShellEnvironment::Init creates a JSContext and sets its "private" pointer (with JS_SetContextPrivate) to point to the XPCShellEnvironment. It then creates a global object and populates it with some native functions that expect to be able to find the XPCShellEnvironment stored in the "private" pointer of the JSContext they were passed.

If code passed to sendCommand registers a listener with some process message manager (like @mozilla.org/childprocessmessagemanager;1), and a message arrives, then the event loop tries to run the listener. However, process message managers run listeners using a JSContext obtained from nsContentUtils::ThreadJSContextStack()->GetSafeJSContext(), not the JSContext the XPCShellEnvironment was created with. If that listener uses any of the primitives defined by the XPCShellEnvironment, they will fetch the "private" pointer of a different JSContext, and try to interpret it as an XPCShellEnvironment, leading to a crash.

What's kind of silly is that everything that the native functions need XPCShellEnvironment for would (today; I know things were different before) be better obtained elsewhere. The context should come from GetSafeJSContext; the 'quitting' flag and exit code are ignored; the principals could come from the callee's compartment.

The attached code, if run with xpcshell, causes the child process to segfault trying to use a null pointer as an XPCShellEnvironment pointer.
(Assignee)

Updated

5 years ago
Blocks: 848408
(Assignee)

Comment 1

5 years ago
Created attachment 732513 [details] [diff] [review]
Make XPCShellEnvironment a singleton, rather than finding it via the JSContext's private pointer.

Patch. I still need to integrate the test (some light hacking required, since 'quit' is also broken).
Assignee: nobody → jimb
Status: NEW → ASSIGNED
(Assignee)

Comment 2

5 years ago
Created attachment 732622 [details] [diff] [review]
Make XPCShellEnvironment a singleton, rather than finding it via the JSContext's private pointer.

Complete patch, including test case. Try:
https://tbpl.mozilla.org/?tree=Try&rev=27836ef67e89
Attachment #729880 - Attachment is obsolete: true
Attachment #732513 - Attachment is obsolete: true
(Assignee)

Comment 3

5 years ago
Comment on attachment 732622 [details] [diff] [review]
Make XPCShellEnvironment a singleton, rather than finding it via the JSContext's private pointer.

Try push looks okay.
Attachment #732622 - Flags: review?(benjamin)
(Assignee)

Comment 4

5 years ago
Comment on attachment 732622 [details] [diff] [review]
Make XPCShellEnvironment a singleton, rather than finding it via the JSContext's private pointer.

Removing review request, as there's a B2G xpcshell failure I'd failed to notice in the Try push which definitely looks related. I'll sort that out before asking for review again.
Attachment #732622 - Flags: review?(benjamin)
(Assignee)

Comment 5

5 years ago
Added some diagnostics, trying that failing test alone:
https://tbpl.mozilla.org/?tree=Try&rev=c5fe8079226f
(Assignee)

Comment 6

5 years ago
With the patch in bug 874755, we may not need TestShell and XPCShellEnvironment at all any more, so working on XPCShellEnvironment is probably not worth it.
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → WONTFIX
You need to log in before you can comment on or make changes to this bug.