The default bug view has changed. See this FAQ.

Build Firefox --enable-libxul (not static) by default

RESOLVED FIXED in Firefox 3 alpha3

Status

()

Firefox
Build Config
P2
normal
RESOLVED FIXED
11 years ago
9 years ago

People

(Reporter: bsmedberg, Assigned: bsmedberg)

Tracking

(Blocks: 1 bug)

Trunk
Firefox 3 alpha3
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(7 attachments, 3 obsolete attachments)

(Assignee)

Description

11 years ago
In trasitioning to firefox-on-xulrunner, I'd like to start building Firefox --enable-libxul by default. This will track the remaining work to make that happen.
(Assignee)

Comment 1

11 years ago
Created attachment 230363 [details] [diff] [review]
Glue additions, rev. 1 [checked in]
Attachment #230363 - Flags: review?(darin)
(Assignee)

Comment 2

11 years ago
Created attachment 230364 [details] [diff] [review]
Browser changes, rev. 1
Attachment #230364 - Flags: review?(darin)
(Assignee)

Comment 3

11 years ago
I need to write an additional patch to make --enable-libxul the default in configure and fix up the installer packaging, but I can't do that until the spellchecker stuff from bug 339106 is landed.

Comment 4

11 years ago
Comment on attachment 230363 [details] [diff] [review]
Glue additions, rev. 1 [checked in]

+  if (*aASCIIString)
+    return PR_FALSE;
+
+  return PR_TRUE;

  return !*aASCIIString;
Attachment #230363 - Flags: review?(darin) → review+

Updated

11 years ago
Attachment #230364 - Flags: review?(darin) → review+
(Assignee)

Updated

11 years ago
Depends on: 346679
(Assignee)

Comment 5

11 years ago
Comment on attachment 230363 [details] [diff] [review]
Glue additions, rev. 1 [checked in]

I've got this completely working on Linux, and startup times are fantastic! Compiling windows/mac now, I should have a patch tomorrow afternoon.
Attachment #230363 - Attachment description: Glue additions, rev. 1 → Glue additions, rev. 1 [checked in]
(Assignee)

Comment 6

11 years ago
Created attachment 232125 [details] [diff] [review]
Build system changes, rev. 1

This won't do the right thing without the prerequisite patches here and in the unionenumerator bug, but it works!
Attachment #232125 - Flags: review?(mark)
(Assignee)

Comment 7

11 years ago
Created attachment 232127 [details] [diff] [review]
Browser changes, rev. 1.1

The final set of browser/components changes, including some win/mac fixup and conflict resolution with my own patch in bug 344159
Attachment #230364 - Attachment is obsolete: true
(Assignee)

Updated

11 years ago
Depends on: 347183
(In reply to comment #1)
> Created an attachment (id=230363) [edit]
> Glue additions, rev. 1

If nspr is avaliable inside nsStringAPI.cpp, isn't it
reasonable to use PR_snprintf(...) instead of _snprintf(...)?

_snprintf is one of dll hell trigers on Windows, so some embedding 
app programmers may fail to link their app to geckosdk static libs,
which are linking to msvcrt/libcmt.

i.e.
http://lxr.mozilla.org/mozilla/source/xpcom/glue/nsStringAPI.cpp

> 59 #ifdef XP_WIN
> 60 #define snprintf _snprintf
> 61 #endif

->

#ifdef XP_WIN
#  ifdef XPCOM_GLUE_AVOID_NSPR
#    define snprintf _snprintf
#  else
#    define snprintf PR_snprintf
#  endif
#endif
(Assignee)

Comment 9

11 years ago
Is there something special about _snprintf in particular? We already have the problem of CRT dependencies and mixed CRTs, but that can usually mostly be fixed with /nodefaultlib, right? Anyway, that's a separate bug.

Comment 10

11 years ago
Comment on attachment 232125 [details] [diff] [review]
Build system changes, rev. 1

+# A Firefox static build is really a meta-component build, not truly static
+if test "$MOZ_BUILD_APP" = "browser" -a -n "$BUILD_STATIC_LIBS"; then
+    MOZ_META_COMPONENT=1
+fi

We should probably init MOZ_META_COMPONENT to empty up above.

Something's missing here - I get this when linking libappmeta.

/usr/bin/ld: Undefined symbols:
Apprunner_NSGetModule(nsIComponentManager*, nsIFile*, nsIModule**)
_NS_NewUnionEnumerator
NS_NewArrayEnumerator(nsISimpleEnumerator**, nsCOMArray_base const&)
_FSGetParentRef
_FSRefValid
_NS_StringGetData_P
_NS_StringGetMutableData_P
nsAString_internal::GetWritableBuffer(unsigned short**, unsigned int)
nsAString_internal::Assign(nsAString_internal const&)
nsAString_internal::Length() const
nsDefaultStringComparator::operator()(unsigned short const*, unsigned short const*, unsigned int) const
vtable for nsDefaultStringComparator

And in a shared build, when linking libbrowserdirprovider:

/usr/bin/ld: Undefined symbols:
_NS_NewUnionEnumerator
NS_NewArrayEnumerator(nsISimpleEnumerator**, nsCOMArray_base const&)
(Assignee)

Comment 11

11 years ago
Created attachment 232594 [details] [diff] [review]
Build system changes, rev. 1.1

I forgot toolkit/xre/Makefile.in in the patch. The remaining linkage changes are covered in bug 347183, which is a prerequisite (I've built this patch successfully on all three platforms).
Attachment #232125 - Attachment is obsolete: true
Attachment #232594 - Flags: review?(mark)
Attachment #232125 - Flags: review?(mark)

Comment 12

11 years ago
Comment on attachment 232594 [details] [diff] [review]
Build system changes, rev. 1.1

These changes look OK then, but I haven't tested this by tracking down the dependencies.  I trust that you have.
Attachment #232594 - Flags: review?(mark) → review+
(Assignee)

Comment 13

11 years ago
This was checked in, but is going to be backed out tomorrow morning due to linux perf regressions and a mysterious balsa orange.

Comment 14

11 years ago
regression 080904-080907 [cairo].

"Recently closed Tabs" is gray, and "Undo close tab" on tab-bar disappears.
+/- cursor disappears when mouse-over on auto-resized image. 

are these ceused by this checkin ?
Feedview is broken.
Building with --disable-libxul on OS X gives me the following error in mozilla/browser/app:

c++ -o firefox-bin -I/usr/X11R6/include -fno-rtti -fno-exceptions -Wall -Wconversion -Wpointer-arith -Wcast-align -Woverloaded-virtual -Wsynth -Wno-ctor-dtor-privacy -Wno-non-virtual-dtor -Wno-long-long -isysroot /Developer/SDKs/MacOSX10.3.9.sdk -fpascal-strings -no-cpp-precomp -fno-common -fshort-wchar -I/Developer/SDKs/MacOSX10.3.9.sdk/Developer/Headers/FlatCarbon -pipe  -DDEBUG -D_DEBUG -DDEBUG_peterv -DTRACING -g  nsBrowserApp.o            -L../../dist/bin -L../../dist/lib  -L../../dist/bin -lmozjs -L../../dist/bin -lxpcom -lxpcom_core -L../../dist/lib -lplds4 -lplc4 -lnspr4 -framework Cocoa -framework Carbon       
/usr/bin/ld: Undefined symbols:
_XRE_main
collect2: ld returned 1 exit status

Comment 17

11 years ago
This may have caused Firefox to crash on startup, please see bug 348044.
(Assignee)

Updated

11 years ago
Depends on: 348212

Comment 18

11 years ago
Now building with --enable-libxul does not build with libxul.

Comment 19

11 years ago
*** Bug 349761 has been marked as a duplicate of this bug. ***

Comment 20

11 years ago
This bug is responsible for creating an API inconsistency.

MOZILLA_INTERNAL_API's {String}::AssignLiteral is for literal constants not const pointers.

The flash frozen instance in glue unfortunately accepts a simple const pointer. As a result people are now confused by what "AssignLiteral" means and it is no longer properly portable.

I don't have any recourse other than to say FLASH FREEZING IS BAD. EVEN WHEN YOU THINK YOU ARE NOT FLASH FREEZING.
(Assignee)

Comment 21

11 years ago
The glue is not API-frozen. It is safe to link against, but the APIs may change in the future.
(Assignee)

Comment 22

11 years ago
Created attachment 244859 [details] [diff] [review]
Update patch, makes browser components use frozen linkage without libxul, rev. 1

This patch is mainly an updated version of the last patches, with some updates for changed trunk code and fixing some regressions I caused last time.
Attachment #244859 - Flags: review?(mano)
Comment on attachment 244859 [details] [diff] [review]
Update patch, makes browser components use frozen linkage without libxul, rev. 1

browser/ changes look OK. please file a bug on  porting bakc EscapeHTML.
Attachment #244859 - Flags: review?(mano) → review+
So... something about this change made string adoption confused _somehow_.  See the leak stats on balsa.
In particular, here's the interesting part from the debug output in the log:

  nsStringStats
...
   => mAdoptCount:           2303
   => mAdoptFreeCount:       2309

That second number should be no greater than the first number...
I've closed the tree until we have reasonable leak numbers on balsa again.
Is it possible that some timing changed somewhere so now we're adopting some strings before we really start keeping track of things?

Comment 28

11 years ago
backing out so we can reopen the tree.
(Assignee)

Comment 29

11 years ago
Created attachment 245232 [details] [diff] [review]
Fix string adoptcount logging, rev. 1
Attachment #245232 - Flags: review?
(Assignee)

Updated

11 years ago
Attachment #245232 - Flags: review? → review?(darin.moz)

Updated

11 years ago
Attachment #245232 - Flags: review?(darin.moz) → review+
(Assignee)

Comment 30

11 years ago
attachment 245232 [details] [diff] [review] committed on trunk. Relanding the big patch will have to wait until Monday.
Is it just me, or was the string change a 165KB codesize win for Firefox and a 250KB codesize win for Seamonkey?

Did taking this constructor out of line really help that much, or is codesighs confused?  It's claiming small (from 20 to 500 byte) decreases in all sorts of functions all through the codebase.
(Assignee)

Comment 32

11 years ago
It was indeed a codesize win... inlining apparently cost a lot in this case.
So bug 188224 is still valid with the new string code...

Comment 34

11 years ago
see also bug 265534, bug 235104

Comment 35

11 years ago
Looking at the tinderboxes it is just me (hmm, although the OS/2 nightlies are also garbled and just 529K downloads), but somehow attachment 245232 [details] [diff] [review] seems to have caused compile errors like these:

make.exe[6]: Entering directory `m:/trunk/fx_thebes/xpcom/string/src'
nsDependentString.cpp
In file included from ../../../dist/include/string/nsSubstring.h:60,
                 from ../../../dist/include/string/nsString.h:44,
                 from ../../../dist/include/string/nsDependentString.h:43,
                 from G:/trunk/mozilla/xpcom/string/src/nsDependentString.cpp:40:
../../../dist/include/string/nsTSubstring.h:501: error: parse error before `*' token
In file included from ../../../dist/include/string/nsSubstring.h:66,
                 from ../../../dist/include/string/nsString.h:44,
                 from ../../../dist/include/string/nsDependentString.h:43,
                 from G:/trunk/mozilla/xpcom/string/src/nsDependentString.cpp:40:
../../../dist/include/string/nsTSubstring.h:501: error: parse error before `*' token
In file included from ../../../dist/include/string/nsDependentSubstring.h:48,
                 from ../../../dist/include/string/nsString.h:48,
                 from ../../../dist/include/string/nsDependentString.h:43,
                 from G:/trunk/mozilla/xpcom/string/src/nsDependentString.cpp:40:
../../../dist/include/string/nsTDependentSubstring.h: In constructor `nsDependentSubstring::nsDependentSubstring(const PRUnichar*, const PRUnichar*)':
../../../dist/include/string/nsTDependentSubstring.h:74: error: no matching function for call to `nsSubstring::nsSubstring(PRUnichar*&, int, nsSubstring::<anonymous enum>)'
../../../dist/include/string/nsTSubstring.h:542: error: candidates are: nsSubstring::nsSubstring(const nsSubstring&)
../../../dist/include/string/nsTSubstring.h:533: error: nsSubstring::nsSubstring(unsigned int)
../../../dist/include/string/nsTSubstring.h:521: error: nsSubstring::nsSubstring()
../../../dist/include/string/nsTSubstring.h:485: error: nsSubstring::nsSubstring(const nsSubstringTuple&)
[... etc ...]

This is an OS/2 trunk build from a freshly checked out tree, but as I don't see anything OS/2 specific in the patch I am a bit lost... Any ideas?

Comment 36

11 years ago
It was definitely the TSubstring.h change that broke us. No idea why yet. I'm going to do some preprocessor output to see what I can figure out.
(Assignee)

Comment 37

11 years ago
Attachment 244859 [details] [diff] committed on trunk. Actually switching to use libxul by default will happen (again) after 1.9alpha1.

Comment 38

11 years ago
(In reply to comment #36)
> It was definitely the TSubstring.h change that broke us. No idea why yet. I'm
> going to do some preprocessor output to see what I can figure out.

Perhaps our OS/2 GCC cannot handle "__attribute__((dllexport))" in front of constructors? Removing NS_COM in front of nsTSubstring_CharT solves the problem. (I am wondering why that is there anyway, the first constructor does not have that decoration.)
(Assignee)

Comment 39

11 years ago
That is there because the other is an inline function, and this is not. What is the syntax for marking a constructor dllexport on OS/2?

Comment 40

11 years ago
I don't know, but perhaps Andy does (or can find out).
I talked to Knut and he saie:
#define __declspec(a) __attribute__((a))
BirdWrk>	that's how __declspec is implement, so we should support that dllexport attribute.

Updated

11 years ago
Depends on: 360662
Hmm, that would only apply to gcc 3.3.5.  Knut would like a testcase but I am not entirely certain what it is doing so not sure how to create one.
(Assignee)

Comment 43

11 years ago
Created attachment 245588 [details] [diff] [review]
Fix a couple more nonfrozen imports, rev. 1
Attachment #245588 - Flags: review?(gavin.sharp)
Attachment #245588 - Flags: review?(gavin.sharp) → review+
The actual landing increased codesize by almost 80KB, with lots of changes to lots of functions...  mostly it looks like libbrowsercomps.so got bigger by 245KB and libbrowserdirprovider.so got bigger by 33KB, while firefox-bin got smaller by 200KB.

Is that about as expected?
(Assignee)

Comment 45

11 years ago
Yes, that is expected. It was more than offset by the string uninlining ;-)
Peter put together a testcase and Knut found that the problem we are having on OS/2 is that there is a parsing error in the gcc 3.3.x series that was fixed in 3.4.  As we don't have 3.4 is our best option to ifdef and go without the NS_COM there?
(Assignee)

Comment 47

11 years ago
I'd be a little surprised if it linked without NS_COM... it is an imported symbol.
Created attachment 246530 [details] [diff] [review]
Fixes build failure on OS/2

As far as I know the ifdef may not be required but I can't test on other systems so I put the ifdef in until/unless someone else tests and confirms it is not necessary.
Got the idea here:
http://gcc.gnu.org/ml/gcc-prs/2002-12/msg00672.html
Setting Mike for Review but not sure who to set for SuperReview.
Attachment #246530 - Flags: review?(mozilla)
Created attachment 246540 [details] [diff] [review]
OS/2 build break patch... including comment
[Checked into trunk]
Attachment #246530 - Attachment is obsolete: true
Attachment #246540 - Flags: review?
Attachment #246530 - Flags: review?(mozilla)

Updated

11 years ago
Attachment #246540 - Flags: review? → review?(mozilla)

Comment 50

11 years ago
should we do a GCC 3 check instead of an OS/2 check?
Well, we can use #if HAVE_GCC_VERSION(3,3) I think.  No one else has apparently seen this (or at least not reported it that I have seen) so I went with the XP_OS2.  I just didn't want to change anyone else if they were working as is, though it should affect at least 3.2 and 3.3 for everyone.
Ok, the previous was wrong... it was pointed out on IRC that that would check at _least_ and not exact.  #if (__GNUC__ == 3 && __GNUC_MINOR__ < 4) should work.
Mike, I haven't tested this with 3.2.2 though it should work fine as far as I can tell.

Updated

11 years ago
Attachment #246540 - Flags: review?(mozilla) → review?(benjamin)
(Assignee)

Updated

11 years ago
Attachment #246540 - Flags: review?(benjamin) → review+

Comment 53

10 years ago
(In reply to comment #52)
> Ok, the previous was wrong... it was pointed out on IRC that that would check
> at _least_ and not exact.  #if (__GNUC__ == 3 && __GNUC_MINOR__ < 4) should
> work.
> Mike, I haven't tested this with 3.2.2 though it should work fine as far as I
> can tell.

Somehow I missed this and I see that no fix for the OS/2 problem was checked in yet. Do you want to make a new patch with the GCC test? Or should I check in the one with XP_OS2 that has r+?

Comment 54

10 years ago
Let's checkin the one with r+ to get us building again.

Comment 55

10 years ago
Comment on attachment 246540 [details] [diff] [review]
OS/2 build break patch... including comment
[Checked into trunk]

OK, done. (Works fine here with GCC 3.2.2, too.)
Attachment #246540 - Attachment description: OS/2 build break patch... including comment → OS/2 build break patch... including comment [Checked into trunk]
Depends on: 364042
(Assignee)

Updated

10 years ago
Priority: -- → P2
Target Milestone: Firefox 3 alpha1 → Firefox 3 alpha3
(Assignee)

Updated

10 years ago
Blocks: 326925

Comment 56

10 years ago
wtf? why was this enabled for debug builds?  Having to rebuild/relink libxul takes at least 7 minutes on my windows box.  This needs to be backed out.

On a side note, why is thebes part of this?  We said all along that thebes should not be part of libxul.
(Assignee)

Comment 57

10 years ago
This is enabled as the default config because we want the default onfig to math the release config. Per dbaron and I tend to agree options like --enable-debug should not have side-effects.

As for thebes, it has dependencies on xpom and internal linkage, and thus has to live in libxul until those dependencies are broken. But I think it should not be exposed in any case, beause it's not safe to link against in any case because of C++RT mixing.
(Assignee)

Updated

10 years ago
Status: NEW → RESOLVED
Last Resolved: 10 years ago
Resolution: --- → FIXED

Comment 58

10 years ago
Imho we should just completely disable libxul across the board if that is your stance.  Leaving it on is just going to cause all developers to disable it which isn't going to get you the coverage that you want.
(In reply to comment #57)
> This is enabled as the default config because we want the default onfig to math
> the release config. Per dbaron and I tend to agree options like --enable-debug
> should not have side-effects.

Normally I'd agree, but in this case, doing a debug build without doing disable-libxul makes it quite impossible to do any work on any C++ code.  I think the number of people who want a debug build with libxul are much smaller.  We already have everyone needing to build with --disable-installer, which is just as bad; let's not add another magic option that you have to build with to be able to do development at a layer lower than toolkit.
(Assignee)

Comment 60

10 years ago
I fully support somebody coming up with a --enable-developer-mode meta-flag, but that's not this bug.

Updated

10 years ago
Depends on: 384267

Updated

10 years ago
Depends on: 384269

Updated

10 years ago
Depends on: 384831

Comment 61

10 years ago
This apparently caused critical a11y bug 382626.
Depends on: 392626

Comment 62

10 years ago
Sorry, I meant bug 392626, but bug 382626 is probably a dupe since it shows the same symptoms.
Depends on: 368464
Just one question: Is the ifdef check for MOZ_META_COMPONENT in the code under http://lxr.mozilla.org/seamonkey/source/config/config.mk#288 still needed? It was checked in as part of this bug here, but MOZ_META_COMPONENT is no longer(?) defined anywhere in the code, see http://mxr.mozilla.org/mozilla/search?string=MOZ_META_COMPONENT.
(Assignee)

Comment 64

9 years ago
The meta-component bits got removed during an iteration of this patch, so that ifdef is unnecessary.
Blocks: 451918
You need to log in before you can comment on or make changes to this bug.