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

Status: RESOLVED FIXED

(Firefox Build System :: General, defect, P2)

Description

[Benjamin Smedberg]

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.

Comment 1

[Benjamin Smedberg]

Attachment: Glue additions, rev. 1 [checked in]

Comment 2

[Benjamin Smedberg]

Attachment: Browser changes, rev. 1

Comment 3

[Benjamin Smedberg]

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

[Darin Fisher]

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

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

  return !*aASCIIString;

Comment 5

[Benjamin Smedberg]

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.

Comment 6

[Benjamin Smedberg]

Attachment: 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!

Comment 7

[Benjamin Smedberg]

Attachment: 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

Comment 8

[O. Atsushi (Torisugari)]

(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

Comment 9

[Benjamin Smedberg]

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

[Mark Mentovai]

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&)

Comment 11

[Benjamin Smedberg]

Attachment: 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).

Comment 12

[Mark Mentovai]

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.

Comment 13

[Benjamin Smedberg]

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

[pal-moz]

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 ?

Comment 15

[Ria Klaassen (not reading all bugmail)]

Feedview is broken.

Comment 16

[Peter Van der Beken [:peterv]]

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

[Jon Henry]

This may have caused Firefox to crash on startup, please see bug 348044.

Comment 18

[Simon Howes]

Now building with --enable-libxul does not build with libxul.

Comment 19

[Walter Meinl]

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

Comment 20

[timeless]

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.

Comment 21

[Benjamin Smedberg]

The glue is not API-frozen. It is safe to link against, but the APIs may change in the future.

Comment 22

[Benjamin Smedberg]

Attachment: 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.

Comment 23

[Asaf Romano (gone)]

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.

Comment 24

[Boris Zbarsky [:bzbarsky]]

So... something about this change made string adoption confused _somehow_.  See the leak stats on balsa.

Comment 25

[Boris Zbarsky [:bzbarsky]]

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...

Comment 26

[Boris Zbarsky [:bzbarsky]]

I've closed the tree until we have reasonable leak numbers on balsa again.

Comment 27

[Boris Zbarsky [:bzbarsky]]

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

[Stuart Parmenter]

backing out so we can reopen the tree.

Comment 29

[Benjamin Smedberg]

Attachment: Fix string adoptcount logging, rev. 1

Comment 30

[Benjamin Smedberg]

attachment 245232 [details] [diff] [review] committed on trunk. Relanding the big patch will have to wait until Monday.

Comment 31

[Boris Zbarsky [:bzbarsky]]

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.

Comment 32

[Benjamin Smedberg]

It was indeed a codesize win... inlining apparently cost a lot in this case.

Comment 33

[David Baron :dbaron:]

So bug 188224 is still valid with the new string code...

Comment 34

[Bernard Alleysson]

see also bug 265534, bug 235104

Comment 35

[Peter Weilbacher]

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

[Mike Kaply [:mkaply]]

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.

Comment 37

[Benjamin Smedberg]

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

Comment 38

[Peter Weilbacher]

(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.)

Comment 39

[Benjamin Smedberg]

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

[Peter Weilbacher]

I don't know, but perhaps Andy does (or can find out).

Comment 41

[Andy Willis (abwillis)]

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.

Comment 42

[Andy Willis (abwillis)]

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.

Comment 43

[Benjamin Smedberg]

Attachment: Fix a couple more nonfrozen imports, rev. 1

Comment 44

[Boris Zbarsky [:bzbarsky]]

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?

Comment 45

[Benjamin Smedberg]

Yes, that is expected. It was more than offset by the string uninlining ;-)

Comment 46

[Andy Willis (abwillis)]

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?

Comment 47

[Benjamin Smedberg]

I'd be a little surprised if it linked without NS_COM... it is an imported symbol.

Comment 48

[Andy Willis (abwillis)]

Attachment: 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.

Comment 49

[Andy Willis (abwillis)]

Attachment: OS/2 build break patch... including comment [Checked into trunk]

Comment 50

[Mike Kaply [:mkaply]]

should we do a GCC 3 check instead of an OS/2 check?

Comment 51

[Andy Willis (abwillis)]

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.

Comment 52

[Andy Willis (abwillis)]

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.

Comment 53

[Peter Weilbacher]

(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

[Mike Kaply [:mkaply]]

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

Comment 55

[Peter Weilbacher]

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.)

Comment 56

[Stuart Parmenter]

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.

Comment 57

[Benjamin Smedberg]

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.

Comment 58

[Stuart Parmenter]

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.

Comment 59

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

(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.

Comment 60

[Benjamin Smedberg]

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

Comment 61

[Aaron Leventhal]

This apparently caused critical a11y bug 382626.

Comment 62

[Aaron Leventhal]

Sorry, I meant bug 392626, but bug 382626 is probably a dupe since it shows the same symptoms.

Comment 63

[Frank Wein [:mcsmurf]]

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.

Comment 64

[Benjamin Smedberg]

The meta-component bits got removed during an iteration of this patch, so that ifdef is unnecessary.