--enable-accessibility broken for MinGW
Categories
(Firefox Build System :: General, enhancement)
Tracking
(firefox-esr60 fixed, firefox59 wontfix, firefox64 wontfix, firefox65 wontfix, firefox66 fixed)
People
(Reporter: tjr, Assigned: tjr)
References
(Blocks 1 open bug)
Details
(Whiteboard: [tor])
Attachments
(10 files, 3 obsolete files)
2.59 KB,
patch
|
Details | Diff | Splinter Review | |
1.88 KB,
patch
|
bugzilla
:
review-
|
Details | Diff | Splinter Review |
1.56 KB,
patch
|
bugzilla
:
review+
|
Details | Diff | Splinter Review |
2.32 KB,
patch
|
bugzilla
:
review+
|
Details | Diff | Splinter Review |
2.87 KB,
patch
|
bugzilla
:
review+
|
Details | Diff | Splinter Review |
47 bytes,
text/x-phabricator-request
|
Details | Review | |
1.11 KB,
patch
|
jcristau
:
approval-mozilla-esr60+
|
Details | Diff | Splinter Review |
2.00 KB,
patch
|
jcristau
:
approval-mozilla-esr60+
|
Details | Diff | Splinter Review |
2.52 KB,
patch
|
jcristau
:
approval-mozilla-esr60+
|
Details | Diff | Splinter Review |
1.78 KB,
patch
|
jcristau
:
approval-mozilla-esr60+
|
Details | Diff | Splinter Review |
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).
Updated•6 years ago
|
Comment 1•6 years ago
|
||
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•6 years ago
|
||
Comment 3•6 years ago
|
||
Comment 4•6 years ago
|
||
Comment 5•6 years ago
|
||
Comment 6•6 years ago
|
||
Comment 7•6 years ago
|
||
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•6 years ago
|
||
Those patches don't break clang-cl builds: https://treeherder.mozilla.org/#/jobs?repo=try&revision=0587f6cd453c39bdebdac10e9eaf7ccfca96ea54
Comment 9•6 years ago
|
||
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
Updated•6 years ago
|
Updated•6 years ago
|
Updated•6 years ago
|
Updated•6 years ago
|
Updated•6 years ago
|
Comment 10•6 years ago
|
||
Updated•6 years ago
|
Comment 11•5 years ago
|
||
(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?
Assignee | ||
Comment 12•5 years ago
|
||
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.
Assignee | ||
Comment 13•5 years ago
|
||
Assignee | ||
Updated•5 years ago
|
Assignee | ||
Comment 14•5 years ago
|
||
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•5 years ago
|
||
bugherder uplift |
https://hg.mozilla.org/releases/mozilla-esr60/rev/aaa9d1093d37
Updated•5 years ago
|
Updated•5 years ago
|
Updated•5 years ago
|
Comment 16•5 years ago
|
||
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•5 years ago
|
||
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•5 years ago
|
||
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•5 years ago
|
||
(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•5 years ago
|
||
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.
Updated•5 years ago
|
Comment 21•5 years ago
|
||
Pushed by jacek@codeweavers.com: 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•5 years ago
|
||
MozReview-Commit-ID: FIswvwMzGdC
Comment 23•5 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/1c7f0326d7b3 https://hg.mozilla.org/mozilla-central/rev/eb56a37618db https://hg.mozilla.org/mozilla-central/rev/680af67bb1e2
Updated•5 years ago
|
Comment 24•5 years ago
|
||
(Also, please mark D11583 as obsolete)
Comment 25•5 years ago
|
||
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•5 years ago
|
||
Pushed by ccoroiu@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/9e200f5673a3 Fix AsyncInvoker.h compilation on mingw. r=aklotz
Comment 27•5 years ago
|
||
bugherder |
Assignee | ||
Comment 28•5 years ago
|
||
Assignee | ||
Comment 29•5 years ago
|
||
Assignee | ||
Comment 30•5 years ago
|
||
Assignee | ||
Comment 31•5 years ago
|
||
Assignee | ||
Comment 32•5 years ago
|
||
[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•5 years ago
|
||
Pushed by cbrindusan@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/2830bd5ae375 Enable Accessibility for MinGW Clang builds. r=froydnj
Comment 34•5 years ago
|
||
bugherder |
Comment 35•5 years ago
|
||
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
Updated•5 years ago
|
Comment 36•5 years ago
|
||
Comment on attachment 9036096 [details] [diff] [review]
Bug 1430149 - Fix NtUndoc.h on mingw. r=aklotz (esr60)
mingw build fixes, approved for 60.5esr
Updated•5 years ago
|
Updated•5 years ago
|
Updated•5 years ago
|
Comment 37•5 years ago
|
||
bugherder uplift |
Description
•