Disable COM's catch-all exception handler

RESOLVED FIXED in Firefox 50

Status

()

defect
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: aklotz, Assigned: aklotz)

Tracking

(Blocks 1 bug)

Trunk
mozilla50
Unspecified
Windows
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox49 affected, firefox50 fixed)

Details

(Whiteboard: aes+)

Attachments

(2 attachments)

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!
Whiteboard: aes-win
Whiteboard: aes-win → aes+
Depends on: 1287002
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.
Attachment #8771206 - Flags: review?(jmathies) → review+
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?
Attachment #8771205 - Flags: review?(jmathies) → review+
(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...
(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 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 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/
Assignee: nobody → aklotz
Pushed by aklotz@mozilla.com:
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
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.
Flags: needinfo?(aklotz)
Backout by ihsiao@mozilla.com:
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
(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.
Flags: needinfo?(aklotz)
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 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/
Pushed by aklotz@mozilla.com:
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
Backout by ihsiao@mozilla.com:
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
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.
Flags: needinfo?(aklotz)
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.
Flags: needinfo?(aklotz)
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 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/
Pushed by aklotz@mozilla.com:
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
https://hg.mozilla.org/mozilla-central/rev/c2d21e0b1856
https://hg.mozilla.org/mozilla-central/rev/6867c82fc64d
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla50
thanks for this patch, I see a 2% win7 compositor video performance win:
https://treeherder.mozilla.org/perf.html#/alerts?id=2012
This doesn't work properly on non-English Windows version. See Bug 1297794 for more info and a patch.
Blocks: 1323055
Depends on: 1330496
You need to log in before you can comment on or make changes to this bug.