Note: There are a few cases of duplicates in user autocompletion which are being worked on.

Store Debugger.X objects in the cross-compartment map

RESOLVED FIXED in mozilla16

Status

()

Core
JavaScript Engine
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: billm, Assigned: billm)

Tracking

unspecified
mozilla16
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 1 obsolete attachment)

(Assignee)

Description

5 years ago
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.
Attachment #612642 - Flags: review?(jorendorff)
(Assignee)

Updated

5 years ago
Blocks: 743396
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.)
Attachment #612642 - Flags: review?(jorendorff) → review+
(Assignee)

Comment 2

5 years ago
(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.
Agreed.
Blocks: 754495
(Assignee)

Comment 4

5 years ago
Created attachment 628135 [details] [diff] [review]
rebased patch

I've got this running on tryserver now.
Attachment #612642 - Attachment is obsolete: true
Attachment #628135 - Flags: review+
(Assignee)

Comment 5

5 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/463d5ad214e5
Target Milestone: --- → mozilla16
https://hg.mozilla.org/mozilla-central/rev/463d5ad214e5
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.