Closed Bug 1254376 Opened 4 years ago Closed 4 years ago

[jsdbg2] GlobalObject's debugger vector is sort of weak, so it needs read barriers

Categories

(Core :: JavaScript Engine, defect, major)

defect
Not set
major

Tracking

()

RESOLVED FIXED
mozilla48
Tracking Status
firefox48 --- fixed

People

(Reporter: jimb, Assigned: jonco)

Details

Attachments

(1 file, 1 obsolete file)

Access to a GlobalObject's observing Debuggers should use a read barrier.

A GlobalObject's vector of the js::Debuggers of which it is a debuggee is not traced; rather, its contents are edited by code in js/src/vm/Debugger.cpp that copes with Debugger objects and GlobalObjects being GC'd. However, this vector is used to notify observing Debuggers of activity that takes place in the global's scope. As a consequence, we can end up invoking hooks on Debuggers that the incremental GC has no way of knowing are live.

Bug 1252453 is an example of the kind of problem this engenders.

The classic problem with finalizers is that it's hard to tell what order to run them in, because the collector doesn't understand the dependencies objects have on each other. There's a bunch of hair in Debugger.cpp, handling two things at once and running steps from the sweep phase and other such merriment which is clearly trying to cope with these problems. I'm sure there's a more principled way to address this.
Attached patch bug1254376-debugger-barrier (obsolete) — Splinter Review
What do you think about relaxing the type constraints on barriers to allow us to use them with non-GC pointers?
Attachment #8731664 - Flags: feedback?(terrence)
Comment on attachment 8731664 [details] [diff] [review]
bug1254376-debugger-barrier

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

In this case I think it makes sense. My only worry is that someone will do Heap<LargeContainer> and then we'll be stuck trying to figure out why the GC is janking. I guess the lack of barrier methods on our container classes prevents this to a certain degree.

I'm not against this, given the benefits, but we should be sure to update the documentation to make it clearer where barriers should and should not be used.
Attachment #8731664 - Flags: feedback?(terrence) → feedback+
Assignee: nobody → jcoppeard
Attachment #8731804 - Flags: review?(jimb)
Attachment #8731664 - Attachment is obsolete: true
Comment on attachment 8731804 [details] [diff] [review]
bug1254376-debugger-barrier v2

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

This passes all tests? Delightful.
Attachment #8731804 - Flags: review?(jimb) → review+
Thanks very much for the patch, Jon!
https://hg.mozilla.org/mozilla-central/rev/a803bb1e41a2
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
You need to log in before you can comment on or make changes to this bug.