726354 - Can't build mozilla-central with Parental Controls disabled

Status: RESOLVED FIXED

(Core :: General, defect)

Description

[Ryan VanderMeulen [:RyanVM]]

When building with Parental Controls disabled (--disable-parental-controls), the build fails due to the errors shown below:

nsToolkitCompsModule.cpp
d:\sdks\v7.0\include\msoav.h(18) : error C2146: syntax error : missing ';' before identifier 'lpstg'
d:\sdks\v7.0\include\msoav.h(18) : error C4430: missing type specifier - int assumed. Note: C++ does not support default-int
d:\sdks\v7.0\include\msoav.h(18) : error C4430: missing type specifier - int assumed. Note: C++ does not support default-int
d:\sdks\v7.0\include\msoav.h(36) : error C2065: 'IOfficeAntiVirus' : undeclared identifier
d:\sdks\v7.0\include\msoav.h(36) : error C2065: 'IUnknown' : undeclared identifier
d:\sdks\v7.0\include\msoav.h(37) : error C2448: 'DECLARE_INTERFACE_' : function-style initializer appears to be a function definition

Full log:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-a7a538c80fbb/try-win32/try-win32-build1998.txt.gz

Bisecting in hg, the regressing patch was the second one to land from bug 696033 - "Add xperf probes (sample startup probes)".
http://hg.mozilla.org/mozilla-central/rev/0eebc33d8593

I've also confirmed it via local backout.

This can be tested on Try by adding |ac_add_options --disable-parental-controls| to browser/config/mozconfigs/win32/nightly

Comment 1

[David Teller [:Yoric] - still alive but not very active]

I will investigate this asap.
Can anybody tell me what |--disable-parental-controls| does in terms of compilation?

Comment 2

[Ryan VanderMeulen [:RyanVM]]

http://mxr.mozilla.org/mozilla-central/search?find=%2F&string=MOZ_DISABLE_PARENTAL_CONTROLS

Comment 3

[:Felipe Gomes (needinfo for replies!)]

Yoric: do you really need windows.h in prefprobe.h? Is perfprobe.h meant to be included in various places? (i.e. each file that needs to use the probes)
Usually including windows headers (specially windows.h) in big existing files causes various #define clash problems.. I hit some similar problems in the past while working on the profiler, see here: http://mxr.mozilla.org/mozilla-central/source/tools/profiler/thread_helper.h#42
and here: http://mxr.mozilla.org/mozilla-central/source/tools/profiler/sps_sampler.h#105

I suspect this problem is similar

Comment 4

[David Teller [:Yoric] - still alive but not very active]

I need windows.h to define a number of types, including TRACEHANDLE. I might be able to get around this by pretending it is a |void*|, of course, but I do not really like that solution.

Now, I have a fix for Ryan's particular issue, but for some reason, it does not pass linking. I suspect this is not my fault, but I am not sure.

Comment 5

[David Teller [:Yoric] - still alive but not very active]

Attachment: Experimental fix

Ryan, does this solve your issue?

Comment 6

[Ryan VanderMeulen [:RyanVM]]

I'll try it out on my local builds tonight. Otherwise, you can test it on Try as noted in comment 0 if you want an answer sooner than that.

Comment 7

[David Teller [:Yoric] - still alive but not very active]

(In reply to Ryan VanderMeulen from comment #6)
> I'll try it out on my local builds tonight. Otherwise, you can test it on
> Try as noted in comment 0 if you want an answer sooner than that.

I will do that, thanks.

Comment 8

[Mozilla RelEng Bot]

Try run for 7edd7bb478b5 is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=7edd7bb478b5
Results (out of 1 total builds):
    success: 1
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-7edd7bb478b5

Comment 9

[Ryan VanderMeulen [:RyanVM]]

(In reply to Mozilla RelEng Bot from comment #8)
> Try run for 7edd7bb478b5 is complete.
> Detailed breakdown of the results available here:
>     https://tbpl.mozilla.org/?tree=Try&rev=7edd7bb478b5
> Results (out of 1 total builds):
>     success: 1
> Builds (or logs if builds failed) available at:
> http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-
> 7edd7bb478b5

Works locally as well. Thanks for the patch!

Comment 10

[David Teller [:Yoric] - still alive but not very active]

Attachment: Fix

Comment 11

[David Teller [:Yoric] - still alive but not very active]

The final fix will probably be a small refactoring of perfprobe.{h, cpp}, that will let us avoid using <windows.h>.

Comment 12

[Ryan VanderMeulen [:RyanVM]]

(In reply to David Rajchenbach Teller [:Yoric] from comment #11)
> The final fix will probably be a small refactoring of perfprobe.{h, cpp},
> that will let us avoid using <windows.h>.

Was that work planned for this bug or a new one?

Comment 13

[David Teller [:Yoric] - still alive but not very active]

(In reply to Ryan VanderMeulen from comment #12)
> (In reply to David Rajchenbach Teller [:Yoric] from comment #11)
> > The final fix will probably be a small refactoring of perfprobe.{h, cpp},
> > that will let us avoid using <windows.h>.
> 
> Was that work planned for this bug or a new one?

This bug demonstrated that I should not have used <windows.h> in a public header, as it contains a number of highly contagious macros.

Comment 14

[Ryan VanderMeulen [:RyanVM]]

Mainly, I'm wondering if this patch is good enough for review and checkin with further work in a follow-up bug or if the "right" fix is desired for here.

Comment 15

[David Teller [:Yoric] - still alive but not very active]

Comment on attachment 597377 [details] [diff] [review]
Fix

Mike, could you possibly review this patch? I intend to work on a more comprehensive fix later, but I do not like the prospect of breaking compilation for some people while I am working on other urgent stuff.

Comment 16

[Mike Hommey [:glandium]]

Comment on attachment 597377 [details] [diff] [review]
Fix

rubberstamp

Comment 17

[David Teller [:Yoric] - still alive but not very active]

Thanks, Mike

Comment 18

[Masatoshi Kimura [:emk]]

> +#include <ObjIdl.h>
Please use the lower-cased filename. Otherwise it will break MinGW builds.

Comment 19

[David Teller [:Yoric] - still alive but not very active]

(In reply to Masatoshi Kimura [:emk] from comment #18)
> > +#include <ObjIdl.h>
> Please use the lower-cased filename. Otherwise it will break MinGW builds.

ok

Comment 20

[David Teller [:Yoric] - still alive but not very active]

Attachment: Fix

Comment 21

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/d96ebdd268b2

Comment 22

[Phil Ringnalda (:philor)]

Pushed https://hg.mozilla.org/integration/mozilla-inbound/rev/211031b47396 as a followup - my feeling is, and 20 burning Windows builds seem to agree, "Idl".toLowerCase() != "dir" ;)

Comment 23

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/d96ebdd268b2
https://hg.mozilla.org/mozilla-central/rev/211031b47396

Comment 24

[David Teller [:Yoric] - still alive but not very active]

Does anybody remember why we had [don't close when landing on m-c]?
Otherwise, I believe that this bug can be closed.

Comment 25

[Ryan VanderMeulen [:RyanVM]]

Presumably for the follow-up work discussed above. No idea how relevant any of that conversation is anymore, though.

Comment 26

[David Teller [:Yoric] - still alive but not very active]

Let's close this bug, then.