I came across this while looking at crash stacks. It looks like there is an error in NSS somewhere, then it calls nsContentUtils::LogSimpleConsoleError() to report the error. We then end up in nsConsoleService::LogMessageWithMode(). It looks like we've exceeded the size of the console buffer, so we pull out an error into |retiredMessage|. Then we call NS_RELEASE on this message, but unfortunately it is one of these new nsScriptErrorWithStack things, which is cycle collected, and we're off on some random thread, so we crash when we call release and try to find a cycle collector on the current thread. Maybe some kind of release-to-main-thread thing would work here? https://crash-stats.mozilla.com/report/index/0191799f-f11f-468f-9771-4544d2150716 https://crash-stats.mozilla.com/report/index/4e742187-73ea-49fc-b38d-725ec2150717 https://crash-stats.mozilla.com/report/index/11fe052f-d318-48c5-9d6d-59a252150716 https://crash-stats.mozilla.com/report/index/e36637b0-93cd-41d2-81ca-aec2f2150716 https://crash-stats.mozilla.com/report/index/779a7203-d870-49cc-9b5e-505e32150717

Sotaro, I remember you had something that would automatically release something that was off-main-thread on the main thread (I seem to recall it was for gfx), would you mind telling us if we could reuse it here?

NS_ReleaseOnMainThread?

(In reply to Nathan Froyd [:froydnj][:nfroyd] from comment #2) > NS_ReleaseOnMainThread? Yes! But I don't know how I could distinguish nsScriptErrorWithStack instances here: http://mxr.mozilla.org/mozilla-central/source/xpcom/base/nsConsoleService.cpp#323 We have nsScriptError(thread-safe) and nsScriptErrorWithStack(main-thread, garbage collected) instances. And possibly other kind of objects that inherits from nsIConsoleMessage. We could have QI into nsIConsoleMessageWithStack, but as stated in bug 814497 command 55, I wasn't able to implement such custom interface. Also it sounds bad regarding performances to do a QI?

(In reply to Alexandre Poirot [:ochameau] from comment #3) > We have nsScriptError(thread-safe) and nsScriptErrorWithStack(main-thread, > garbage collected) instances. And possibly other kind of objects that > inherits from nsIConsoleMessage. Maybe add a C++-only method ReleaseFromAnyThread() to nsIConsoleMessage? > We could have QI into nsIConsoleMessageWithStack, but as stated in bug > 814497 command 55, I wasn't able to implement such custom interface. The assertion you were seeing means that you didn't clear the JS object in the unlink method. That is bad because it could potentially lead to a dangling pointer to a JS object. > Also it sounds bad regarding performances to do a QI? It isn't the fastest thing around, but it is probably not too bad compared to everything else that is going on in that method already, and I'd think it is much cheaper than doing the ReleaseToMainThread.

Can't we just use NS_ReleaseOnMainThread() here? gfx things do not work here, since classes are expected to be derived from AtomicRefCountedWithFinalize.

In terms of security implications, the actual stack here is benign: we will always hit a null deref crash. However, both the main thread and worker threads have a cycle collector. If there are a bunch of errors from the main thread, then you try to dispatch a console error message on a worker using LogSimpleConsoleError, then you'd end up with a main thread object being manipulated by the worker cycle collector, which could be quite bad. I assume we only generate these error messages with stacks when the browser console is open, due to perf concerns, so that mitigates this.

(In reply to Sotaro Ikeda [:sotaro] from comment #5) > Can't we just use NS_ReleaseOnMainThread() here? gfx things do not work > here, since classes are expected to be derived from AtomicRefCountedWithFinalize. Yes, I think that would be okay. It would be doing some more work than is really needed in the common case, but I doubt this code is super perf sensitive.

(In reply to Andrew McCreight [:mccr8] from comment #6) > I assume we only generate these error messages with stacks when the browser > console is open, due to perf concerns, so that mitigates this. I think we do it no matter what. We always pass the stack objects along, but only start processing/accessing it when a console is opened. Note that it isn't just for the Browser console. it is especially for regular content where we were missing stack for JS exception. (In reply to Andrew McCreight [:mccr8] from comment #7) > (In reply to Sotaro Ikeda [:sotaro] from comment #5) > > Can't we just use NS_ReleaseOnMainThread() here? gfx things do not work > > here, since classes are expected to be derived from AtomicRefCountedWithFinalize. > > Yes, I think that would be okay. Hum. But if we just replace NS_Release with NS_ReleaseOnMainThread wouldn't it be broken on the opposite direction? Where objects on the other threads are not going to be released correctly? (In reply to Andrew McCreight [:mccr8] from comment #4) > (In reply to Alexandre Poirot [:ochameau] from comment #3) > > We have nsScriptError(thread-safe) and nsScriptErrorWithStack(main-thread, > > garbage collected) instances. And possibly other kind of objects that > > inherits from nsIConsoleMessage. > > Maybe add a C++-only method ReleaseFromAnyThread() to nsIConsoleMessage? That would call NS_RELEASE(this) and be overloaded by nsScriptErrorWithStack to call NS_ReleaseOnMainThread? > > > We could have QI into nsIConsoleMessageWithStack, but as stated in bug > > 814497 command 55, I wasn't able to implement such custom interface. > > The assertion you were seeing means that you didn't clear the JS object in > the unlink method. That is bad because it could potentially lead to a > dangling pointer to a JS object. Sorry, I meant bug 814497 comment 56, I fixed the assertion of comment 55.

(In reply to Alexandre Poirot [:ochameau] from comment #8) > Hum. But if we just replace NS_Release with NS_ReleaseOnMainThread wouldn't > it be broken on the opposite direction? Where objects on the other threads > are not going to be released correctly? For threadsafe objects, I think it should be okay. But yeah I guess NS_ReleaseOnMainThread won't really work, because I guess the console message could have been created on the cycle collector thread, and we'd need to release it there. I'm not sure how to deal with that, without each nsScriptErrorWithStack somehow carrying around a thread pointer object. > That would call NS_RELEASE(this) and be overloaded by nsScriptErrorWithStack > to call NS_ReleaseOnMainThread? That was my idea, but as you said this won't actually work if the error with stack isn't on the main thread. > > > We could have QI into nsIConsoleMessageWithStack, but as stated in bug > > > 814497 command 55, I wasn't able to implement such custom interface. > > > > The assertion you were seeing means that you didn't clear the JS object in > > the unlink method. That is bad because it could potentially lead to a > > dangling pointer to a JS object. > > Sorry, I meant bug 814497 comment 56, I fixed the assertion of comment 55. Oh, okay. For that, I think you need to use NS_DECL_CYCLE_COLLECTION_CLASS_AMBIGUOUS. If you want to address that in a separate open bug I can help you with that. I think a ReleaseOnOwnThread() kind of method is a better approach, but others may disagree.

Sorry I missed this in review :-(

billm suggested that we're probably in trouble anyways if one of these things is being created on a worker, so we should probably just assert or even release assert that nsScriptErrorWithStack is only created on the main thread, and then we can do release to main thread.

Alexandre, should it be okay for now if we restrict nsScriptErrorWithStack to being created on the main thread, or is this expected to work for workers right now?

Assuming the devtools code runs on the main thread JSRuntime, there's no way it would be able to access worker stuff anyway.

Ok. Yeah, it looks like this object is only created in some XPConnect method xpc::ErrorReport::LogToConsoleWithStack, so hopefully it will be okay.

I think this can be moderate given that these error messages only seem to be made on the main thread. I added an assert in my try push to double-check that assumption.

Well, except that the problem is when any kind of console message is generated on a worker thread. I'm not confident enough that that doesn't happen, so I'll just leave it at high. This is trunk-only so it doesn't matter too much either way. The real bug here is in nsConsoleService. I'll try to look through other places that use nsIConsoleMessage.

Yes. Worker isn't targeted by this patch. We would be happy to start by seeing just main thread stacks!

Also, add release asserts that the other methods that addref or release console messages are only run on the main thread. Finally, add an assert that nsScriptErrorWithStack is only created on the main thread. try run: https://treeherder.mozilla.org/#/jobs?repo=try&revision=3b7a51e5cc3d

Comment on attachment 8636342 [details] [diff] [review] nsConsoleService::LogMessageWithMode() should release the retired message on the main thread. Review of attachment 8636342 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/xpconnect/src/nsScriptErrorWithStack.cpp @@ +42,5 @@ > nsScriptErrorWithStack::nsScriptErrorWithStack(JS::HandleObject aStack) > : nsScriptError(), > mStack(aStack) > { > + MOZ_ASSERT(NS_IsMainThread(), "You can't use this class on workers."); Not MOZ_RELEASE_ASSERT?

(In reply to Nathan Froyd [:froydnj][:nfroyd] from comment #19) > Not MOZ_RELEASE_ASSERT? This is a new class that isn't used in many places, so I feel like I have a good understanding of how it is called, and any further changes will probably have tests, so I think that's good enough. Thanks for the fast review! https://hg.mozilla.org/integration/mozilla-inbound/rev/49ae0961591e

From my perusal of stacks on crash reports, this is the number 2 crash on Nightly 42 ("about to become #1 on Nightly" according to Tracy).