Closed Bug 443878 Opened 16 years ago Closed 16 years ago

XPConnect doesn't always suspend requests when switching contexts

Categories

(Core :: XPConnect, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED

People

(Reporter: bent.mozilla, Assigned: bent.mozilla)

References

Details

Attachments

(1 file, 1 obsolete file)

Attached patch Patch (obsolete) — Splinter Review
While working on bug 437152 I hit some deadlocks where XPConnect wasn't properly suspending requests on a context that was already on the stack when it switched to a new context. Patch attached.
Attachment #328301 - Flags: superreview?(jst)
Attachment #328301 - Flags: review?(jst)
Version: unspecified → Trunk
Comment on attachment 328301 [details] [diff] [review]
Patch

     // We need to use the safe context for this thread because we don't want
     // to parent the new (and cached forever!) function object to the current
     // JSContext's global object. That would be bad!
 
-    JSContext* cx = ccx.GetSafeJSContext();
-    if(!cx)
-        return JS_FALSE;
-

Move the comment with this change too.

+    {
+      // Switching
+      AutoJSSuspendRequest req(ccx);
+
+      JSAutoRequest ar(cx);
+  
+      fun = JS_NewFunction(cx, callback, argc, flags, nsnull, memberName);
+    }

Do we need to worry about the case where ccx == cx here? I guess it's not the end of the world to end a request only to begin another one, and then the reverse too. It's certainly possible to hit that case, but I don't know for sure how common of a case that is.

r+sr=jst with that figured out one way or another.
Attachment #328301 - Flags: superreview?(jst)
Attachment #328301 - Flags: superreview+
Attachment #328301 - Flags: review?(jst)
Attachment #328301 - Flags: review+
Attached patch Patch, v1.1Splinter Review
Moved the comment and fixed the case where cx and ccx are the same. I'm not using the auto helpers anymore, think that's ok?
Attachment #328301 - Attachment is obsolete: true
Attachment #330643 - Flags: superreview?(jst)
Attachment #330643 - Flags: review?(jst)
Attachment #330643 - Flags: superreview?(jst)
Attachment #330643 - Flags: superreview+
Attachment #330643 - Flags: review?(jst)
Attachment #330643 - Flags: review+
Pushed changeset be03f86900e1.
Status: NEW → RESOLVED
Closed: 16 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.