1430149 - --enable-accessibility broken for MinGW

Status: RESOLVED FIXED

(Firefox Build System :: General, enhancement)

Description

[Tom Ritter [:tjr]]

This is an upstream problem in widl: https://sourceforge.net/p/mingw-w64/bugs/648/

We'll use this bug to bump the MinGW version once the option is implemented (and to track).

Comment 1

[Jacek Caban]

Attachment: Bug 1430149 - Fix AsyncInvoker.h compilation on mingw. r?aklotz

Bug 1430149 - Don't use RpcExceptionCode in builds not supporting SEH. r?aklotz

This is defined to _exception_code which clang understands, but does not allow outside actual __try/__except.

Bug 1430149 - Fix NtUndoc.h on mingw. r?aklotz

mingw headers declare both ObjectNameInformation and OBJECT_NAME_INFORMATION, which conflicts with local definitions.

Bug 1430149 - widl compatibility fixes. r?aklotz

widl can't handle generating the typelib without explicit importlib() in this case. This should be fixed in widl as well, but using importlib() in this case looks right anyway.

Bug 1430149 - Don't define INITGUID when not needed. r?aklotz

On mingw this causes IIDs to be defined in headers. Those definitions conflict with _i.c files that are included as well. Since we include _i.c anyway, INITGUID is simply not needed.

Comment 2

[Jacek Caban]

Attachment: 0001-Bug-1430149-Fix-AsyncInvoker.h-compilation-on-mingw.-.diff

Comment 3

[Jacek Caban]

Attachment: 0002-Bug-1430149-Don-t-use-RpcExceptionCode-in-builds-not-.diff

Comment 4

[Jacek Caban]

Attachment: 0003-Bug-1430149-Fix-NtUndoc.h-on-mingw.-r-aklotz.diff

Comment 5

[Jacek Caban]

Attachment: 0004-Bug-1430149-widl-compatibility-fixes.-r-aklotz.diff

Comment 6

[Jacek Caban]

Attachment: 0005-Bug-1430149-Fix-AsyncInvoker.h-compilation-on-mingw.-.diff

Comment 7

[Jacek Caban]

I meant to submit this as a series of patches to make review easier, but apparently the new review system can't handle that :/ I attached them here instead.

I implemented -acf option support in widl as well as a few other things that came out to be needed: async_uuid attribute handling, __int32 builtin type, -robust option, call_as fix and MIDL_DEFINE_GUID compatibility fix. You may find most of it in Wine commit history:
https://source.winehq.org/git/wine.git/history/HEAD:/tools/widl
I have a few more fixes (preprocessor fix, fix const of proxy vtbl and async IIDs generation) in works, but you can see it working in this try push (it's not the final version, but close):
https://treeherder.mozilla.org/#/jobs?repo=try&revision=9170668290263f958a2e329e5c82c0e57fd37485
I will submit remaining fixes to Wine soon, so that picking them will be just a matter of bumping mingw version.

In addition to widl fixes, we need changes attached to this bug.

With all those, I got a working build of Firefox with accessibility enabled. I tested that with NVDA. Sadly, it hits bug 1506450 (asserts and crashes content process), but NVDA is able to read Firefox GUI, so most of accessibility support seems okay.

Comment 8

[Jacek Caban]

Those patches don't break clang-cl builds:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=0587f6cd453c39bdebdac10e9eaf7ccfca96ea54

Comment 9

[Jacek Caban]

widl and mingw-w64 changes are upstreamed now, all we need now is version bump and patches from this bug:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=4e5a981b581026376b27fe73c1e8e90cfa96bde4

Comment 10

[Jacek Caban]

Attachment: mingw version bump

Comment 11

[Georg Koppen]

(In reply to Jacek Caban from comment #9)
> widl and mingw-w64 changes are upstreamed now, all we need now is version
> bump and patches from this bug:
> https://treeherder.mozilla.org/#/
> jobs?repo=try&revision=4e5a981b581026376b27fe73c1e8e90cfa96bde4

I can't run that build as it crashes early on my Windows 7 test box but I created a build with the patches backported to ESR 60 and gave it a user to test. They report that the chrome parts are accessible again now but the content window just shows "unknown" instead of web content. It turns out that this is related to e10s (but not to sandboxing) as setting `browser.tabs.remote.autostart` to `false` makes accessibility work for content as well. (see: https://trac.torproject.org/projects/tor/ticket/27503#comment:13 ff.)

So, there seems to be some multi-process bit missing here... I wonder who can help with that. Jacek, any idea? Or maybe Aaron has?

Comment 12

[Tom Ritter [:tjr]]

These 5 patches (plus 1 more for fix --enable-accessibility; plus the resource fix from 1506450) on esr60 created this build: https://treeherder.mozilla.org/#/jobs?repo=try&revision=d3b3ef86ff6e298eb9f75ce5491e33ba3e5600a8

On Windows 10; both the x86 and x64 opt builds work fine for me; although I did not test any accessibility tool.

Comment 13

[Tom Ritter [:tjr]]

Attachment: Bug 1430149 - Add 'typename' to fix a mingw-clang build error r?aklotz

Comment 14

[Tom Ritter [:tjr]]

Comment on attachment 9031026 [details] [diff] [review]
Bug 1430149 - Add 'typename' to fix a mingw-clang build error r?aklotz

Sorry, this patch is already part of Bug 1472806, so we'll just need to backport that to esr60.

Comment 15

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-esr60/rev/aaa9d1093d37

Comment 16

[(No longer employed by Mozilla) Aaron Klotz]

Comment on attachment 9024303 [details] [diff] [review]
0001-Bug-1430149-Fix-AsyncInvoker.h-compilation-on-mingw.-.diff

Review of attachment 9024303 [details] [diff] [review]:
-----------------------------------------------------------------

::: ipc/mscom/AsyncInvoker.h
@@ +351,4 @@
>  #define ASYNC_INVOKE(InvokerObj, SyncMethodName, ...) \
>    InvokerObj.Invoke(&decltype(InvokerObj)::SyncInterfaceT::SyncMethodName, \
>                      &decltype(InvokerObj)::AsyncInterfaceT::Begin_##SyncMethodName, \
> +                    ##__VA_ARGS__)

Can you point me to some information explaining why __VA_ARGS__ needs to be prepended by the ## operator?

Comment 17

[(No longer employed by Mozilla) Aaron Klotz]

Comment on attachment 9024304 [details] [diff] [review]
0002-Bug-1430149-Don-t-use-RpcExceptionCode-in-builds-not-.diff

Review of attachment 9024304 [details] [diff] [review]:
-----------------------------------------------------------------

Is this going to have a material impact on correctness? What happens when Windows RPC raises an exception?

Comment 18

[(No longer employed by Mozilla) Aaron Klotz]

Comment on attachment 9024304 [details] [diff] [review]
0002-Bug-1430149-Don-t-use-RpcExceptionCode-in-builds-not-.diff

Review of attachment 9024304 [details] [diff] [review]:
-----------------------------------------------------------------

Dmajor is already fixing this via bug 1514592, so let's defer to his patch for this one.

Comment 19

[Jacek Caban]

(In reply to Georg Koppen from comment #11)
> So, there seems to be some multi-process bit missing here... I wonder who
> can help with that. Jacek, any idea? Or maybe Aaron has?

From what I learned the code while fixing build, I'd suspect HandlerData marshaling (but I don't know Mozilla code base enough to know the context). It needs more debugging and trunk proven to be the most convenient tree for that so far, so I looked at that first. With bug 1506450 fixed, it's just bug 1515982 that prevents us from using trunk for testing now.

Comment 20

[Jacek Caban]

Thanks for reviews.

(In reply to Aaron Klotz [:aklotz] from comment #16)
> Can you point me to some information explaining why __VA_ARGS__ needs to be
> prepended by the ## operator?


It's a problem when there are no variadic argument is passed to macro, __VA_ARGS__ expands to empty string, but the extra comma before it remains, which is not allowed until c++2a. See:
https://gcc.gnu.org/onlinedocs/cpp/Variadic-Macros.html

BTW, after rebasing, other parts of this patch are not needed. I will attach an updated version.

Comment 21

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/1c7f0326d7b3
Fix NtUndoc.h on mingw. r=aklotz
https://hg.mozilla.org/integration/mozilla-inbound/rev/eb56a37618db
widl compatibility fixes. r=aklotz
https://hg.mozilla.org/integration/mozilla-inbound/rev/680af67bb1e2
Don't define INITGUID when not needed. r=aklotz

Comment 22

[Jacek Caban]

Attachment: Bug 1430149 - Fix AsyncInvoker.h compilation on mingw. r?aklotz

MozReview-Commit-ID: FIswvwMzGdC

Comment 23

[Stefan Hindli [:stefan_hindli]]

https://hg.mozilla.org/mozilla-central/rev/1c7f0326d7b3
https://hg.mozilla.org/mozilla-central/rev/eb56a37618db
https://hg.mozilla.org/mozilla-central/rev/680af67bb1e2

Comment 24

[(No longer employed by Mozilla) Aaron Klotz]

(Also, please mark D11583 as obsolete)

Comment 25

[(No longer employed by Mozilla) Aaron Klotz]

Comment on attachment 9024303 [details] [diff] [review]
0001-Bug-1430149-Fix-AsyncInvoker.h-compilation-on-mingw.-.diff

Review of attachment 9024303 [details] [diff] [review]:
-----------------------------------------------------------------

This is obsoleted by D15325. Cancelling.

Comment 26

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/9e200f5673a3
Fix AsyncInvoker.h compilation on mingw. r=aklotz

Comment 27

[Razvan Maries]

https://hg.mozilla.org/mozilla-central/rev/9e200f5673a3

Comment 28

[Tom Ritter [:tjr]]

Attachment: Bug 1430149 - Fix NtUndoc.h on mingw. r=aklotz (esr60)

Comment 29

[Tom Ritter [:tjr]]

Attachment: Bug 1430149 - Don't define INITGUID when not needed. r=aklotz (esr60)

Comment 30

[Tom Ritter [:tjr]]

Attachment: Bug 1430149 - widl compatibility fixes. r=aklotz (esr60)

Comment 31

[Tom Ritter [:tjr]]

Attachment: Bug 1430149 - Fix AsyncInvoker.h compilation on mingw. r=aklotz (esr60)

Comment 32

[Tom Ritter [:tjr]]

[ESR Uplift Approval Request]

If this is not a sec:{high,crit} bug, please state case for ESR consideration: Needed for the mingwclang build to be able to --enable-accesibility

User impact if declined: Tor will have to carry an additional few patches; and we will have no mingwclang build on TC that compiled with --enable-accessibility (so if it broke we wouldn't notice until Tor did a release.)

Fix Landed on Version: 66.0a1 / 20190108215840

Risk to taking this patch: Low

Why is the change risky/not risky? (and alternatives if risky): All they do it makes the compiler happy.

To be safe I've sent in the following try to confirm it doesn't break any other platforms: https://treeherder.mozilla.org/#/jobs?repo=try&revision=3ac8521d34054b23e2eb1456330c2dd7f93e1fd8

Comment 33

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/2830bd5ae375
Enable Accessibility for MinGW Clang builds. r=froydnj

Comment 34

[Oana Pop-Rus]

https://hg.mozilla.org/mozilla-central/rev/2830bd5ae375

Comment 35

[Cosmin Sabou [:CosminS]]

Backed out for landing with the wrong bug number: https://hg.mozilla.org/integration/mozilla-inbound/rev/b52da9b4235b6574b986418985144a9237bba180
And reland here: https://hg.mozilla.org/integration/mozilla-inbound/rev/b88a370148fb89dc001e1bff825a4a95f7fb1abb

Comment 36

[Julien Cristau [:jcristau]]

Comment on attachment 9036096 [details] [diff] [review]
Bug 1430149 - Fix NtUndoc.h on mingw. r=aklotz (esr60)

mingw build fixes, approved for 60.5esr

Comment 37

[Julien Cristau [:jcristau]]

https://hg.mozilla.org/releases/mozilla-esr60/rev/cbb386941cf7
https://hg.mozilla.org/releases/mozilla-esr60/rev/35d886531689
https://hg.mozilla.org/releases/mozilla-esr60/rev/fe547fe73bba
https://hg.mozilla.org/releases/mozilla-esr60/rev/3e18d1aa6c31