1393903 - webrtc uses __try macros (which don't exist in MinGW)

Status: RESOLVED INCOMPLETE

(Core :: WebRTC, defect, P5)

Description

[Tom Ritter [:tjr]]

In at least one place:

> 0:11.65 /home/tom/Documents/moz/mingw-work/just-build-7/media/webrtc/trunk/webrtc/base/platform_thread.cc: In function ‘void rtc::SetCurrentThreadName(const char*)’:
> 0:11.65 /home/tom/Documents/moz/mingw-work/just-build-7/media/webrtc/trunk/webrtc/base/platform_thread.cc:93:40: error: ‘__except’ was not declared in this scope
> 0:11.65    } __except (EXCEPTION_EXECUTE_HANDLER) {

Comment 1

[Bulk Bug Changes for mreavy's org]

Mass change P4->P5 to align with new Mozilla triage process.

Comment 2

[Tom Ritter [:tjr]]

So for background, __try is a MSVC construct that clang-cl implements, but gcc doesn't understand. It caused a lot of problems in the sandbox, but we were able to work around them by basically commenting them out. Things worked. To get webrtc compiling with MinGW, we need to address these. I'm hoping we can ignore them similarly.



I found three instances of __try in media/webrtc:

http://searchfox.org/mozilla-central/rev/40e8eb46609dcb8780764774ec550afff1eed3a5/media/webrtc/trunk/webrtc/base/proxydetect.cc#602
This looks like a safety net to handle crashing plugins. For MinGW we can just remove the safety net, and if the plugin crashes, oh well.

http://searchfox.org/mozilla-central/rev/40e8eb46609dcb8780764774ec550afff1eed3a5/media/webrtc/trunk/webrtc/modules/video_capture/windows/sink_filter_ds.cc#343,346
http://searchfox.org/mozilla-central/rev/40e8eb46609dcb8780764774ec550afff1eed3a5/media/webrtc/trunk/webrtc/base/platform_thread.cc#90

Both of these uses appear to be used to set a thread name. I think this is only used for Debugging, is that correct?  If so, we can simply omit this from MinGW.  (As an aside, is this being run in release builds? If so, should it?)


Randell would you be able to weigh in on my interpretations of the code?

Comment 3

[Tom Ritter [:tjr]]

Attachment: Bug 1393903 ifdef out the uses of __try in media/webrtc for the MinGW build (WIP)

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

Comment 4

[Randell Jesup [:jesup] (needinfo me)]

We definitely use the thread-names (for debugging, and for crashreporter I presume).

We do not use (and probably don't compile) the proxy code in webrtc.org. There's quite a bit of not-compiled or not-used code in media/webrtc/trunk.  We eliminate some subdirs when importing, but mostly we just tell it to compile the stuff we care about.

Comment 5

[Randell Jesup [:jesup] (needinfo me)]

Comment on attachment 8922974 [details]
Bug 1393903 ifdef out the uses of __try in media/webrtc for the MinGW build (WIP)

https://reviewboard.mozilla.org/r/194152/#review199174

::: media/webrtc/trunk/webrtc/base/platform_thread.cc:83
(Diff revision 1)
>  #endif
>  }
>  
>  void SetCurrentThreadName(const char* name) {
> -#if defined(WEBRTC_WIN)
> +#if defined(__MINGW32__)
> +  // Do nothing

"Do nothing on MINGW" or some such; or "MINGW doesn't support __try"

::: media/webrtc/trunk/webrtc/base/proxydetect.cc:601
(Diff revision 1)
>    // In the case of McAfee scriptproxy.dll, it does crash in
>    // older versions. Try to catch crashes here and treat as an
>    // error.
>    BOOL success = FALSE;
>  
> -#if (_HAS_EXCEPTIONS == 0)
> +#if !defined(__MINGW32__) && (_HAS_EXCEPTIONS == 0)

proxydetext.cc is in the "rtc_base" section of base.gyp; we only use the "rtc_base_approved" section, so we shouldn't be building/linking it.

::: media/webrtc/trunk/webrtc/modules/video_capture/windows/sink_filter_ds.cc:335
(Diff revision 1)
>      {
>          HANDLE handle= GetCurrentThread();
>          SetThreadPriority(handle, THREAD_PRIORITY_HIGHEST);
>          _threadHandle = handle;
> +
> +#ifndef __MINGW32__

ditto, add comment per above

Comment 6

[Tom Ritter [:tjr]]

(In reply to Randell Jesup [:jesup] from comment #4)
> We definitely use the thread-names (for debugging, and for crashreporter I
> presume).

This doesn't seem like a problem then. The only consumer of mingw builds is Tor (we don't and won't ship them to users) - and they disable the crash reporter. Either way, making debugging more tricky or the crash reporter less accurate isn't a huge issue to them. (I mean, we'd avoid it if it was simple of course.)

Comment 7

[Tom Ritter [:tjr]]

Comment on attachment 8922974 [details]
Bug 1393903 ifdef out the uses of __try in media/webrtc for the MinGW build (WIP)

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

Comment 8

[Tom Ritter [:tjr]]

Comment on attachment 8922974 [details]
Bug 1393903 ifdef out the uses of __try in media/webrtc for the MinGW build (WIP)

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

Comment 9

[Tom Ritter [:tjr]]

Last time I tried building WebRTC I did not get the same errors. I'm resolving the sub-bugs as Incomplete until I can go back to building WebRTC and if they are still applicable, I'll reopen.