Last Comment Bug 742841 - Store Debugger.X objects in the cross-compartment map
: Store Debugger.X objects in the cross-compartment map
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: unspecified
: All All
: -- normal (vote)
: mozilla16
Assigned To: Bill McCloskey (:billm)
:
: Jason Orendorff [:jorendorff]
Mentors:
Depends on:
Blocks: 743396 754495
  Show dependency treegraph
 
Reported: 2012-04-05 12:21 PDT by Bill McCloskey (:billm)
Modified: 2012-06-12 03:08 PDT (History)
4 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
patch (23.83 KB, patch)
2012-04-05 12:21 PDT, Bill McCloskey (:billm)
jorendorff: review+
Details | Diff | Splinter Review
rebased patch (8.57 KB, patch)
2012-05-29 16:23 PDT, Bill McCloskey (:billm)
wmccloskey: review+
Details | Diff | Splinter Review

Description Bill McCloskey (:billm) 2012-04-05 12:21:40 PDT
Created attachment 612642 [details] [diff] [review]
patch

This patch adds Debugger objects to the cross-compartment map. I have a later patch where I want to be able to easily iterate over all cross-compartment references.
Comment 1 Jason Orendorff [:jorendorff] 2012-04-09 17:15:47 PDT
Comment on attachment 612642 [details] [diff] [review]
patch

Review of attachment 612642 [details] [diff] [review]:
-----------------------------------------------------------------

This looks great. r=me.

::: js/src/jscompartment.cpp
@@ +504,5 @@
> +        JS_ASSERT(static_cast<Cell *>(value.toGCThing())->compartment() == this);
> +
> +        if (IsAboutToBeFinalized(key.wrapped) ||
> +            IsAboutToBeFinalized(value) ||
> +            (key.debugger && IsAboutToBeFinalized(key.debugger)))

OK. This is the only tricky bit, and I think this is correct.

IsAboutToBeFinalized(value) will never be true if key.debugger and key.wrapped are both still live, thanks to all that complex markAllIteratively code in Debugger.cpp that you left as-is. Right?

::: js/src/jscompartment.h
@@ +179,5 @@
> +    Kind kind;
> +    JSObject *debugger;
> +    js::gc::Cell *wrapped;
> +
> +    CrossCompartmentKey() {}

Maybe initialize to NULL. (HashMap::remove requires this default constructor; the value is never used, so an empty constructor is correct--but deterministic behavior is correct too and only costs a few insns. Your call.)
Comment 2 Bill McCloskey (:billm) 2012-04-09 17:23:58 PDT
(In reply to Jason Orendorff [:jorendorff] from comment #1)
> IsAboutToBeFinalized(value) will never be true if key.debugger and
> key.wrapped are both still live, thanks to all that complex
> markAllIteratively code in Debugger.cpp that you left as-is. Right?

This is true, but I think it's because of the weakmap that the debugger keeps from Objects to Debugger.Objects. If the debugger is alive, its weakmap is traced. The weakmap key is key.wrapped, and if that's alive then the weakmap keeps value alive.
Comment 3 Jason Orendorff [:jorendorff] 2012-04-09 18:37:15 PDT
Agreed.
Comment 4 Bill McCloskey (:billm) 2012-05-29 16:23:57 PDT
Created attachment 628135 [details] [diff] [review]
rebased patch

I've got this running on tryserver now.
Comment 6 Graeme McCutcheon [:graememcc] 2012-06-12 03:08:11 PDT
https://hg.mozilla.org/mozilla-central/rev/463d5ad214e5

Note You need to log in before you can comment on or make changes to this bug.