763773 - replace WrapperIsNotMainThreadOnly() with false

Status: RESOLVED FIXED

(Core :: XPConnect, defect)

Description

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

WrapperIsNotMainThreadOnly uses a few heuristics to figure out if we should trace through a JS object wrapper.  But it seems like all JS is mainthread-only now, so maybe we can drop this check?  I admit I'm not entirely sure what the goal of this check is.  I'm guessing, from looking at CC logs, that this logic is triggered at least a bit, for StatementRow, StatementParams and AsyncStatementParams.

Comment 1

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

The comment around there are:
    // We do not want to add wrappers to the cycle collector if they're not
    // explicitly marked as main thread only, because the cycle collector isn't
    // able to deal with objects that might be used off of the main thread. We
    // do want to explicitly mark them for cycle collection if the wrapper has
    // an external reference, because the wrapper would mark the JS object if
    // we did add the wrapper to the cycle collector.

    // If the native participates in cycle collection then we know it can only
    // be used on the main thread. In that case we assume the wrapped native
    // can only be used on the main thread too.

Comment 2

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

I think that if we trace a native that isn't a CCed class, it will just get canonicalized to null, and then NoteXPCOMChild will just return, so we won't add the child.

Comment 3

[Bobby Holley (:bholley)]

I'm in the process of ripping this stuff out over in bug 755255. It's currently waiting on review.

Comment 4

[Blake Kaplan (:mrbkap) (inactive)]

Do we have any way of detecting off-main-thread JS use from extensions for easily figuring out if an extension is doing evil stuff (even debug only)?

Comment 5

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

Luke added a bunch of fatal asserts at various entry points into the JS engine before he landed his patches that single thread the JS engine, IIRC.

Comment 6

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

(release mode fatal asserts)

Comment 7

[Blake Kaplan (:mrbkap) (inactive)]

Right, perfect.

Comment 8

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

bholley said that we could assume that WrapperIsNotMainThreadOnly is just false, because places that use XPConnect off the main thread should die to fatal asserts.  If we do that, we can simplify nsXPConnect::Traverse quite a bit.

This will affect cycle collector behavior, so it deserves a little bit of investigation.  We could potentially end up visiting more objects.

Comment 9

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

Attachment: untested WIP

This is on top of some patches that haven't landed yet, and needs testing, but imagine for a moment the beautiful future.  If we can assume WrapperIsNotMainThreadOnly should always return false, then the whole dontTraverse/markJSObject mess at the start of nsXPConnect::Traverse melts away, like tears in the rain.

Comment 10

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

Attachment: replace and simplify

Rebased the patch on top of my compartment merging patch.

The objects that were getting marked black by this check just form a cycle with an XPCWN that has no children.  Any other children of those nodes are optimized out, so removing this should only add a dozen or so nodes to the graph.

https://tbpl.mozilla.org/?tree=Try&rev=59be989c13fe

Comment 11

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

Comment on attachment 637278 [details] [diff] [review]
replace and simplify

Try run looks good.  No hurry on reviewing this, I may wait a bit to land it, as this affects the way another crash I'm looking at works.

Comment 12

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Comment on attachment 637278 [details] [diff] [review]
replace and simplify

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

This is great!

::: js/xpconnect/src/nsXPConnect.cpp
@@ +883,4 @@
>          return;
>  
> +    JSObject *obj = static_cast<JSObject*>(p);
> +    NoteGCThingXPCOMChildren(js::GetObjectClass(obj), obj, cb);

I think this would be a little clearer if it were structured as follows:

if (traceKind == JSTRACE_OBJECT) {
    JSObject *obj = static_cast<JSObject*>(p);
    NoteGCThingXPCOMChildren(js::GetObjectClass(obj), obj, cb);
}

Comment 13

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

(In reply to Bill McCloskey (:billm) from comment #12)
> I think this would be a little clearer if it were structured as follows:

That's a good point.  I was being too dogmatic with my early returning.

Comment 14

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

Re-trying: https://tbpl.mozilla.org/?tree=Try&rev=ca183cb30400

Comment 15

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

Hurray.

https://hg.mozilla.org/integration/mozilla-inbound/rev/8832cdd52d1c

Comment 16

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/8832cdd52d1c