765305 - WrapperIsNotMainThreadOnly() wrappers can violate black-gray invariant

Status: RESOLVED WORKSFORME

(Core :: XPConnect, defect)

Description

[Andrew McCreight [:mccr8]]

In nsXPConnect::Traverse, if an object is a wrapper such that WrapperIsNotMainThreadOnly() and HasExternalReference(), then the CC treats it as marked black, even though it may not be.  I think this is not sound in the post-GC/CC split world, because the object's children may be gray.  In other words, if the idea is to treat the wrapper as black so we keep it and its children alive, that won't really work, as we can end up freeing some C++ objects reachable from JS (in a controlled way, this isn't use-after-free, like all the other black-gray bugs).

As I said in bug 763773, I think we do actually hit this case for a few classes (StatementRow, StatementParams and AsyncStatementParams).  I regularly see these classes in the CC graph as being marked, and the CC filters out all objects that are actually marked, so I think these must be "fake marked" objects.

I don't think there's anything we can really do to fix this, but this code should go away anyways as bholley makes XPConnect single-thread only.

Marking sec-moderate, because that's what we've been using for black-gray problems.

Comment 1

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Aren't all wrappers main thread only these days?

Comment 2

[Andrew McCreight [:mccr8]]

That's what I was hoping when I filed bug 763773, but nobody replied in the affirmative.  Some things appear to fail the WrapperIsNotMainThreadOnly() test, but given that JS is single threaded, I hope the test is just a conservative approximation that is missing some cases.

Comment 3

[Bobby Holley (:bholley)]

(In reply to Andrew McCreight [:mccr8] from comment #2)
> That's what I was hoping when I filed bug 763773, but nobody replied in the
> affirmative.  Some things appear to fail the WrapperIsNotMainThreadOnly()
> test, but given that JS is single threaded, I hope the test is just a
> conservative approximation that is missing some cases.

Everything _should_ be main thread only. But it looks like WrapperIsNotMainThreadOnly uses a CC-related heuristic to guess in cases where the nsIClassInfo doesn't have the MAIN_THREAD_ONLY flag (which could be missing in certain places).

We should currently abort if XPConnect is ever used off-main-thread. I have patches to rip this stuff out entirely waiting on review, but feel free to just land a patch that makes WrapperIsNotMainThreadOnly() just return false if this is an issue for the CC.

Comment 4

[Andrew McCreight [:mccr8]]

Awesome, thanks.  I'll nuke WrapperIsNotMainThreadOnly at some point.  It should  simplify nsXPConnect::Traverse quite a bit.

Comment 5

[Andrew McCreight [:mccr8]]

This function doesn't exist any more on 17, so it is fixed: the markedness of JS for the CC is now based purely on their GC mark bits.

This isn't worth backporting, in my opinion.