Closed Bug 448613 Opened 13 years ago Closed 13 years ago

Initialize nsToolkit correctly when libxul is enabled in a debug version

Categories

(Core :: Widget: Win32, defect)

x86
Windows XP
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla1.9.1b1

People

(Reporter: Techrazy.Yang, Assigned: Techrazy.Yang)

Details

Attachments

(1 file, 3 obsolete files)

Attached patch A Proposed patch (obsolete) — Splinter Review
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.
Attachment #331815 - Flags: review?(emaijala)
Attachment #331815 - Flags: superreview+
Attachment #331815 - Flags: review?(emaijala) → review+
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?
Attachment #331815 - Flags: superreview+ → superreview-
(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...
Yeah, we should be building nsDllMain.cpp for both mingw and MSVC.
Attached patch The revised patch (obsolete) — Splinter Review
Benjamin, do you mean I should change the libxul-config.mk like the revised patch?
Attachment #331815 - Attachment is obsolete: true
Attachment #333204 - Flags: review?(benjamin)
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.
(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)) 

ifeq ($(OS_ARCH),WINNT) should be sufficient for desktop windows (WINCE for windows mobile). We use a canonical OS_ARCH.
Attached patch The third version (obsolete) — Splinter Review
Now, we make sure the desktop use the nsDllMain.cpp. Disregard the WINCE env...
Attachment #333204 - Attachment is obsolete: true
Attachment #333494 - Flags: review?(benjamin)
Attachment #333204 - Flags: review?(benjamin)
Attachment #333494 - Flags: review?(benjamin) → review+
Should we ask super-review or check this in? 
Blocks: 449557
Attachment #333494 - Flags: superreview?(roc)
Comment on attachment 333494 [details] [diff] [review]
The third version

Just check it in.
Attachment #333494 - Flags: superreview?(roc) → superreview+
Keywords: checkin-needed
Assignee: nobody → Techrazy.Yang
Bo, your patch had spaces instead of tabs, so it could not apply.

http://hg.mozilla.org/mozilla-central/rev/6b64d5fe2848
Attachment #333494 - Attachment is obsolete: true
Attachment #337244 - Attachment description: (Av3a) maintain tabs, not spaces [Checkin: Comment 10] → (Av3a) maintain tabs, not spaces [Checkin: Comment 11]
No longer blocks: 449557
Status: NEW → RESOLVED
Closed: 13 years ago
Flags: in-testsuite-
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → mozilla1.9.1b1
(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!
You need to log in before you can comment on or make changes to this bug.