718029 - don't skip JSContexts with outstanding requests during CC traversal

Status: RESOLVED FIXED

(Core :: XPConnect, defect)

Description

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

The CC adds JSContexts as roots, but skips those that have outstanding requests, because they are necessarily alive.  However, JSContexts can have JS objects as children, and it is not okay to skip traversal of a live edge from C++ to JS, because it can make the CC throw away (by NULLing out pointers) to things that are still reachable from JS.

Not a huge deal, because the worst thing this should cause is some null-pointer derefs.  Fix is just to remove the two lines.

Giving this moderate because that's what bug 690970, which is the same kind of bug.

Comment 1

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

In some casual browsing, I never noticed this optimization doing anything, but I wasn't trying to do things with outstanding requests, whatever those are.

Comment 2

[Bobby Holley (:bholley)]

Note that requests will be going away soon with bug 650411.

Comment 3

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

Yeah, this is only going to be a problem if somehow the cycle collector fires off while you have some multi-threaded JS going on or something.  It seems like this is barely a problem, and a bogus optimization besides.

Comment 4

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

Attachment: remove the bogus optimization

Comment 5

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Now that we don't allow JS from the main runtime on non-main threads I don't think this makes any sense.

Comment 6

[Olli Pettay [:smaug][[e-mail]]]

So the case when there can be GetOutstandingRequests is when either CC is called
explicitly via JS or when there is sync XHR or modal dialog/window open when CC runs.

Comment 7

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

Probably makes sense to just roll this patch into something improving skipping of JSContexts in the CC.

Comment 8

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 588516 [details] [diff] [review]
remove the bogus optimization

Hopefully the global object is usually black so that this doesn't affect to
the graph size.

Comment 9

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

(In reply to Andrew McCreight [:mccr8] from comment #7)
> Probably makes sense to just roll this patch into something improving
> skipping of JSContexts in the CC.

or not. ;)

Yeah, I'll check to see if this does anything awful to the CC graph.

Comment 10

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

Well, actually, I already was doing some examination of the behavior of skipping JSContexts, and I never saw this optimization triggered, so it should be okay.

Comment 11

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

https://hg.mozilla.org/integration/mozilla-inbound/rev/26c821293430

Comment 12

[Joe Drew (not getting mail)]

https://hg.mozilla.org/mozilla-central/rev/26c821293430