448613 - Initialize nsToolkit correctly when libxul is enabled in a debug version

Status: RESOLVED FIXED

(Core :: Widget: Win32, defect)

Description

[Bo Yang]

Attachment: A Proposed patch

nsToolkit::Startup is used to init the nsToolkit. When I build the FF with --enable-debug, the windows widgets code is built in a separate dll, and it is in the DllMain where the nsToolkit::Startup is called. But if I specify --enable-debug and --enable-libxul, there is no point the nsToolkit::Startup is called. And this will cause some error on the debug version FF when it did not run in a debugger.

This patch just fix this by calling nsToolkit::Startup in proper point.

Comment 1

[Benjamin Smedberg]

Comment on attachment 331815 [details] [diff] [review]
A Proposed patch

Actually, I don't think this patch is correct. In libxul builds this should be called by DllMain at http://mxr.mozilla.org/mozilla-central/source/toolkit/library/nsDllMain.cpp#48

Can you explain why it isn't being called from there, perhaps specific to the mingw build somehow?

Comment 2

[Bo Yang]

(In reply to comment #1)
> (From update of attachment 331815 [details] [diff] [review])
> Actually, I don't think this patch is correct. In libxul builds this should be
> called by DllMain at
> http://mxr.mozilla.org/mozilla-central/source/toolkit/library/nsDllMain.cpp#48
> 
> Can you explain why it isn't being called from there, perhaps specific to the
> mingw build somehow?
> 

Ah, from the code:
http://mxr.mozilla.org/mozilla-central/source/toolkit/library/libxul-config.mk#44

It is only called when the platform is WINNT and the compiler is not GCC...

Comment 3

[Benjamin Smedberg]

Yeah, we should be building nsDllMain.cpp for both mingw and MSVC.

Comment 4

[Bo Yang]

Attachment: The revised patch

Benjamin, do you mean I should change the libxul-config.mk like the revised patch?

Comment 5

[Benjamin Smedberg]

Comment on attachment 333204 [details] [diff] [review]
The revised patch

Does this patch build correctly? I'm a little surprised that the RCINCLUDE stuff builds with mingw, and I know that the *deps files are unneeded.

What I meant for you to do was separate out nsDllMain.cpp and build that always on windows, but leave the rest of that block MSVC-only.

Comment 6

[Bo Yang]

(In reply to comment #5)
> (From update of attachment 333204 [details] [diff] [review])
> Does this patch build correctly? I'm a little surprised that the RCINCLUDE
> stuff builds with mingw, and I know that the *deps files are unneeded.

Yes, the patch build correctly. 
 
> What I meant for you to do was separate out nsDllMain.cpp and build that always
> on windows, but leave the rest of that block MSVC-only.

Does all windows machines include WINCE platform? If so, I think the ifdefined(WINCE) Macro in the source code should be removed too. 

Now I have figured out a new patch for this:

diff --git a/toolkit/library/libxul-config.mk b/toolkit/library/libxul-config.mk
--- a/toolkit/library/libxul-config.mk
+++ b/toolkit/library/libxul-config.mk
@@ -41,13 +41,17 @@
 	nsStaticXULComponents.cpp \
 	$(NULL)
 
+ifeq ($(OS_ARCH),WINNT)
+REQUIRES += libreg widget gfx
+CPPSRCS += \
+        nsDllMain.cpp \
+        $(NULL)
+endif
+
 ifeq ($(OS_ARCH)_$(GNU_CC),WINNT_)
-REQUIRES += libreg widget gfx
-
 CPPSRCS += \
 	dlldeps.cpp \
 	nsGFXDeps.cpp \
-	nsDllMain.cpp \
 	$(NULL)
 
 RCINCLUDE = xulrunner.rc

But I think the condition 
ifeq ($(OS_ARCH),WINNT) is not enough to contain all windows platform. Could you please tell me how can I make a full condition , maybe:

ifneq (,$(filter WIN% Windows% CYGWIN%, $(OS_ARCH)) 

Comment 7

[Benjamin Smedberg]

ifeq ($(OS_ARCH),WINNT) should be sufficient for desktop windows (WINCE for windows mobile). We use a canonical OS_ARCH.

Comment 8

[Bo Yang]

Attachment: The third version

Now, we make sure the desktop use the nsDllMain.cpp. Disregard the WINCE env...

Comment 9

[Bo Yang]

Should we ask super-review or check this in? 

Comment 10

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 333494 [details] [diff] [review]
The third version

Just check it in.

Comment 11

[Serge Gautherie (:sgautherie)]

Attachment: (Av3a) maintain tabs, not spaces [Checkin: Comment 11]

Bo, your patch had spaces instead of tabs, so it could not apply.

http://hg.mozilla.org/mozilla-central/rev/6b64d5fe2848

Comment 12

[Bo Yang]

(In reply to comment #11)
> Created an attachment (id=337244) [details]
> (Av3a) maintain tabs, not spaces [Checkin: Comment 10]
> 
> Bo, your patch had spaces instead of tabs, so it could not apply.
> 
> http://hg.mozilla.org/mozilla-central/rev/6b64d5fe2848

Thank you very much for the revise, Serge!