925382 - Make bindings not compile if they include Windows system headers

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[Andrew McCreight [:mccr8]]

from bz: "I found the include chain involved and told qDot how to break it so he
can land his patch, but we should actually go ahead and make bindings
not compile if they include Windows headers"

If you end up including them into the bindings code, you get a weird linking error:

LINK : warning LNK4199: /DELAYLOAD:VCCORLIB110D.DLL ignored; no imports found from VCCORLIB110D.DLL

UnifiedBindings4.obj : error LNK2019: unresolved external symbol "public: struct already_AddRefed<class nsDOMEvent> __thiscall nsIDocument::CreateEventW(class nsAString_internal const &,class mozilla::ErrorResult &)const " (?CreateEventW@nsIDocument@@QBE?AU?$already_AddRefed@VnsDOMEvent@@@@ABVnsAString_internal@@AAVErrorResult@mozilla@@@Z) referenced in function "bool __cdecl mozilla::dom::DocumentBinding::createEvent(struct JSContext *,class JS::Handle<class JSObject *>,class nsIDocument *,class JSJitMethodCallArgs const &)" (?createEvent@DocumentBinding@dom@mozilla@@YA_NPAUJSContext@@V?$Handle@PAVJSObject@@@JS@@PAVnsIDocument@@ABVJSJitMethodCallArgs@@@Z)

xul.dll : fatal error LNK1120: 1 unresolved externals

Comment 1

[Boris Zbarsky [:bzbarsky]]

Oh, and the reason you get that error is because window.h has:

  #define CreateEvent CreateEventW

or some such.  And many more beside.

Maybe it's as "simple" as throwing

#ifdef _WINDOWS_
#error "Foo.cpp included windows.h"
#endif

in the unified files after each .cpp #include and then seeing whether we compile...

Comment 2

[Andrew McCreight [:mccr8]]

bz has a patch for this, so reassigning to him.  He can actually build on Windows locally so that makes him a better fit for this. ;)

Comment 3

[Boris Zbarsky [:bzbarsky]]

Attachment: Error out of unified bindings if one of them includes windows.h.

Comment 4

[Boris Zbarsky [:bzbarsky]]

Hackytry at https://tbpl.mozilla.org/?tree=Try&rev=14bee79e261f (pending the peerconnectionimpl stuff being fixed).

Comment 5

[Nathan Froyd [:froydnj]]

Comment on attachment 824173 [details] [diff] [review]
Error out of unified bindings if one of them includes windows.h.

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

::: python/mozbuild/mozbuild/backend/recursivemake.py
@@ +609,5 @@
>              # handle source files being added/removed/renamed.  Therefore, we
>              # generate them here also to make sure everything's up-to-date.
>              with self._write_file(os.path.join(output_directory, unified_file)) as f:
> +                includeTemplate = '#include "%(cppfile)s"'
> +                if unified_prefix == "UnifiedBindings":

Let's make this check a keyword argument, defaulting to false, to the function instead.  poison_windows_h?

Comment 6

[Boris Zbarsky [:bzbarsky]]

https://tbpl.mozilla.org/?tree=Try&rev=55735f0cd7b9

Comment 7

[Boris Zbarsky [:bzbarsky]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/9e70cbddf7fb

Gregory, as a heads-up this might conflict slightly with your bindings build system changes; if you want prefer I can do the merge there.

Comment 8

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

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