Closed Bug 708480 Opened 13 years ago Closed 7 years ago

improved testing for double unlinks

Categories

(Core :: XPCOM, defect)

defect
Not set
normal

Tracking

()

RESOLVED INCOMPLETE

People

(Reporter: mccr8, Unassigned)

References

(Depends on 1 open bug)

Details

Attachments

(1 file)

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.
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 :)
Attached patch double unlink!Splinter Review
(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
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.
Disabled assertion and push to try, just for linux this time. https://tbpl.mozilla.org/?tree=Try&rev=8fbb8ef60562
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.
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
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.
Depends on: 715684
Crash stacks are working on try again, so I should move forward on this again.
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → INCOMPLETE
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: