lift restrictions on order of XPCJSContextStack::Push and AutoRequest calls

RESOLVED INACTIVE

Status

()

Core
XPConnect
RESOLVED INACTIVE
8 years ago
2 months ago

People

(Reporter: Igor Bukanov, Unassigned)

Tracking

Trunk
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment)

(Reporter)

Description

8 years ago
+++ This bug was initially created as a clone of Bug #552266 +++

When working on bug 584673 I notices the following code pattern in xpc_EvalInSandbox, http://hg.mozilla.org/tracemonkey/file/f6c1d5ab87f9/js/src/xpconnect/src/xpccomponents.cpp#l3562 :

{
    JSAutoRequest req(cx);
    ...
}
....
{
   JSAutoRequest req(sandbox_cx);

   if (exception) {
       JSAutoRequest req(cx); // (*)

   }
}

With bug 552266 changes the above would be wrong if on entrance to xpc_EvalInSandbox the cx were in request that outo-suspended some other JSContext instance cx3. In this case the line marked with (*) would assert as it would imply that cx must suspend a request for sandbox_cx in addition for already suspended cx3.
(Reporter)

Comment 1

8 years ago
Created attachment 463938 [details] [diff] [review]
v1

The patch fixes the request order. It also changes XPCWrapper::RewrapObject called to rewrap the exception object, to use sandcx->GetJSContext(), not the cx, to match XPCWrapper::RewrapObject done for normal call.
Attachment #463938 - Flags: review?(mrbkap)
(Reporter)

Comment 2

8 years ago
Comment on attachment 463938 [details] [diff] [review]
v1

jsdStackFrame::Eval has the same no-longer-valid pattern of starting a request before doing Push. I will try to fix all such problematic places in one patch.
Attachment #463938 - Flags: review?(mrbkap)
(Reporter)

Comment 3

8 years ago
After trying to fix all the places with violated order of XPCJSContextStack::Push and AutoRequest calls and introducing a not-so-easy-to-find bug in the patch I decided to take another approach. The idea is to lift the restriction on the order  of the methods so Push can be called both before and after JS_BeginRequest and similarly with Pop and JS_EndRequest. This should fully restore the compatibility with the current code.

For that it is enough to provide JS_GetRequestContext API that Push/Pop can use to query the request status of the current thread and to insist in JS_RestoreFrameChain and JS_SaveFrameChain only that the current thread is in a request, not the passed JSContext instance.
Summary: xpc_EvalInSandbox can violate one request cx per thread assumption → lift restrictions on order of XPCJSContextStack::Push and AutoRequest calls
(Reporter)

Updated

7 years ago
Assignee: igor → nobody

Comment 4

2 months ago
Per policy at https://wiki.mozilla.org/Bug_Triage/Projects/Bug_Handling/Bug_Husbandry#Inactive_Bugs. If this bug is not an enhancement request or a bug not present in a supported release of Firefox, then it may be reopened.
Status: NEW → RESOLVED
Last Resolved: 2 months ago
Resolution: --- → INACTIVE
You need to log in before you can comment on or make changes to this bug.