1185144 - nsContentUtils::LogSimpleConsoleError() releases nsScriptErrorWithStack off the main thread

Status: RESOLVED FIXED

(Core :: XPCOM, defect)

Description

[Andrew McCreight [:mccr8]]

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

Comment 1

[Paul Adenot (:padenot)]

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?

Comment 2

[Nathan Froyd [:froydnj]]

NS_ReleaseOnMainThread?

Comment 3

[Alexandre Poirot [:ochameau]]

(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?

Comment 4

[Andrew McCreight [:mccr8]]

(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.

Comment 5

[Sotaro Ikeda [:sotaro]]

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

Comment 6

[Andrew McCreight [:mccr8]]

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.

Comment 7

[Andrew McCreight [:mccr8]]

(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.

Comment 8

[Alexandre Poirot [:ochameau]]

(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.

Comment 9

[Andrew McCreight [:mccr8]]

(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.

Comment 10

[Bobby Holley (:bholley)]

Sorry I missed this in review :-(

Comment 11

[Andrew McCreight [:mccr8]]

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.

Comment 12

[Andrew McCreight [:mccr8]]

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?

Comment 13

[Bobby Holley (:bholley)]

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

Comment 14

[Andrew McCreight [:mccr8]]

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

Comment 15

[Andrew McCreight [:mccr8]]

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.

Comment 16

[Andrew McCreight [:mccr8]]

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.

Comment 17

[Alexandre Poirot [:ochameau]]

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

Comment 18

[Andrew McCreight [:mccr8]]

Attachment: nsConsoleService::LogMessageWithMode() should release the retired message on the main thread.

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 19

[Nathan Froyd [:froydnj]]

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?

Comment 20

[Andrew McCreight [:mccr8]]

(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

Comment 21

[Andrew McCreight [:mccr8]]

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).

Comment 22

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/49ae0961591e