Closed Bug 591576 Opened 12 years ago Closed 12 years ago

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

Categories

(Core :: Graphics, defect)

x86
Windows Server 2003
defect
Not set
blocker

Tracking

()

VERIFIED FIXED
mozilla2.0b5

People

(Reporter: sgautherie, Assigned: jrmuizel)

References

Details

(Keywords: regression)

Attachments

(1 file, 2 obsolete files)

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!! :-(
Summary: widget/src/windows/GfxInfo.cpp now reports various errors → [comm-central] widget/src/windows/GfxInfo.cpp now reports various errors
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?
I believe this should be fixed now...
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.
(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
}
Could that be from the landing for Bug 586048?
Does this patch fix it?
Alternatively 

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

Might work in widget/src/windows/Makefile.in
Comment on attachment 470194 [details] [diff] [review]
Fix link error [Checkin: Comment 21]

Patch works. I built SeaMonkey 2.1b1pre successfully.
Attachment #470194 - Flags: feedback+
Attached patch Makefile.in patch (obsolete) — Splinter Review
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)
Attachment #470271 - Flags: feedback?(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.
Attachment #470271 - Flags: feedback?(philip.chee)
Attached patch Makefile.in patch v2 (obsolete) — Splinter Review
Attachment #470271 - Attachment is obsolete: true
Attachment #470279 - Flags: feedback?(philip.chee)
(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 on attachment 470279 [details] [diff] [review]
Makefile.in patch v2

Thanks this one works.
Attachment #470279 - Flags: feedback?(philip.chee) → feedback+
(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 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).
Attachment #470279 - Flags: review-
Comment on attachment 470194 [details] [diff] [review]
Fix link error [Checkin: Comment 21]

This is what you want.
Attachment #470194 - Flags: review+
(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.
(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.
(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.
(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).
Attachment #470279 - Attachment is obsolete: true
Comment on attachment 470194 [details] [diff] [review]
Fix link error [Checkin: Comment 21]

http://hg.mozilla.org/mozilla-central/rev/4f6d1a4cd8ee
Attachment #470194 - Attachment description: Fix link errror → Fix link error [Checkin: Comment 21]
Assignee: nobody → jmuizelaar
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
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
Status: RESOLVED → VERIFIED
Flags: in-testsuite-
Target Milestone: --- → mozilla2.0b5
You need to log in before you can comment on or make changes to this bug.