Last Comment Bug 763773 - replace WrapperIsNotMainThreadOnly() with false
: replace WrapperIsNotMainThreadOnly() with false
Product: Core
Classification: Components
Component: XPConnect (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla17
Assigned To: Andrew McCreight [:mccr8]
: Andrew Overholt [:overholt]
Depends on: 752764 755255
Blocks: CleanupCC 615762 765305
  Show dependency treegraph
Reported: 2012-06-11 16:57 PDT by Andrew McCreight [:mccr8]
Modified: 2012-08-07 10:10 PDT (History)
7 users (show)
See Also:
Crash Signature:
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---

untested WIP (4.21 KB, patch)
2012-06-18 16:00 PDT, Andrew McCreight [:mccr8]
no flags Details | Diff | Splinter Review
replace and simplify (4.40 KB, patch)
2012-06-27 15:05 PDT, Andrew McCreight [:mccr8]
wmccloskey: review+
Details | Diff | Splinter Review

Description Andrew McCreight [:mccr8] 2012-06-11 16:57:58 PDT
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 [:mccr8] 2012-06-11 16:59:13 PDT
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 [:mccr8] 2012-06-11 17:01:41 PDT
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) (busy with Stylo) 2012-06-12 01:58:54 PDT
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) 2012-06-15 14:24:47 PDT
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 [:mccr8] 2012-06-15 14:51:02 PDT
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 [:mccr8] 2012-06-15 14:51:21 PDT
(release mode fatal asserts)
Comment 7 Blake Kaplan (:mrbkap) 2012-06-15 15:42:54 PDT
Right, perfect.
Comment 8 Andrew McCreight [:mccr8] 2012-06-18 15:55:53 PDT
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 [:mccr8] 2012-06-18 16:00:02 PDT
Created attachment 634220 [details] [diff] [review]
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 [:mccr8] 2012-06-27 15:05:14 PDT
Created attachment 637278 [details] [diff] [review]
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.
Comment 11 Andrew McCreight [:mccr8] 2012-06-28 11:42:42 PDT
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 (:billm) 2012-06-28 11:54:55 PDT
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 [:mccr8] 2012-06-28 11:56:33 PDT
(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 [:mccr8] 2012-08-06 08:52:29 PDT
Comment 15 Andrew McCreight [:mccr8] 2012-08-06 13:43:51 PDT
Comment 16 Ed Morley [:emorley] 2012-08-07 07:37:47 PDT

Note You need to log in before you can comment on or make changes to this bug.