Bugzilla

(In reply to Toshihito Kikuchi [:toshi] from comment #1)
> (In reply to Aaron Klotz [:aklotz] from comment #0)
> > (Note that I have no plans on removing the assertion in question, as it is a diagnostic assert to catch more serious problems, and is only affecting a small number of installations.)
> 
> As I mentioned in [this comment](https://bugzilla.mozilla.org/show_bug.cgi?id=1716743#c7), our crash reporter does not catch this assert in the main process, and it might prevent Firefox from being auto-updated.

I held an "MSCOM brain dump" meeting on August 9. We discussed this bug among other issues, and I wanted to leave some notes here for posterity:

By default, for compatibility reasons, the COM runtime wraps itself in a catch-all SEH handler that suppresses those exceptions. This can manifest itself in many ways. One of the most significant is when our process passes in an interface pointer to be used as a callback and the COM runtime later invokes our interface. If our implementation of that interface crashes, that crash will be suppressed by COM. At this risk of pointing out the obvious, this leaves the process in an **undefined state**. Any crashes within the COM runtime itself will also be suppressed by the catch-all SEH handler, also leaving the process in an undefined state.

Fortunately COM allows us to override this default behaviour to ensure that exceptions propagate as expected. This is done via the `IGlobalOptions` interface. Our use of `mscom::ProcessRuntime` to start up COM ensures that this happens. **HOWEVER:** `IGlobalOptions` **must** be set before any interface (de)marshaling is performed by the COM runtime. Given the various hooks and other opportunities for marshaling to occur transitively, we must therefore set this option before creating any windows, pumping any messages, or invoking any other APIs that might trigger COM under the hood.

Further complicating the situation is that any calls to `CoInitializeSecurity` also have the same restriction, with the added dependency that it should only be called once per process, and it must be called before instantiating `CLSID_GlobalOptions`. Since `CoInitializeSecurity` and `CLSID_GlobalOptions` both have the same restrictions re: running prior to any marshaling, if `CoInitializeSecurity` fails with `RPC_E_TOO_LATE`, then any subsequent attempt to do anything with `IGlobalOptions` is also likely to fail.

(If you read the `ProcessRuntime` source code, you see that we still try to instantiate `CLSID_GlobalOptions` anyway, but this is more of a last-ditch, "I don't expect this to work but I'm going to try anyway out of desparation" maneuver. As I understand things, if it is too late for `CoInitializeSecurity`, then it is too late for `CLSID_GlobalOptions` as well.)

Analyzing these dependencies, this means that we end up with the following order of operations:
1. `CoInitializeSecurity`;
2. Instantiate `CLSID_GlobalOptions` and disable the catch-all SEH handler;
3. Create UI, pump messages, marshal COM interfaces, etc...

Note that these warnings are not purely theoretical: when I first enabled `ProcessRuntime` during startup, crashes that were previously suppressed by COM became revealed in at least two locations! It is therefore critically important that we ensure that our developers are not inadvertently breaking `ProcessRuntime` when making changes to the Firefox startup sequence. Early and immediate detection is key, so I strongly believe that we detect such conditions at build time and have the sheriffs back out the culprit.

Having said that, I do acknowledge Toshi's concerns in comment 1. Rather than completely moving the diagnostic assert, I would strongly suggest that we make it "smarter" instead. Whether that means only asserting when `MOZ_AUTOMATION` is set, or we only assert if we're not running in updater mode, or we set a flag that avoids asserting until later in the startup sequence, once the crash reporter is up... I'll leave that to my successors to decide what to do. I think if we're careful about how we do this, we can still catch self-induced errors without harming the user experience, even if it's only on pre-release channels.

(In reply to Toshihito Kikuchi [:toshi] from comment #1)
> (In reply to Aaron Klotz [:aklotz] from comment #0)
> > (Note that I have no plans on removing the assertion in question, as it is a diagnostic assert to catch more serious problems, and is only affecting a small number of installations.)
> 
> As I mentioned in [this comment](https://bugzilla.mozilla.org/show_bug.cgi?id=1716743#c7), our crash reporter does not catch this assert in the main process, and it might prevent Firefox from being auto-updated.

I held an "MSCOM brain dump" meeting on August 9. We discussed this bug among other issues, and I wanted to leave some notes here for posterity:

By default, for compatibility reasons, the COM runtime wraps itself in a catch-all SEH handler that suppresses those exceptions. This can manifest itself in many ways. One of the most significant is when our process passes in an interface pointer to be used as a callback and the COM runtime later invokes our interface. If our implementation of that interface crashes, that crash will be suppressed by COM. At this risk of pointing out the obvious, this leaves the process in an **undefined state**. Any crashes within the COM runtime itself will also be suppressed by the catch-all SEH handler, also leaving the process in an undefined state.

Fortunately COM allows us to override this default behaviour to ensure that exceptions propagate as expected. This is done via the `IGlobalOptions` interface. Our use of `mscom::ProcessRuntime` to start up COM ensures that this happens. **HOWEVER:** `IGlobalOptions` **must** be set before any interface (de)marshaling is performed by the COM runtime. Given the various hooks and other opportunities for marshaling to occur transitively, we must therefore set this option before creating any windows, pumping any messages, or invoking any other APIs that might trigger COM under the hood.

Further complicating the situation is that any calls to `CoInitializeSecurity` also have the same restriction, with the added dependency that it should only be called once per process, and it must be called before instantiating `CLSID_GlobalOptions`. Since `CoInitializeSecurity` and `CLSID_GlobalOptions` both have the same restrictions re: running prior to any marshaling, if `CoInitializeSecurity` fails with `RPC_E_TOO_LATE`, then any subsequent attempt to do anything with `IGlobalOptions` is also likely to fail.

(If you read the `ProcessRuntime` source code, you see that we still try to instantiate `CLSID_GlobalOptions` anyway, but this is more of a last-ditch, "I don't expect this to work but I'm going to try anyway out of desparation" maneuver. As I understand things, if it is too late for `CoInitializeSecurity`, then it is too late for `CLSID_GlobalOptions` as well.)

Analyzing these dependencies, this means that we end up with the following order of operations:
1. `CoInitializeSecurity`;
2. Instantiate `CLSID_GlobalOptions` and disable the catch-all SEH handler;
3. Create UI, pump messages, marshal COM interfaces, etc...

Note that these warnings are not purely theoretical: when I first enabled `ProcessRuntime` during startup, crashes that were previously suppressed by COM became revealed in at least two locations! It is therefore critically important that we ensure that our developers are not inadvertently breaking `ProcessRuntime` when making changes to the Firefox startup sequence. Early and immediate detection is key, so I strongly believe that we detect such conditions at build time and have the sheriffs back out the culprit.

Having said that, I do acknowledge Toshi's concerns in comment 1. Rather than completely removing the diagnostic assert, I would strongly suggest that we make it "smarter" instead. Whether that means only asserting when `MOZ_AUTOMATION` is set, or we only assert if we're not running in updater mode, or we set a flag that avoids asserting until later in the startup sequence, once the crash reporter is up... I'll leave that to my successors to decide what to do. I think if we're careful about how we do this, we can still catch self-induced errors without harming the user experience, even if it's only on pre-release channels.