1277075 - Disable COM's catch-all exception handler

Status: RESOLVED FIXED

(Core :: Disability Access APIs, defect)

Description

[(No longer employed by Mozilla) Aaron Klotz]

By default, when COM invokes a method from a stub (ie, the client is remote), it includes a catch-all exception handler that ends up suppressing important things like assertions and crashes, none of which end up in the crash reporter or are visible without an attached debugger.

This policy is configurable via the IGlobalOptions interface outlined at:
https://msdn.microsoft.com/en-us/library/windows/desktop/aa344211(v=vs.85).aspx

We should disable that thing so that we get proper information. I definitely want to disable it in content, though I think we could make a pretty good case for disabling it everywhere. IMHO catch-all exception handlers are pure evil!

Comment 1

[(No longer employed by Mozilla) Aaron Klotz]

Attachment: Bug 1277075: Add MainThreadRuntime class to mscom glue, enabling safe initialization of COM security and exception handlers;

Review commit: https://reviewboard.mozilla.org/r/64472/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/64472/

Comment 2

[(No longer employed by Mozilla) Aaron Klotz]

Attachment: Bug 1277075: Modify parent and child process startup to use mscom::MainThreadRuntime;

Review commit: https://reviewboard.mozilla.org/r/64474/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/64474/

Comment 3

[Jim Mathies [:jimm]]

Comment on attachment 8771206 [details]
Bug 1277075: Modify parent and child process startup to use mscom::MainThreadRuntime;

https://reviewboard.mozilla.org/r/64474/#review61606

::: dom/ipc/ContentProcess.h:46
(Diff revision 1)
>  
>  private:
>    ContentChild mContent;
>    mozilla::ipc::ScopedXREEmbed mXREEmbed;
> +#if defined(XP_WIN)
> +  mozilla::mscom::MainThreadRuntime mCOMRuntime;

lets add a comment to each of these noting they initialize COM.

Comment 4

[Jim Mathies [:jimm]]

Comment on attachment 8771205 [details]
Bug 1277075: Add MainThreadRuntime class to mscom glue, enabling safe initialization of COM security and exception handlers;

https://reviewboard.mozilla.org/r/64472/#review61612

::: ipc/mscom/MainThreadRuntime.cpp:118
(Diff revision 1)
> +  if (!::InitializeSecurityDescriptor(&sd, SECURITY_DESCRIPTOR_REVISION)) {
> +    return HRESULT_FROM_WIN32(::GetLastError());
> +  }
> +
> +  // Grant access to SYSTEM, Administrators, and the user.
> +  EXPLICIT_ACCESS entries[] = {

Curious at how you arrived at this list. From the CoInitializeSecurity api we resrict access to our objects to this list. Doesn't this break 3rd party a11y consumers?

Comment 5

[(No longer employed by Mozilla) Aaron Klotz]

(In reply to Jim Mathies [:jimm] from comment #4)
> Curious at how you arrived at this list. From the CoInitializeSecurity api
> we resrict access to our objects to this list. Doesn't this break 3rd party
> a11y consumers?

Basically I just thought about what the minimum requirements were. Certainly we want to allow SYSTEM and Administrators.

I also added the SID of the current user -- that is working fine with a11y clients (at least what I've seen so far) -- my assumption in using that SID is that any a11y client is also going to be running under the same SID as the current user of the Firefox processes. I had spent some time digging around for the default SD and the locations appear to be multiple registry locations with a resolution algorithm that is just as undocumented, though I suppose we could try to replicate it.

My main concern is that this single API call is overloaded for both incoming *and outgoing* traffic. Passing nullptr for the SD is the worst thing to do, while passing "Everyone" doesn't seem to be much better...

Comment 6

[Jim Mathies [:jimm]]

(In reply to Aaron Klotz [:aklotz] from comment #5)
> (In reply to Jim Mathies [:jimm] from comment #4)
> > Curious at how you arrived at this list. From the CoInitializeSecurity api
> > we resrict access to our objects to this list. Doesn't this break 3rd party
> > a11y consumers?
> 
> Basically I just thought about what the minimum requirements were. Certainly
> we want to allow SYSTEM and Administrators.
> 
> I also added the SID of the current user -- that is working fine with a11y
> clients (at least what I've seen so far) -- my assumption in using that SID
> is that any a11y client is also going to be running under the same SID as
> the current user of the Firefox processes.

Ah, yes of course. makes sense.

Comment 7

[(No longer employed by Mozilla) Aaron Klotz]

Comment on attachment 8771205 [details]
Bug 1277075: Add MainThreadRuntime class to mscom glue, enabling safe initialization of COM security and exception handlers;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/64472/diff/1-2/

Comment 8

[(No longer employed by Mozilla) Aaron Klotz]

Comment on attachment 8771206 [details]
Bug 1277075: Modify parent and child process startup to use mscom::MainThreadRuntime;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/64474/diff/1-2/

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/26fe03ac12be
Add MainThreadRuntime class to mscom glue, enabling safe initialization of COM security and exception handlers; r=jimm
https://hg.mozilla.org/integration/autoland/rev/0a12376c8ba5
Modify parent and child process startup to use mscom::MainThreadRuntime; r=jimm

Comment 10

[Iris Hsiao [:ihsiao]]

I read your mail, but am sorry had to back out because build bustage, e.g. https://treeherder.mozilla.org/logviewer.html#?job_id=677693&repo=autoland#L13788
If that's the expected result, please let me know.

Comment 11

[Pulsebot]

Backout by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/1efa8a2db0ec
Backed out changeset 26fe03ac12be for Windows build bustage
https://hg.mozilla.org/integration/autoland/rev/b7842f149fdc
Backed out changeset 0a12376c8ba5

Comment 12

[(No longer employed by Mozilla) Aaron Klotz]

(In reply to Iris Hsiao [:ihsiao] from comment #10)
> I read your mail, but am sorry had to back out because build bustage, e.g.
> https://treeherder.mozilla.org/logviewer.
> html#?job_id=677693&repo=autoland#L13788
> If that's the expected result, please let me know.

Huh, looks like unified source concatenated the files in a different order on my machine than on the build machines or something. An easy fix which apparently I need to land tomorrow since apparently MozReview is down.

Comment 13

[(No longer employed by Mozilla) Aaron Klotz]

Comment on attachment 8771205 [details]
Bug 1277075: Add MainThreadRuntime class to mscom glue, enabling safe initialization of COM security and exception handlers;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/64472/diff/2-3/

Comment 14

[(No longer employed by Mozilla) Aaron Klotz]

Comment on attachment 8771206 [details]
Bug 1277075: Modify parent and child process startup to use mscom::MainThreadRuntime;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/64474/diff/2-3/

Comment 15

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/88e5ef869df6
Add MainThreadRuntime class to mscom glue, enabling safe initialization of COM security and exception handlers; r=jimm
https://hg.mozilla.org/integration/autoland/rev/8feb113aaba7
Modify parent and child process startup to use mscom::MainThreadRuntime; r=jimm

Comment 16

[Pulsebot]

Backout by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/b4ddb57e3e2c
Backed out changeset 88e5ef869df6 for assertion failures
https://hg.mozilla.org/integration/autoland/rev/df92a916d7ad
Backed out changeset 8feb113aaba7

Comment 17

[Iris Hsiao [:ihsiao]]

Hi Aaron, sorry had to back out again, it caused assertion failures and broke tests, such as https://treeherder.mozilla.org/logviewer.html#?job_id=746301&repo=autoland#L4000
Please look at the issue, and push it again after a try run? Thanks.

Comment 18

[(No longer employed by Mozilla) Aaron Klotz]

Windows XP nonsense. Looks like it returns an IGlobalOptions object even though MSDN says it's unsupported. But calling anything on that object fails.

Also, it behaves differently depending on whether or not the OS is fully patched. Fun!

I'm just going to disable that code path on XP; it doesn't really give those users anything anyway.

Comment 19

[(No longer employed by Mozilla) Aaron Klotz]

Comment on attachment 8771205 [details]
Bug 1277075: Add MainThreadRuntime class to mscom glue, enabling safe initialization of COM security and exception handlers;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/64472/diff/3-4/

Comment 20

[(No longer employed by Mozilla) Aaron Klotz]

Comment on attachment 8771206 [details]
Bug 1277075: Modify parent and child process startup to use mscom::MainThreadRuntime;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/64474/diff/3-4/

Comment 21

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/c2d21e0b1856
Add MainThreadRuntime class to mscom glue, enabling safe initialization of COM security and exception handlers; r=jimm
https://hg.mozilla.org/integration/autoland/rev/6867c82fc64d
Modify parent and child process startup to use mscom::MainThreadRuntime; r=jimm

Comment 22

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/c2d21e0b1856
https://hg.mozilla.org/mozilla-central/rev/6867c82fc64d

Comment 23

[Joel Maher ( :jmaher ) (UTC -8)]

thanks for this patch, I see a 2% win7 compositor video performance win:
https://treeherder.mozilla.org/perf.html#/alerts?id=2012

Comment 24

[Kacper Michajłow [:kasper93]]

This doesn't work properly on non-English Windows version. See Bug 1297794 for more info and a patch.