464676 - Cycle collector sometimes unlinks live cycles

Status: RESOLVED FIXED

(Core :: XPConnect, defect, P2)

Description

[Peter Van der Beken [:peterv]]

We currently only add the JS objects of XPCWrappedNatives to the cycle collector if they are marked as main-thread-only. We really should be adding all the JS objects of XPCWrappedNatives, but not traverse to the XPCWrappedNatives themselves. With the current code we do not detect if a cycle is held by a JS object of a non-main-thread-only wrapper and so we try to unlink that cycle, even though it's still 'live'. Anything held by the XPCWrappedNative will be marked black if we don't traverse to it (because it'll have a missing refcount).

Comment 1

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Sounds like this can cause random crashers in multi-threaded JS code then? Like the new places code?

Comment 2

[Peter Van der Beken [:peterv]]

I don't think it'll cause crashes, unless the unlinking makes us access nulled out members (but that could then happen without this bug too). We'll still mark all the live JS objects once cycle collection is done, because we never set the "unlink" bit on the non-main-thread-only XPCWrappedNatives and they're the ones holding everything else alive.

Comment 3

[Johnny Stenback (:jst)]

Sounds like this should be fixed for 1.9.1.

Comment 4

[Peter Van der Beken [:peterv]]

Attachment: v1

Comment 5

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

Comment on attachment 355816 [details] [diff] [review]
v1

Waiting on a new patch.

>+        if(IS_PROTO_CLASS(clazz))

We chatted on IRC and this should probably become:

  if(IS_WRAPPER_CLASS(clazz) || IS_TEAROFF_CLASS(clazz))

>+        else {

nit: separate line for that '{' (I <3 xpconnect).

Comment 6

[Peter Van der Beken [:peterv]]

Attachment: v2

This ended up slightly different, since we were already dealing with tearoffs separately in nsXPConnect::Traverse. Also decided to not call GetWrappedNativeOfJSObject since we don't need most of that function.

Comment 7

[Peter Van der Beken [:peterv]]

Comment on attachment 357075 [details] [diff] [review]
v2

Verified in the debugger that it works ok. No leaks on mochitest.

Comment 8

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

Comment on attachment 357075 [details] [diff] [review]
v2

This looks good!

Comment 9

[Peter Van der Beken [:peterv]]

This is the right thing to do, but thinking this over I think it doesn't completely solve the problem. We need to add the JS objects of non-main thread only wrappers that have external references to the cycle collector as marked, since if we don't traverse to those wrappers the wrappers themselves can't mark the JS objects as black.

Comment 10

[Peter Van der Beken [:peterv]]

Attachment: v3

Ben: sorry for the third review request in a row, I don't think I'll come up with more issues ;-).

Comment 11

[Peter Van der Beken [:peterv]]

Comment on attachment 357383 [details] [diff] [review]
v3

Ugh, this isn't right either.

Comment 12

[Peter Van der Beken [:peterv]]

Attachment: v3.1

Comment 13

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

Comment on attachment 357422 [details] [diff] [review]
v3.1

Cool, that makes sense. r=me.

Comment 14

[Peter Van der Beken [:peterv]]

Had to back out, chrome and browser tests are leaking, which is worrying since it means we might be relying on unlinking live cycles to mask leaks.

Comment 15

[Peter Van der Beken [:peterv]]

Attachment: v3.2

We have a number of objects that participate in cycle collection but that don't have classinfo. These objects should only be used on the main thread, but we can't detect that from the wrapper. We can detect it by checking if they participate in cycle collection, so I added code to WrapperIsNotMainThreadOnly that does just that. It's a bit expensive (QI), but on the other hand it might make us traverse less XPCWrappedNatives than we do currently.

This seems to fix all the leaks, I'll double-check that before checking in.

Comment 16

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

Comment on attachment 358467 [details] [diff] [review]
v3.2

This looks good, though if we need to (want to?) optimize this I think we should do this QI up front when creating the wrappers and set a bit somewhere.

Comment 17

[Peter Van der Beken [:peterv]]

Checked in again and backed out again, no leaks on the unit boxes but the leak boxes turned red with:

*** Potential deadlock between XPCJSRuntime::mMapLockMonitor@22698b0 and nsComponentManagerImplMonitor@c3bb30

They did have one green cycle after my checkin and I can't reproduce locally (and the stacks in the logs are useless). I'm not sure this patch was to blame, so I might try landing again tomorrow.

Comment 18

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

I've seen that assertion lots of times before without this patch... Maybe this increases the frequency or something but the problem is elsewhere.

Comment 19

[Peter Van der Beken [:peterv]]

Relanded on trunk, seems to stick.

Comment 20

[Peter Van der Beken [:peterv]]

http://hg.mozilla.org/mozilla-central/rev/0b0850b7f90b
http://hg.mozilla.org/releases/mozilla-1.9.1/rev/4e308e5150bb