Last Comment Bug 472799 - Use binaryname=CurrentTimeMoz annotation rather than #undef GetCurrentTime to address Windows macro idiocy
: Use binaryname=CurrentTimeMoz annotation rather than #undef GetCurrentTime to...
Product: Core
Classification: Components
Component: Audio/Video (show other bugs)
: Trunk
: x86 Windows XP
-- trivial (vote)
: mozilla17
Assigned To: Jacek Caban
: Maire Reavy [:mreavy] Please needinfo me
Depends on: 466432
  Show dependency treegraph
Reported: 2009-01-09 00:28 PST by Jeff Walden [:Waldo] (remove +bmo to email)
Modified: 2012-07-20 06:47 PDT (History)
5 users (show)
See Also:
Crash Signature:
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---

fix v1.0 (36.58 KB, patch)
2012-07-14 06:01 PDT, Jacek Caban
jwatt: review-
Details | Diff | Splinter Review
fix v2.0 (895 bytes, patch)
2012-07-17 05:24 PDT, Jacek Caban
cpearce: review+
Details | Diff | Splinter Review

Description User image Jeff Walden [:Waldo] (remove +bmo to email) 2009-01-09 00:28:59 PST
Undefining GetCurrentTime breaks any third-party code that wants to interact with media elements while simultaneously using the Windows macro for whatever it does.  The binaryname annotation is the correct way to address this:
Comment 1 User image Aiko 2009-01-09 01:58:11 PST
Just for the record, GetCurrentTime is an ancient and obsolete part of the Win16 API. I doubt there are many Windows 3.1 apps dying to be ported to XULRunner or whatever, so this problem is pretty hypothetic.

It sure is nice that there is a better way to handle this, though.
Comment 2 User image Jacek Caban 2012-07-14 06:01:58 PDT
Created attachment 642219 [details] [diff] [review]
fix v1.0

I found this bug when I was looking at current mingw build breakage due to GetCurrentTime macro. It looks like there is a difference in includes order between GCC and MSVC build causing problems with the macro during Navigator.cpp compilation. Instead of improving the hack, I decided to get rid of all GetCurrentTime usages in m-c. The attached patch changes IDLs to use GetCurrentTimeMoz and all other cases s/GetCurrentTime/GetTime/. It passes locally, waiting for try results:
Comment 3 User image Jacek Caban 2012-07-14 08:39:01 PDT
Comment on attachment 642219 [details] [diff] [review]
fix v1.0

Looks good on try
Comment 4 User image Chris Pearce (:cpearce) 2012-07-16 10:48:36 PDT
I work on media and I've spoken to the SVG and SMIL guys and we agree that we'd prefer to not rename our uses of GetCurrentTime. We think it would reduce our code clarity.

However we are interested in ensuring that we can build on Mingw/GCC... So can you work around this some other way? Can you instead re-order the includes so that you can build on Mingw/GCC? Or is there some other work around?
Comment 5 User image Jonathan Watt [:jwatt] 2012-07-16 11:08:38 PDT
Comment on attachment 642219 [details] [diff] [review]
fix v1.0

Yeah, sorry, r- on the SVG name changes at least, unless it turns out this can't be worked around another way. What are the actual Mingw build errors being caused by? GetCurrentTime being redefined and breaking the SVG code? GetCurrentTime being undefined and then being needed by non-Mozilla source files?
Comment 6 User image Jacek Caban 2012-07-17 05:22:44 PDT
I investigated it more and it's not a difference in includes. Following code:

#define TEST()

will cause an error on GCC, while it's only warning on (most) MSVCs. See:

for more info. To sum it up, MSVC ignores extra macro arguments, while GCC throws an error. We have #undef GetCurrentTime in a few places, but it's not sufficient. What happens in this case is that we attempt to undef it before winbase.h is included, then include winbase.h and use it in nsDOMMediaStream.h. In case of MSVC, it just causes a warning, When you look at preprocessed file, it contains bogus declaration of nsDOMMediaStream class with a function:

virtual nsresult __stdcall GetTickCount();

instead of GetCurrentTime(double *aCurrentTime). This happens to work because that function is not used in Navigator.cpp. The same happens to nsSMILTimeContainer's GetCurrentTime in Navigator.cpp. I expect this to happen in more places in the codebase, but it's unnoticed (no example to prove). Note that such cases work fine even on GCC as long as GetCurrentTime doesn't get arguments (and its non-IDL-based implementations don't take arguments). This may lead to strange behavior.

However, if you prefer the current solution, that's fine with me. I will attach a patch adding one more #undef to make GCC happy and we can deal with eventual future collisions when they happen.
Comment 7 User image Jacek Caban 2012-07-17 05:24:43 PDT
Created attachment 642917 [details] [diff] [review]
fix v2.0

This is sufficient for mingw compilation and fixes a warning on MSVC.
Comment 8 User image Jacek Caban 2012-07-17 05:26:45 PDT
To give better context, the problemactic macro from winbase.h looks like this:

#define GetCurrentTime() GetTickCount()
Comment 9 User image Chris Pearce (:cpearce) 2012-07-17 05:49:03 PDT
Comment on attachment 642917 [details] [diff] [review]
fix v2.0

Review of attachment 642917 [details] [diff] [review]:

Much better, thanks!
Comment 10 User image Jacek Caban 2012-07-19 03:01:19 PDT
Thanks for reviews, pushed to m-i:
Comment 11 User image Ed Morley [:emorley] 2012-07-20 06:47:39 PDT

Note You need to log in before you can comment on or make changes to this bug.