1624810 - Add a flag to indicate which JS holders can contain GC things more than one zone

Status: RESOLVED FIXED

(Core :: XPCOM, task)

Description

[Jon Coppeard (:jonco)]

To allow us to store JS holders separately by zone we need to know which ones are can contain pointers to GC things in more than one zone. This bug is about adding a flag to indicate this and enforcing it with assertions.

Comment 1

[Jon Coppeard (:jonco)]

Attachment: Bug 1624810 - Replace individual nsCycleCollectionParticipant flags with a bit field to make it easier pass multiple flags through derived class oonstructors r?mccr8

Currently classes derived from nsCycleCollectionParicipant have one flag to pass and adding more makes the class delcaration macros get messy really quickly. This patch replaces boolean flags with a bitfield which makes adding new flags easier.

Comment 2

[Jon Coppeard (:jonco)]

Attachment: Bug 1624810 - Add a nsCycleCollectionParticipant flag for JS holders that can have GC things in multiple zones r?mccr8

This adds the flag itself and a means of setting it for a cycle collected class.

The proper way to do this would be to add versions of all the NS_DECL_CYCLE_COLLECTION_CLASS macros that also set this flag but we end up needing to create separate versions for five of these and it's a lot of macro code to add. Here I added a version of NS_IMPL_CYCLE_COLLECTION_CLASS that sets the flag. This has the disadvantage that it wouldn't work well if we needed to set the flag on a base class as we'd have to use this macro for every derived class. However that situation doesn't actually arise (and ideally this flag will bet set on the fewest number of classes possible).

What do you think, is this OK?

Depends on D68194

Comment 3

[Jon Coppeard (:jonco)]

Attachment: Bug 1624810 - Set the multi-zone JS holder flag on appropriate cycle collected classes r?mccr8

Set the flag on the affected classes, which are:

• CallbackTimeoutHandler
• nsJSArgArray
• CallbackObject
• Console
• MessageEvent
• IDBIndexCursor
• ExtendableMessageEvent
• JSPurpleBuffer

Depends on D68195

Comment 4

[Jon Coppeard (:jonco)]

Attachment: Bug 1624810 - Assert that only JS holders with the approprate flag set can contain pointers to GC things in more than one zone r?mccr8

Depends on D68196

Comment 5

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/c6da88c617db
Replace individual nsCycleCollectionParticipant flags with a bit field to make it easier pass multiple flags through derived class oonstructors r=mccr8
https://hg.mozilla.org/integration/autoland/rev/4dcdeed2265f
Add a nsCycleCollectionParticipant flag for JS holders that can have GC things in multiple zones r=mccr8
https://hg.mozilla.org/integration/autoland/rev/f4d11e6a6e3f
Set the multi-zone JS holder flag on appropriate cycle collected classes r=mccr8
https://hg.mozilla.org/integration/autoland/rev/258117b770a5
Assert that only JS holders with the approprate flag set can contain pointers to GC things in more than one zone r=mccr8

Comment 6

[Alexandru Michis [:malexandru]]

https://hg.mozilla.org/mozilla-central/rev/c6da88c617db
https://hg.mozilla.org/mozilla-central/rev/4dcdeed2265f
https://hg.mozilla.org/mozilla-central/rev/f4d11e6a6e3f
https://hg.mozilla.org/mozilla-central/rev/258117b770a5

Comment 7

[Andrew Sutherland [:asuth] (he/him)]

(In reply to Jon Coppeard (:jonco) from comment #3)

Set the flag on the affected classes, which are:

• Console
• MessageEvent
• IDBIndexCursor
• ExtendableMessageEvent

It seems like these all involve structured clone deserialization which suggests maybe we're doing something wrong/weird when deserializing? Do either of you have thoughts on this and/or things we should look at in terms of the implementation?

Comment 8

[Andrew McCreight (out of office until 8/21) [:mccr8]]

For the console and message event stuff, I'd guess that the C++ object implements an interface that is exposed to chrome JS (so the wrapper will be in chrome's zone), but the data is holds onto is content JS. This is actually kind of a leaky pattern in general, because then nuking chrome to content references doesn't catch it.

Comment 9

[Andrew Sutherland [:asuth] (he/him)]

(In reply to Andrew McCreight [:mccr8] from comment #8)

For the console and message event stuff, I'd guess that the C++ object implements an interface that is exposed to chrome JS (so the wrapper will be in chrome's zone), but the data is holds onto is content JS. This is actually kind of a leaky pattern in general, because then nuking chrome to content references doesn't catch it.

Could this also then mean devtools and test logic that uses frame scripts?

Comment 10

[Andrew McCreight (out of office until 8/21) [:mccr8]]

(In reply to Andrew Sutherland [:asuth] (he/him) from comment #9)

Could this also then mean devtools and test logic that uses frame scripts?

I'm only talking about references from "chrome C++" to content JS. Frame scripts are JS, so content references will get nuked. The underlying debugger APIs do cause some of this (bug 1084605).