591576 - [comm-central] widget/src/windows/GfxInfo.cpp now reports various errors

Status: VERIFIED FIXED

(Core :: Graphics, defect)

Description

[Serge Gautherie (:sgautherie)]

First, all SM and TB builds failed due to 1(+) other cause, which regression timeframe was
http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=a6c18a123fbb&tochange=9f8fc8a50525

When that was fixed, this new Windows-only error was revealed:
http://tinderbox.mozilla.org/showbuilds.cgi?tree=Thunderbird&maxdate=1282937546&hours=24&legend=0&norules=1
http://tinderbox.mozilla.org/showlog.cgi?log=Thunderbird/1282925383.1282926551.31277.gz
WINNT 5.2 comm-central build on 2010/08/27 09:09:43
{
widget/src/windows/GfxInfo.cpp(194) : fatal error C1903: unable to recover from previous error(s); stopping compilation
}

Regression timeframe:
http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=a6c18a123fbb&tochange=a8328013a773
Likely culprits: bug 586046 (and/or bug 586048).
(Damn, I missed to find bug 586046 comment 35 in my search :-/ Anyway.)

Please, fix or back out!
This has been going on for 16 hours!! :-(

Comment 1

[Ian Neal]

Just wondering why this doesn't show for Firefox windows builds, some difference in build config?
Does changing back to WCHAR from TCHAR fix it?

Comment 2

[Jeff Muizelaar [:jrmuizel]]

I believe this should be fixed now...

Comment 3

[Jeff Muizelaar [:jrmuizel]]

Apparently there are still linker errors. These should be able to be fixed by changing the ifdef MOZ_CRASHREPORTER in GfxInfo.cpp to #if MOZ_CRASHREPORTER && MOZ_ENABLE_LIBXUL or something like that.

Comment 4

[Serge Gautherie (:sgautherie)]

(In reply to comment #3)
> Apparently there are still linker errors.

{
GfxInfo.obj : error LNK2019: unresolved external symbol "unsigned int __cdecl CrashReporter::AppendAppNotesToCrashReport(class nsACString_internal const &)" (?AppendAppNotesToCrashReport@CrashReporter@@YAIABVnsACString_internal@@@Z) referenced in function "private: void __thiscall mozilla::widget::GfxInfo::AddCrashReportAnnotations(void)" (?AddCrashReportAnnotations@GfxInfo@widget@mozilla@@AAEXXZ)

GfxInfo.obj : error LNK2019: unresolved external symbol "unsigned int __cdecl CrashReporter::AnnotateCrashReport(class nsACString_internal const &,class nsACString_internal const &)" (?AnnotateCrashReport@CrashReporter@@YAIABVnsACString_internal@@0@Z) referenced in function "private: void __thiscall mozilla::widget::GfxInfo::AddCrashReportAnnotations(void)" (?AddCrashReportAnnotations@GfxInfo@widget@mozilla@@AAEXXZ)

NEXT ERROR gkwidget.dll : fatal error LNK1120: 2 unresolved externals
}

Comment 5

[Ian Neal]

Could that be from the landing for Bug 586048?

Comment 6

[Jeff Muizelaar [:jrmuizel]]

Attachment: Fix link error [Checkin: Comment 21]

Does this patch fix it?

Comment 7

[Justin Wood (:Callek)]

Alternatively 

SHARED_LIBRARY_LIBS += $(DEPTH)/toolkit/crashreporter/$(LIB_PREFIX)exception_handler_s.$(LIB_SUFFIX)

Might work in widget/src/windows/Makefile.in

Comment 8

[Philip Chee]

Comment on attachment 470194 [details] [diff] [review]
Fix link error [Checkin: Comment 21]

Patch works. I built SeaMonkey 2.1b1pre successfully.

Comment 9

[Ian Neal]

Attachment: Makefile.in patch

This patch:
* Uses Callek's Makefile.in suggestion

If this does not work, try doing between the ifdef/endif:
SHARED_LIBRARY_LIBS += \
        $(DEPTH)/toolkit/crashreporter/$(LIB_PREFIX)exception_handler_s.$(LIB_SUFFIX)
        $(DEPTH)/toolkit/crashreporter/google-breakpad/src/client/windows/handler/$(LIB_PREFIX)exception_handler_s.$(LIB_SUFFIX) \
        $(DEPTH)/toolkit/crashreporter/google-breakpad/src/client/windows/crash_generation/$(LIB_PREFIX)crash_generation_s.$(LIB_SUFFIX) \
        $(DEPTH)/toolkit/crashreporter/google-breakpad/src/common/windows/$(LIB_PREFIX)breakpad_windows_common_s.$(LIB_SUFFIX)

Comment 10

[Philip Chee]

Comment on attachment 470271 [details] [diff] [review]
Makefile.in patch

> +ifdef MOZ_CRASHREPORTER
> +SHARED_LIBRARY_LIBS += $(DEPTH)/toolkit/crashreporter/$(LIB_PREFIX)exception_handler_s.$(LIB_SUFFIX)
> +endif

I tried that earlier but got four unresolved externals instead of two.

Comment 11

[Ian Neal]

Attachment: Makefile.in patch v2

Comment 12

[Justin Wood (:Callek)]

(In reply to comment #6)
> Created attachment 470194 [details] [diff] [review]
> Fix link errror
> 
> Does this patch fix it?

This does work for me, I think we should just take it other than my suggestion.

Comment 13

[Philip Chee]

Comment on attachment 470279 [details] [diff] [review]
Makefile.in patch v2

Thanks this one works.

Comment 14

[Justin Wood (:Callek)]

(In reply to comment #13)
> Comment on attachment 470279 [details] [diff] [review]
> Makefile.in patch v2
> 
> Thanks this one works.

From a chat with khuey on IRC, I doubt this one works, "right" (yes it links and runs). But I don't think linking in the crashreporter multiple times actually is correct.

Comment 15

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Comment on attachment 470279 [details] [diff] [review]
Makefile.in patch v2

SHARED_LIBRARY_LIBS says "take these static libs and make me a shared library out of them".  This will link two separate copies of crashreporter, one into gkwidget.dll and another into wherever it lives ATM (probably xul.dll).

Comment 16

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Comment on attachment 470194 [details] [diff] [review]
Fix link error [Checkin: Comment 21]

This is what you want.

Comment 17

[Ian Neal]

(In reply to comment #16)
> Comment on attachment 470194 [details] [diff] [review]
> Fix link errror
> 
> This is what you want.

Except this version means unless you building with enable-xul you won't get crash reporter information for this part of the code.

Comment 18

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

(In reply to comment #17)
> (In reply to comment #16)
> > Comment on attachment 470194 [details] [diff] [review] [details]
> > Fix link errror
> > 
> > This is what you want.
> 
> Except this version means unless you building with enable-xul you won't get
> crash reporter information for this part of the code.

It may be suboptimal, but I'm not going to take a patch that links crashreporter in twice for this.  Ted, of course, is the module owner, so you may make your case to him if you disagree.

Non-libxul will be dead soon enough anyways.

Comment 19

[Justin Wood (:Callek)]

(In reply to comment #17)
> (In reply to comment #16)
> > Comment on attachment 470194 [details] [diff] [review] [details]
> > Fix link errror
> > 
> > This is what you want.
> 
> Except this version means unless you building with enable-xul you won't get
> crash reporter information for this part of the code.

Correct, but I don't think this data is valuable enough to warrant a lot of extra time on this (for us), whereas libxul will let us get it for free. Also I don't want to take a patch that could also damage non-libxul being able to actually _use_ crashreporter.

Comment 20

[Ian Neal]

(In reply to comment #19)
> (In reply to comment #17)
> > (In reply to comment #16)
> > > Comment on attachment 470194 [details] [diff] [review] [details] [details]
> > > Fix link errror
> > > 
> > > This is what you want.
> > 
> > Except this version means unless you building with enable-xul you won't get
> > crash reporter information for this part of the code.
> 
> Correct, but I don't think this data is valuable enough to warrant a lot of
> extra time on this (for us), whereas libxul will let us get it for free. Also I
> don't want to take a patch that could also damage non-libxul being able to
> actually _use_ crashreporter.

True, ship it (as they say).

Comment 21

[Ian Neal]

Comment on attachment 470194 [details] [diff] [review]
Fix link error [Checkin: Comment 21]

http://hg.mozilla.org/mozilla-central/rev/4f6d1a4cd8ee

Comment 22

[Serge Gautherie (:sgautherie)]

V.Fixed, per
http://tinderbox.mozilla.org/showlog.cgi?log=SeaMonkey/1283123989.1283130605.8915.gz
WINNT 5.2 comm-central-trunk build on 2010/08/29 16:19:49