708480 - improved testing for double unlinks

Status: RESOLVED INCOMPLETE

(Core :: XPCOM, defect)

Description

[Andrew McCreight [:mccr8]]

Objects can be Unlinked twice if they are held in a wrapped native, and cycle collection runs twice in a row without a GC in between.  This is kind of a weird combination of factors, so there is probably not much testing of this.  It might be worth spending a little bit of effort to see if there are any other bugs like bug 669903 lurking out there.

I can see two ways this could be done:

1. With fuzzing, the cycle collector could be explicitly invoked twice in a row at various points.  This will only help for nodes held in wrapped natives.
2. Make the cycle collector always unlink twice in a row.  Then you could do a try run, fuzz, etc.  If we really wanted, this could be done in all debug builds, but I'm not sure if that is worthwhile.

I'll set up a build and see what happens.

Comment 1

[Jesse Ruderman]

Regarding (1), will calling nsIDOMWindowUtils.cycleCollect twice in a row do what you want? I think my fuzzer already does sometimes.

    window.QueryInterface(Components.interfaces.nsIInterfaceRequestor)
          .getInterface(Components.interfaces.nsIDOMWindowUtils)
          .cycleCollect();

Regarding (2), please use Try before fuzzing :)

Comment 2

[Andrew McCreight [:mccr8]]

Attachment: double unlink!

Comment 3

[Andrew McCreight [:mccr8]]

(In reply to Jesse Ruderman from comment #1)
> Regarding (1), will calling nsIDOMWindowUtils.cycleCollect twice in a row do
> what you want? I think my fuzzer already does sometimes.

Yup, that's what I meant.

> Regarding (2), please use Try before fuzzing :)

Unlinks are simple enough that I imagine try will find many problems, if there are any.  I'm curious if it will turn up anything for nsDOMStringMaps.

https://tbpl.mozilla.org/?tree=Try&rev=6e1a8c05dfc0

Comment 4

[Andrew McCreight [:mccr8]]

So far it is mostly just firing off this assertion a kajillion times:
###!!! ASSERTION: global object NULL before unlinking: 'cx->globalObject', file /builds/slave/try-osx-dbg/build/js/xpconnect/src/nsXPConnect.cpp, line 990

I'll try disabling that and restart a build tomorrow.

Comment 5

[Andrew McCreight [:mccr8]]

Disabled assertion and push to try, just for linux this time.
https://tbpl.mozilla.org/?tree=Try&rev=8fbb8ef60562

Comment 6

[Andrew McCreight [:mccr8]]

Crashes in M1, M4 and Moth, but no symbols on the stack.  I'll try another platform.  Or maybe I'll have to run it locally.

Comment 7

[Andrew McCreight [:mccr8]]

I think the global object NULL assert is okay, because it is only for JSContexts, which I think are special XPConnect structures that can't be wrapped.

Pushing to try again on OSX and Windows hoping that I'll get better stacks:
https://tbpl.mozilla.org/?tree=Try&rev=acd5b28284d1

Comment 8

[Andrew McCreight [:mccr8]]

The M1 crash was cause by nsPresContext's Unlink, which crashes when Unlinked twice, due to an unguarded NS_RELEASE.  Seems unlikely that it would end up in a wrapped native.

Comment 9

[Andrew McCreight [:mccr8]]

Crash stacks are working on try again, so I should move forward on this again.