1684043 - When throwing an exception or rejecting a promise in system or about: principal compartments without a clear message, dump the JS stack and MOZ_CRASH if we're running tests

Status: NEW

(Core :: JavaScript Engine, task, P3)

Description

[:Gijs (he/him)]

Intermittent test failures with uncaught errors or rejections that have no meaningful stack or label are unfortunately not unheard of, and they are an absolute nightmare to debug / fix (cf. bug 1620121, bug 1621861, bug 1641039, bug 1602266, bug 1677300). They are normally caused by either frontend or platform code rejecting or throwing an exception without providing any further information.

I'm hoping that with a relatively minor change in the JS engine, we can get more information about these failures and (hopefully) eradicate them from the codebase and prevent them from being reintroduced.

For rejections/exceptions with no stack/message that occur in system (or about: or extension) principal compartments / code, while xpc::IsInAutomation() is true, we should

1. dump the JS stack, if any
2. MOZ_CRASH with a reasonable message

I'm hoping this is not already so frequent as to be completely infeasible right now (though I guess we may need to disable a test or two).

Comment 1

[Jason Orendorff [:jorendorff]]

For ordinary exceptions, you can try adding a check here: https://searchfox.org/mozilla-central/source/xpcom/base/CycleCollectedJSRuntime.cpp#1878

For rejections I'm not sure. This might be a great idea, but there also are many possible complications, like rethrowing, OOM, async errors, and the debugger. Try it out.

Comment 2

[Jason Orendorff [:jorendorff]]

It looks like 4 out of 5 of those bugs contain the string "(unknown module)". Do you think that might be coming from here? If so, I think that means we are dealing with an Error object, at least originally. But then something is catching that error, and rethrowing it as a regular Object object? Maybe going through some place like here?

If I had to choose, I'd rather add your crash so we get more information in the future. But maybe this problem would be obvious to someone familiar with this code... and maybe we can do both.

Comment 3

[:Gijs (he/him)]

Oops, didn't mean to clear needinfo.