Closed
Bug 1277075
Opened 8 years ago
Closed 8 years ago
Disable COM's catch-all exception handler
Categories
(Core :: Disability Access APIs, defect)
Tracking
()
RESOLVED
FIXED
mozilla50
People
(Reporter: bugzilla, Assigned: bugzilla)
References
(Blocks 1 open bug)
Details
(Whiteboard: aes+)
Attachments
(2 files)
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!
Updated•8 years ago
|
Whiteboard: aes-win
Updated•8 years ago
|
Whiteboard: aes-win → aes+
Assignee | ||
Comment 1•8 years ago
|
||
Review commit: https://reviewboard.mozilla.org/r/64472/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/64472/
Attachment #8771205 -
Flags: review?(jmathies)
Attachment #8771206 -
Flags: review?(jmathies)
Assignee | ||
Comment 2•8 years ago
|
||
Review commit: https://reviewboard.mozilla.org/r/64474/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/64474/
Comment 3•8 years ago
|
||
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 4•8 years ago
|
||
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+
Assignee | ||
Comment 5•8 years ago
|
||
(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•8 years ago
|
||
(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.
Assignee | ||
Comment 7•8 years ago
|
||
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/
Assignee | ||
Comment 8•8 years ago
|
||
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 | ||
Updated•8 years ago
|
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
Comment 10•8 years ago
|
||
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)
Comment 11•8 years ago
|
||
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
Assignee | ||
Comment 12•8 years ago
|
||
(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)
Assignee | ||
Comment 13•8 years ago
|
||
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/
Assignee | ||
Comment 14•8 years ago
|
||
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•8 years ago
|
||
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
Comment 16•8 years ago
|
||
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
Comment 17•8 years ago
|
||
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)
Assignee | ||
Comment 18•8 years ago
|
||
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)
Assignee | ||
Comment 19•8 years ago
|
||
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/
Assignee | ||
Comment 20•8 years ago
|
||
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•8 years ago
|
||
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
Comment 22•8 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/c2d21e0b1856
https://hg.mozilla.org/mozilla-central/rev/6867c82fc64d
Status: NEW → RESOLVED
Closed: 8 years ago
status-firefox50:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla50
Comment 23•8 years ago
|
||
thanks for this patch, I see a 2% win7 compositor video performance win:
https://treeherder.mozilla.org/perf.html#/alerts?id=2012
Comment 24•8 years ago
|
||
This doesn't work properly on non-English Windows version. See Bug 1297794 for more info and a patch.
You need to log in
before you can comment on or make changes to this bug.
Description
•