59 bytes, text/x-review-board-request
Landing the fix for bug 956338 broke compiling with mingw-w64: /home/firefox/win52/toolchain/mingw-w64/bin/i686-w64-mingw32-g++ -std=gnu++11 -mwindows -o Unified_cpp_dom_presentation0.o -c -I/home/firefox/win52/mozilla-central/obj-mingw/dist/stl_wrappers -DNDEBUG=1 -DTRIMMED=1 -DWIN32_LEAN_AND_MEAN -D_WIN32 -DWIN32 -D_CRT_RAND_S -DCERT_CHAIN_PARA_HAS_EXTRA_FIELDS -DOS_WIN=1 -D_UNICODE -DCHROMIUM_BUILD -DU_STATIC_IMPLEMENTATION -DUNICODE -D_WINDOWS -D_SECURE_ATL -DSTATIC_EXPORTABLE_JS_API -DMOZILLA_INTERNAL_API -DIMPL_LIBXUL -I/home/firefox/win52/mozilla-central/dom/presentation -I/home/firefox/win52/mozilla-central/obj-mingw/dom/presentation -I/home/firefox/win52/mozilla-central/dom/base -I/home/firefox/win52/mozilla-central/obj-mingw/ipc/ipdl/_ipdlheaders -I/home/firefox/win52/mozilla-central/ipc/chromium/src -I/home/firefox/win52/mozilla-central/ipc/glue -I/home/firefox/win52/mozilla-central/obj-mingw/dist/include -I/home/firefox/win52/mozilla-central/obj-mingw/dist/include/nspr -I/home/firefox/win52/mozilla-central/obj-mingw/dist/include/nss -DMOZILLA_CLIENT -include /home/firefox/win52/mozilla-central/obj-mingw/mozilla-config.h -MD -MP -MF .deps/Unified_cpp_dom_presentation0.o.pp -Wall -Wc++11-compat -Wempty-body -Wignored-qualifiers -Woverloaded-virtual -Wpointer-arith -Wsign-compare -Wtype-limits -Wunreachable-code -Wwrite-strings -Wno-invalid-offsetof -Wc++14-compat -Wno-error=maybe-uninitialized -Wno-error=deprecated-declarations -Wno-error=array-bounds -Wno-format -fno-exceptions -fno-strict-aliasing -mms-bitfields -mstackrealign -fno-keep-inline-dllexport -fno-rtti -fno-exceptions -fno-math-errno -pipe -g -O -fomit-frame-pointer /home/firefox/win52/mozilla-central/obj-mingw/dom/presentation/Unified_cpp_dom_presentation0.cpp In file included from /home/firefox/win52/mozilla-central/dom/presentation/AvailabilityCollection.h:11:0, from /home/firefox/win52/mozilla-central/dom/presentation/AvailabilityCollection.cpp:7, from /home/firefox/win52/mozilla-central/obj-mingw/dom/presentation/Unified_cpp_dom_presentation0.cpp:2: /home/firefox/win52/mozilla-central/obj-mingw/dist/include/mozilla/WeakPtr.h:96:8: error: ‘thread’ in namespace ‘std’ does not name a type std::thread::id _owningThread; \ ^ /home/firefox/win52/mozilla-central/obj-mingw/dist/include/mozilla/WeakPtr.h:173:3: note: in expansion of macro ‘MOZ_WEAKPTR_DECLARE_THREAD_SAFETY_CHECK’ MOZ_WEAKPTR_DECLARE_THREAD_SAFETY_CHECK ^ /home/firefox/win52/mozilla-central/obj-mingw/dist/include/mozilla/WeakPtr.h: In constructor ‘mozilla::detail::WeakReference<T>::WeakReference(T*)’: /home/firefox/win52/mozilla-central/obj-mingw/dist/include/mozilla/WeakPtr.h:100:5: error: ‘_owningThread’ was not declared in this scope _owningThread = std::this_thread::get_id(); \ ^ /home/firefox/win52/mozilla-central/obj-mingw/dist/include/mozilla/WeakPtr.h:142:5: note: in expansion of macro ‘MOZ_WEAKPTR_INIT_THREAD_SAFETY_CHECK’ MOZ_WEAKPTR_INIT_THREAD_SAFETY_CHECK(); ^ /home/firefox/win52/mozilla-central/obj-mingw/dist/include/mozilla/WeakPtr.h:100:26: error: ‘std::this_thread’ has not been declared _owningThread = std::this_thread::get_id(); \ etc.
Comment on attachment 8826637 [details] Bug 1317642 Disable WeakPtr thread assertions on MinGW https://reviewboard.mozilla.org/r/104558/#review105292 ::: mfbt/WeakPtr.h:92 (Diff revision 1) > // "owning thread". It means that each query for a weak reference, > // its dereference, and destruction of the real object must all happen > // on a single thread. The following macros implement assertions for > // checking these conditions. > > -#if defined(DEBUG) || (defined(NIGHTLY_BUILD) && !defined(MOZ_PROFILING)) > +#if !defined(__MINGW32__) && (defined(DEBUG) || (defined(NIGHTLY_BUILD) && !defined(MOZ_PROFILING))) Please add a comment explaining why the `__MINGW32__` check is here. I know there's an explanation in the commit message, but I think it's also helpful to add it to the code as well.
If you add a comment, it would also be nice to have info about what future state we'd need to remove this ifdef. On IRC jacek mentioned "libgcc/stdc++ support[ing] win32 threads natively". I don't know if that's a thing that anyone has plans to do or at least has a bug on file somewhere or just wishful thinking. If there's a bug somewhere it'd be great to link it in the comment so that if someone in the future stumbles upon it and it has been fixed we can remove the ifdef.
Assigning this bug to myself and ni-ing because it appears the patch kept the r+ despite me editing it, and Nathan should look at again and approve.
Assignee: nobody → tom
Congratulations, you're running into bug 1195661! :) I will look at the patch again.
Comment on attachment 8826637 [details] Bug 1317642 Disable WeakPtr thread assertions on MinGW Looks good. I went ahead and autolanded this for you. Thanks again!
Pushed by firstname.lastname@example.org: https://hg.mozilla.org/integration/autoland/rev/1bc3ddd4cdc3 Disable WeakPtr thread assertions on MinGW r=froydnj
Comment on attachment 8826637 [details] Bug 1317642 Disable WeakPtr thread assertions on MinGW Approval Request Comment [Feature/Bug causing the regression]: Over the past few releases we accumulated multiple bugs that break MinGW compilation. [User impact if declined]: Tor will have to backport the patch and manage it themselves [Is this code covered by automated tests?]: No [Has the fix been verified in Nightly?]: N/A [Needs manual test from QE? If yes, steps to reproduce]: No [List of other uplifts needed for the feature/fix]: No other uplifts are dependent on this one, although other MinGW compilation bugs are desirable to uplift also. [Is the change risky?]: No [Why is the change risky/not risky?]: It is not risky because it only affects MinGW compilations. [String changes made/needed]: n/a
Attachment #8826637 - Flags: approval-mozilla-aurora?
Status: NEW → RESOLVED
Last Resolved: 2 years ago
status-firefox53: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla53
Comment on attachment 8826637 [details] Bug 1317642 Disable WeakPtr thread assertions on MinGW build fix for mingw, aurora52+
Attachment #8826637 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
status-firefox52: --- → fixed
You need to log in before you can comment on or make changes to this bug.