235381 - 2004-02-23-12-trunk Solaris nightly crashes at nsComponentManagerImpl::GetLoaderForType()

Status: RESOLVED FIXED

(SeaMonkey :: Build Config, defect)

Description

[Roland Mainz]

2004-02-23-12-trunk Solaris nightly compiled with Sun Workshop 8 crashes at
nsComponentManagerImpl::GetLoaderForType(). Both Mozilla and regxpcom crashes
like this:
-- snip --
% ./run-mozilla.sh -g -d dbx ./regxpcom 
MOZILLA_FIVE_HOME=.
 
LD_LIBRARY_PATH=.:./plugins:.:/usr/local/staden/lib/solaris-binaries:/usr/local/lib:/usr/local/rvplayer5.0:/usr/local/netpbm/lib:/usr/local/arb/lib
DISPLAY=meridian:0.0
DYLD_LIBRARY_PATH=.:.
     LIBRARY_PATH=.:./components:.
       SHLIB_PATH=.:.
          LIBPATH=.:.
       ADDON_PATH=.
      MOZ_PROGRAM=./regxpcom
      MOZ_TOOLKIT=
        moz_debug=1
     moz_debugger=dbx
/opt/SUNWspro/bin/dbx ./regxpcom 
For information about new features see `help changes'
To remove this message, put `dbxenv suppress_startup_message 7.1' in your .dbxrc
Reading regxpcom
Reading ld.so.1
Reading libpthread.so.1
Reading libthread.so.1
Reading libdl.so.1
Reading libc.so.1
Reading libc_psr.so.1
detected a multithreaded program
(dbx) run
Running: regxpcom 
(process id 15345)
Reading libCrun.so.1
Reading libw.so.1
Reading libnspr4.so
Reading librt.so.1
Reading libsocket.so.1
Reading libnsl.so.1
Reading libaio.so.1
Reading libmp.so.2
Reading libnspr_flt4.so
Reading libplds4.so
Reading libplc4.so
Reading libm.so.1
Reading libCstd.so.1
Reading libCstd_isa.so.1
Reading libxpcom.so
Reading libdemangle.so.1
Reading en_US.ISO8859-1.so.2
Reading ISO8859-1%UTF-16BE.so
Reading UTF-16BE%ISO8859-1.so
Reading libxpcom_compat_c.so
Reading libxpcom_compat.so
Reading libxpconnect.so
Reading libmozjs.so
Reading libuconv.so
Reading libucvmath.so
Reading libi18n.so
Reading libctl.so
Reading libjar50.so
Reading liboji.so
Reading libipcdc.so
Reading libnecko.so
Reading libnecko2.so
Reading libpref.so
Reading libcaps.so
Reading librdf.so
Reading libhtmlpars.so
Reading libgfxxprint.so
Reading libgkgfx.so
Reading libgfx_gtk.so
Reading libgtk-1.2.so.0
Reading libgmodule-1.2.so.0
Reading libglib-1.2.so.0
Reading libXext.so.0
Reading libX11.so.4
Reading libgdk-1.2.so.0
Reading libimglib2.so
Reading libgkplugin.so
Reading libjsdom.so
Reading libwidget_gtk.so
Reading libxremote_client.so
Reading libgklayout.so
Reading libmork.so
Reading libdocshell.so
Reading libprofile.so
Reading libnsprefm.so
Reading libembedcomponents.so
Reading libwebbrwsr.so
Reading libeditor.so
Reading libtxmgr.so
Reading libcomposer.so
Reading libnsappshell.so
Reading libchrome.so
Reading libaccessibility.so
Reading libmozldap.so
Reading libpipboot.so
Reading libpipnss.so
Reading libssl3.so
Reading libnss3.so
Reading libsoftokn3.so
Reading libpippki.so
Reading libfileview.so
Reading libmozfind.so
Reading libxremoteservice.so
Reading libappcomps.so
Reading libxpinstall.so
Reading libjsd.so
Reading libcookie.so
Reading libwallet.so
Reading libwalletviewers.so
Reading libxmlextras.so
Reading libp3p.so
Reading libautoconfig.so
Reading libtransformiix.so
Reading libinspector.so
Reading libuniversalchardet.so
Reading libtypeaheadfind.so
Reading libwebsrvcs.so
Reading libspellchecker.so
Reading libmyspell.so
Reading libxmlterm.so
Reading libnkdatetime.so
Reading libnkfinger.so
Reading libgkdebug.so
Reading libmailnews.so
Reading libmsgdb.so
Reading libmsgnews.so
Reading libmsgbaseutil.so
Reading liblocalmail.so
Reading libmimeemitter.so
Reading libvcard.so
Reading libmime.so
Reading libmsgcompose.so
Reading libmsgimap.so
Reading libaddrbook.so
Reading libimport.so
Reading libimpText.so
Reading libimpComm4xMail.so
Reading libmsgmdn.so
Reading libmailview.so
Reading libbayesflt.so
Reading libmsgsmime.so
Reading libxpical.so
t@1 (l@1) signal SEGV (no mapping at the fault address) in
nsComponentManagerImpl::GetLoaderForType at 0xfe42147c
0xfe42147c: GetLoaderForType+0x00f0:    ld      [%o0], %o3
(dbx) where
current thread: t@1
=>[1] nsComponentManagerImpl::GetLoaderForType(0x51510, 0x10, 0xffbeeab8, 0x2,
0x35c, 0xfe421638), at 0xfe42147c 
  [2] nsComponentManagerImpl::AutoRegisterImpl(0x51510, 0xffbeeabc, 0x26da0,
0xffbeeb34, 0x80000000, 0xffbeeb38), at 0xfe422170 
  [3] NS_InitXPCOM2(0xfe49510c, 0xffbeebf8, 0x2a208, 0xffbeec1c, 0x51510,
0xffbeebc8), at 0xfe3d3a2c 
  [4] startup_xpcom(0x0, 0x2a208, 0x154bc, 0x26d48, 0x26840, 0x0), at 0x128b8 
  [5] main(0x1, 0xffbeed74, 0xffbeed7c, 0x137d8, 0x0, 0x0), at 0x1307c 
(dbx) quit
-- snip --

Comment 1

[Roland Mainz]

Fully-featured stack trace:
-- snip --
Reading libbayesflt.so
Reading libmsgsmime.so
Reading libxpical.so
t@1 (l@1) signal SEGV (no mapping at the fault address) in
nsComponentManagerImpl::GetLoaderForType at line 2906 in file
"nsComponentManager.cpp"
 2906       rv = loader->Init(this, nsnull);
(dbx) print -r loader
loader = {
    nsCOMPtr<nsIComponentLoader>::nsCOMPtr_base::mRawPtr = (nil)
}
(dbx) print -r *this
*this = {
    nsComponentManagerImpl::nsSupportsWeakReference::mProxy = (nil)
    nsComponentManagerImpl::mRefCnt                = {
        nsComponentManagerImpl::nsAutoRefCnt::mValue = 4U
    }
    nsComponentManagerImpl::mFactories             = {
        nsComponentManagerImpl::PLDHashTable::ops          = 0xfe4c4fe8
        nsComponentManagerImpl::PLDHashTable::data         = (nil)
        nsComponentManagerImpl::PLDHashTable::hashShift    = 21
        nsComponentManagerImpl::PLDHashTable::maxAlphaFrac = 'à'
        nsComponentManagerImpl::PLDHashTable::minAlphaFrac = '@'
        nsComponentManagerImpl::PLDHashTable::entrySize    = 8U
        nsComponentManagerImpl::PLDHashTable::entryCount   = 929U
        nsComponentManagerImpl::PLDHashTable::removedCount = 0
        nsComponentManagerImpl::PLDHashTable::generation   = 1U
        nsComponentManagerImpl::PLDHashTable::entryStore   = 0xaf5f0 ""
    }
    nsComponentManagerImpl::mContractIDs           = {
        nsComponentManagerImpl::PLDHashTable::ops          = 0xfe4c500c
        nsComponentManagerImpl::PLDHashTable::data         = (nil)
        nsComponentManagerImpl::PLDHashTable::hashShift    = 21
        nsComponentManagerImpl::PLDHashTable::maxAlphaFrac = 'À'
        nsComponentManagerImpl::PLDHashTable::minAlphaFrac = '@'
        nsComponentManagerImpl::PLDHashTable::entrySize    = 16U
        nsComponentManagerImpl::PLDHashTable::entryCount   = 977U
        nsComponentManagerImpl::PLDHashTable::removedCount = 0
        nsComponentManagerImpl::PLDHashTable::generation   = 1U
        nsComponentManagerImpl::PLDHashTable::entryStore   = 0xa4a70 ""
    }
    nsComponentManagerImpl::mMon                   = 0x5ae00
    nsComponentManagerImpl::mNativeComponentLoader = 0x5ae78
    nsComponentManagerImpl::mStaticComponentLoader = 0x5bcc8
    nsComponentManagerImpl::mComponentsDir         = {
        nsComponentManagerImpl::nsCOMPtr<nsIFile>::nsCOMPtr_base::mRawPtr =
0x5fd10
    }
    nsComponentManagerImpl::mComponentsOffset      = 74
    nsComponentManagerImpl::mGREComponentsDir      = {
        nsComponentManagerImpl::nsCOMPtr<nsIFile>::nsCOMPtr_base::mRawPtr =
0x5ff00
    }
    nsComponentManagerImpl::mGREComponentsOffset   = 74
    nsComponentManagerImpl::mRegistryFile          = {
        nsComponentManagerImpl::nsCOMPtr<nsIFile>::nsCOMPtr_base::mRawPtr =
0x601c0
    }
    nsComponentManagerImpl::mShuttingDown          = 0
    nsComponentManagerImpl::mLoaderData            = 0x5bc90
    nsComponentManagerImpl::mNLoaderData           = 3
    nsComponentManagerImpl::mMaxNLoaderData        = 6
    nsComponentManagerImpl::mRegistryDirty         = 1
    nsComponentManagerImpl::mAutoRegEntries        = {
        nsComponentManagerImpl::nsHashtable::mLock        = (nil)
        nsComponentManagerImpl::nsHashtable::mHashtable   = {
            nsComponentManagerImpl::nsHashtable::PLDHashTable::ops          =
0xfe4c1e30
            nsComponentManagerImpl::nsHashtable::PLDHashTable::data         =
(nil)
            nsComponentManagerImpl::nsHashtable::PLDHashTable::hashShift    = 28
            nsComponentManagerImpl::nsHashtable::PLDHashTable::maxAlphaFrac =
'À'
            nsComponentManagerImpl::nsHashtable::PLDHashTable::minAlphaFrac =
'@'
            nsComponentManagerImpl::nsHashtable::PLDHashTable::entrySize    =
12U
            nsComponentManagerImpl::nsHashtable::PLDHashTable::entryCount   = 1U
            nsComponentManagerImpl::nsHashtable::PLDHashTable::removedCount = 0
            nsComponentManagerImpl::nsHashtable::PLDHashTable::generation   = 0
            nsComponentManagerImpl::nsHashtable::PLDHashTable::entryStore   =
0x54a68 ""
        }
        nsComponentManagerImpl::nsHashtable::mEnumerating = 0
    }
    nsComponentManagerImpl::mCategoryManager       = {
       
nsComponentManagerImpl::nsCOMPtr<nsCategoryManager>::nsCOMPtr_base::mRawPtr =
0x54b30
    }
    nsComponentManagerImpl::mArena                 = {
        nsComponentManagerImpl::PLArenaPool::first     = {
            nsComponentManagerImpl::PLArenaPool::PLArena::next  = 0x603b0
            nsComponentManagerImpl::PLArenaPool::PLArena::base  = 346712U
            nsComponentManagerImpl::PLArenaPool::PLArena::limit = 346712U
            nsComponentManagerImpl::PLArenaPool::PLArena::avail = 346712U
        }
        nsComponentManagerImpl::PLArenaPool::current   = 0xacf58
        nsComponentManagerImpl::PLArenaPool::arenasize = 8192U
        nsComponentManagerImpl::PLArenaPool::mask      = 7U
    }
    nsComponentManagerImpl::gComponentManager      = 0x54978
}
-- snip --

|loader| seems to be |nsnull| ... ;-(

Comment 2

[Roland Mainz]

Can anyone with a Sun Workshop compiler please make a debug build from todays
CVS trunk and check what may be going on here ?

Comment 3

[Roland Mainz]

Startup of a trunk debug build fails like this:
-- snip --
nsNativeComponentLoader: autoregistering succeeded
nsNativeComponentLoader: autoregistering begins.
nsNativeComponentLoader: autoregistering succeeded
###!!! ASSERTION: Could not AutoRegister GRE components: 'Error', file
../../../mozilla/xpcom/build/nsXPComInit.cpp, line 616
Break: at file ../../../mozilla/xpcom/build/nsXPComInit.cpp, line 616
WARNING: gre: NS_InitXPCOM failed, file
../../../../mozilla/xpcom/glue/standalone/nsXPCOMGlue.cpp, line 441
nsStringStats
 => mAllocCount: 3121
 => mReallocCount: 1302
 => mFreeCount: 3028
 => mShareCount: 5700
 => mAdoptCount: 2624
 => mAdoptFreeCount: 2624
nsStringStats
 => mAllocCount: 3121
 => mReallocCount: 1302
 => mFreeCount: 3028
 => mShareCount: 5700
 => mAdoptCount: 2624
 => mAdoptFreeCount: 2624

execution completed, exit code is 1
-- snip --

Comment 4

[Greg Onufer]

Roland asked that this be added to this bug:

(dbx) print -r loader
loader = {
    nsCOMPtr<nsIComponentLoader>::nsCOMPtr_base::mRawPtr = (nil)
}
(dbx) print aType   
aType = 2
(dbx) print mLoaderData[aType]
mLoaderData[aType] = {
    loader = (nil)
    type   = 0x176ff8 "text/javascript"
}

Comment 5

[timeless]

problem #1:
nsComponentManagerImpl::AddLoaderType returns a index on success or an nsresult
on failure.

Comment 6

[Kyle Yuan]

I'm seeing the same problem.

From the first investigation, I found that

2902     loader = do_GetServiceFromCategory("component-loader",
mLoaderData[aType].type, &rv);
2903     if (NS_FAILED(rv))
2904         return rv;
2905 
2906     rv = loader->Init(this, nsnull);

after line 2902, the rv=NS_ERROR_SERVICE_NOT_FOUND. But
NS_ERROR_SERVICE_NOT_FOUND is defined as 

((nsresult) (((PRUint32)(0)<<31) | ((PRUint32)(1 +0x45)<<16) | ((PRUint32)(22)))
); which equals to 0x460022.

So if (NS_FAILED(rv)) is not true, and program end up with a null pointer access.

I haven't found why the service can be found, I'm just wondering why
NS_ERROR_SERVICE_NOT_FOUND is not a kind of failure?

Comment 7

[Kyle Yuan]

s/can be found/can not be found/

Comment 8

[Kyle Yuan]

I found the problem is: getter_Copies works incorrectly for Sun's compiler.

In getter_Copies's implementation, it construct a nsTGetterCopies_CharT object,
pass nsTGetterCopies_CharT::mData as a char ** argument, then put mData back to
the original nsXPIDLCString object in destructor. But for Sun's compiler, seems
the scope of nsTGetterCopies_CharT object is incorrect - its destructor won't be
called until the function returned.

This is broken since the new string classes landed, ccing darin.

Comment 9

[Kyle Yuan]

->string

Comment 10

[Kyle Yuan]

Attachment: simplified testcase

run this testcase, you will get 
"cstr - test - m=3 - dstr - m=8" on Solaris, and
"cstr - test - dstr - m=8 - m=8" on Linux.

Comment 11

[Roland Mainz]

Kyle Yuan wrote:
> But for Sun's compiler, seems
> the scope of nsTGetterCopies_CharT object is incorrect - its destructor won't 
> be called until the function returned.

It isn't incorrect, just different... :)

kyle:
Can you try the following, please:
% export CC=/opt/SUNWspro/bin/cc CXX=/opt/SUNWspro/bin/CC
CXXFLAGS="-features=tmplife"
% ./configure
% timex nice gmake 2>&1 | tee -a buildlog.log

"-features=tmplife" may solve the problem (but may cause problems elsewhere,,,
;-( ) ...

Comment 12

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

(In reply to comment #11)
> It isn't incorrect, just different... :)

ISO/IEC 14882:1998 (I don't have the 2003 version) is quite clear that it is
incorrect.  Section 12.2, clause 3 says:

  Temporary objects are destroyed as the last step in evaluating the
  full-expression (1.9) that (lexically) contains the piont where they were
  created.  This is true even if that evaluation ends in throwing an exception.

clauses 4 and 5 provide two exceptions to this rule, when the temporary is used
as the initializer for a declarator defining an object (4) and binding
temporaries to references (5), but neither applies here or in similar cases,
especially since clause 5 gives the exception:

  A temporary bound to a reference parameter in a function call (5.2.2)
  persists until the completion of the full expression containing the call.

Comment 13

[Roland Mainz]

(In reply to comment #12)
> (In reply to comment #11)
> > It isn't incorrect, just different... :)
> 
> ISO/IEC 14882:1998 (I don't have the 2003 version) is quite clear that it is
> incorrect.

OK, point for you... :)

I am wondering why Sun Workshop violates the C++ specs in this case. AFAIK the
default settings of CC implement strict ISO 14882 behaviour... except this case.
Why ?

Comment 14

[Kyle Yuan]

yes, I got some feedback from the compiler team, -features=tmplife is the right
option to fix this problem in my test case. I'll try a full build today. 

Comment 15

[Roland Mainz]

Kyle Yuan wrote:
> yes, I got some feedback from the compiler team, -features=tmplife is the 
> right option to fix this problem in my test case. I'll try a full build today.

That's no neccesary anymore, I tried it and the option solved the problem.

What we need now is a patch for configure.in which does TWO things:
1. Add "-features=tmplife" to configure.in (maybe after the "-mt" ?) to both
mozilla configure.in and NSPR conifgure.in
2. Add a check for configure.in based on attachment 142324 [details] which tests whether
the destructors are called correctly (just edit your testcase and let it exit
with any error code != 0 when the destructors are not called correctly).
IMHO "configure" should FAIL if the compiler does not do the right thing here.

Comment 16

[Kyle Yuan]

in comment 11, you said '"-features=tmplife" may solve the problem (but may
cause problems elsewhere,,,', is not true?

Comment 17

[Roland Mainz]

Kyle Yuan wrote:
> in comment 11, you said '"-features=tmplife" may solve the problem (but may
> cause problems elsewhere,,,', is not true?

I have no clue what may be affected by "-features=tmplife". It's one _giant_
knob which changes the internal behaviour of LOTS of stuff in Mozilla (just
think about |nsCOMPtr| and strings which are used nearly everywhere). A quick
check with the debug build compiled with -features=tmplife didn't reveal any
problems but that was only a "start, load URL, close and smile test" :)

I have restarted my nightly build system (with CXXFLAGS="-features=tmplife" set)
which will (hopefully) upload a nightly binary to ftp.mozilla.org in 8-10hours.
Then I and the crew from #solaris will start smoketesting the build and check
whether there are any problems (e.g. my usual tour from i18n browser buster,
mailnews, composer, printing, chatzilla down to calendar).

Comment 18

[Kyle Yuan]

okay, debug build with -features=tmplife also passed the "smile test" for me.

->build config

Comment 19

[Roland Mainz]

Test build URL is
http://ftp.mozilla.org/pub/mozilla.org/mozilla/nightly/2004-02-26-20-trunk/mozilla-sparc-sun-solaris2.8.tar.gz
- happy testing... :)

Comment 20

[Iain MacDonnell]

I just ran into this problem too
(http://forums.mozillazine.org/viewtopic.php?p=440661)

It seems that "tmplife" is off by default by design (for backward-compatibility),
so are we still considering working it into the mozilla configure scripts?

Comment 21

[beanladen]

Here's another bug probably exposed by tmplife: see bug #238379

Comment 22

[Paul Boven]

-features=tmpfile fixed things for me, too (Solaris 9, Sun CC 7.)

Comment 23

[Kyle Yuan]

Attachment: how about this?

Comment 24

[Roland Mainz]

Comment on attachment 145673 [details] [diff] [review]
how about this?

Two issues:
1. NSPR (what about NSS ?) needs to be patched, too. Having differnt CXXFLAGS
which affect C++ temp. object lifetime is BAD
2. There should be a configure.in test based on attachment 142324 [details]. The strict
ANSI C++ behaviour is now required by Mozilla and configure.in should test
compilers for that - otherwise we may end-up in hard-to-find bugs on other
platform/compiler combinations, too.

Comment 25

[Roland Mainz]

s/Having differnt CXXFLAGS which affect C++ temp. object lifetime is BAD/Having
two different ways how the temporary lifetime of C++ objects is handled in one
and the same application is bad. This screams for trouble.../

Comment 26

[Christian :Biesinger (don't email me, ping me on IRC)]

(In reply to comment #24)
> 1. NSPR (what about NSS ?) needs to be patched, too. Having differnt CXXFLAGS
> which affect C++ temp. object lifetime is BAD

NSS is plain C (as is the LDAP C-SDK).

Hmm, NSPR does seem to have some C++ files. Not sure what they're used for... I
don't think mozilla uses them.

Comment 27

[Jens Hatlak (:InvisibleSmiley)]

Is there anything that one (i.e. me) can do about this? As far as I can see,
this is the bug that makes Solaris builds completely unusable and resources to
fix it seem to be rare or it's just hard to find a good way to do it - I don't
know (my skills are limited to building and simple ways of debugging Mozilla).
But if there's anything I could to I'd gladly try. Myself and the students at my
university (where Mozilla is running on Solaris) would be thankful for replacing
the "ancient" 1.7a by a current 1.8a nightly build. :-)

Comment 28

[Roland Mainz]

Jens Hatlak wrote:
> Myself and the students at my
> university (where Mozilla is running on Solaris) would be thankful for 
> replacing the "ancient" 1.7a by a current 1.8a nightly build. :-)

OK, this is VERY _OFFTOPIC_ for this bug... but... current nightlies can be
found under
http://ftp.eu.mozilla.org/pub/mozilla.org/mozilla/nightly/contrib/latest-trunk/
http://ftp.eu.mozilla.org/pub/mozilla.org/firefox/nightly/contrib/latest-trunk/
http://ftp.eu.mozilla.org/pub/mozilla.org/thunderbird/nightly/contrib/latest-trunk/
(the build loop hung for a few days... apologies for that. New GTK2 builds
should occur soon...)

Comment 29

[Andreas Lange]

I've run 1.7 built with the tmplife argument without problems for a few weeks. 

I'll fix a patch for configure based on the testcase tomorrow -  I have
something that needs a bit cleanup first though.

Comment 30

[Jens Hatlak (:InvisibleSmiley)]

(In reply to comment #28)
> OK, this is VERY _OFFTOPIC_ for this bug... but... current nightlies can be
> found under (...)

The thing is that I'm building myself and my Solaris builds crash as described
in this bug (they compile cleanly but crash on startup). I also tried this
tmplife hack but either I failed to apply it correctly or it didn't work.
Strange enough that those contrib builds you mentioned do start beyond this
bug's crash point so I assume you (or whoever does the builds) applied this
bug's patch and it worked. However, both the GTK1 and GTK2 contrib builds crash
on me (GTK1 when opening browser window and GTK2 when displaying the profile
manager) spitting gdk errors but I don't think that's related to this bug.

If this tmplife thing works for you, maybe it also will for me when the patch is
checked in. :-/

Comment 31

[Iain MacDonnell]

FWIW, compiling unmodified trunk source with the tmplife feature has solved
the problem for me (no patches).

I use:

     export CC="cc -fsingle -xO5 -xchip=ultra -xalias_level=basic -xmemalign=8s
-xprefetch -dalign -fns -fsimple=1"
    export CXX="CC -xO2 -xchip=ultra -xprefetch -dalign -fns -fsimple=1
-features=tmplife"

Comment 32

[Christian :Biesinger (don't email me, ping me on IRC)]

(In reply to comment #30)
> tmplife hack but either I failed to apply it correctly or it didn't work.

You'll need to regenerate configure after applying it, by running autoconf.

(In reply to comment #31)
>      export CC="cc -fsingle -xO5 -xchip=ultra -xalias_level=basic -xmemalign=8s
> -xprefetch -dalign -fns -fsimple=1"
>     export CXX="CC -xO2 -xchip=ultra -xprefetch -dalign -fns -fsimple=1
> -features=tmplife"

personally I'd use CFLAGS/CXXFLAGS for compiler flags, rather than changing
$CC/$CXX, although it doesn't really matter.

Comment 33

[Christian :Biesinger (don't email me, ping me on IRC)]

so the question is, what's wrong with the attached patch, why wasn't a review
requested?

Comment 34

[Jens Hatlak (:InvisibleSmiley)]

(In reply to comment #32)
> (In reply to comment #30)
> > tmplife hack but either I failed to apply it correctly or it didn't work.
> 
> You'll need to regenerate configure after applying it, by running autoconf.

OK, that explains a lot. So it's lack of knowledge on my side, sorry. While this
might be obvious for you insiders, I couldn't read that from this bug. I'll try
to rebuild but am quite confident that I'll succeed this time. So please feel
free to check the patch in if it is sufficient to fix this bug.

Comment 35

[Roland Mainz]

Jens Hatlak wrote:
> However, both the GTK1 and GTK2 contrib builds crash
> on me (GTK1 when opening browser window and GTK2 when displaying the profile
> manager) spitting gdk errors but I don't think that's related to this bug.

OFFTOPIC, part 2:
Two things can cause this:
1. Do _NOT_ use GTK+/GDK libraries which were build with gcc together wirth Sun
Workshop binaries - this will result in instability (this is a gcc bug...
GTK+/GDK libraries compiled with Sun Workshop work perfectly with binaries
compiled with gcc or Sun Workshop).
2. Please apply the Solaris patches listed in
http://lxr.mozilla.org/seamonkey/source/xpfe/bootstrap/init.d/moz_patch_checker.dtksh#53
_before_ starting Mozilla/FireFox/Thunderbird the _first_ time.
Newer versions even have a "Solaris patch checker"-script which will _WARN_ you
with a dialog if patches are missing (I added that feature to get rid of the
random bugreports caused due missing OS patches).

Comment 36

[Roland Mainz]

Christian Biesinger wrote:
> so the question is, what's wrong with the attached patch, why wasn't a review
> requested?

Did you read comment #24 ?

Comment 37

[Christian :Biesinger (don't email me, ping me on IRC)]

(In reply to comment #36)
> Did you read comment #24 ?

yes. see comment 26.

Comment 38

[Andreas Lange]

Attachment: configure test based on testcase

Ok, here is a proper configure test based on the testcase. Seems to work fine
with forte7 and gcc3.3 (on linux).

If the test succeeds, nothing happens. If the test fails, it adds the tmplife
flag if using the forte compiler or dies otherwise. I find it very likely
people with have opinions on wording, placement in configure etc. :-)

I guess it comes to if there should be a configure test for this (a version of
this patch) or simply always set tmplife for forte (attachment 145673 [details] [diff] [review]).

Comment 39

[Andreas Lange]

Comment on attachment 147135 [details] [diff] [review]
configure test based on testcase

No response or other complaints in the bug, requesting review. 

The patch introduces a test for the C++ compiler generating correct temporary
object destruct order. This became necessary after the string landing. If
necessary, the patch also adds a flag to get the Sun Forte compiler to behave.

Comment 40

[Benjamin Smedberg]

Comment on attachment 147135 [details] [diff] [review]
configure test based on testcase

The build hackery looks right, I'd like dbaron to have a look at the actual C++
testcase.

Comment 41

[Roland Mainz]

Andreas Lange wrote:
> (From update of attachment 147135 [details] [diff] [review])
> No response or other complaints in the bug, requesting review. 

Erm, I am not that fast... :)
The patch doesn't do the right thing,  "-features=tmplife" should always be ON,
not only for some compilers, otherwise binary compatibility gets broken for
modules compiled with old vs. new compilers.
The compiler check should check _all_ compilers in _general_ whether they
conform to the part of ISO C++ standard (see dbaron's comment #12) which is now
a requirement to run mozilla.

Comment 42

[Andreas Lange]

How do you mean? You can't mean that "-features=tmplife" should always be on,
because gcc will not like that. If you mean that it always should be on for
Forte,  then attachment 145673 [details] [diff] [review] should be fine. However, you said in comment 24
that that wasn't enough and that there should be a test based on the testcase,
so that is what I did.

You want all compilers to be tested. Right, the patch does that. It is not
compiler specific in that sense.

But, if the test fails, it gets specific. When failing, it knows the Sun forte
does the right thing if supplied with "-features=tmplife" so it adds that. It
can't really add it to other compilers and hope it will work, so if it is some
other compiler configure dies with an error. 

I thought it sensible to add the parameter only when the compiler actually
failed (needed it). In case future Forte (SunOne etc) releases/updates follows
the standard, should we still add the parameter when it isn't necessary? If that
is the case, I suspect you want a merge of the two patches in this bug...?
First, always add -features=tmplife for forte and then run the test without any
compiler fixes...?

Comment 43

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 147135 [details] [diff] [review]
configure test based on testcase

I'm not a big fan of the build stuff in this patch, actually.  It seems like it
would be cleaner to add --features=tmplife unconditionally for the forte
compiler (i.e., right after "-xbuiltin=%all"), which lets you make the autoconf
test simpler.  (It's also a better approach if anyone ever does a cross-compile
with this compiler.)

Also, all the comments in this bug refer to "-features", but the patch uses
"--features".  Is there a difference?

Comment 44

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 147135 [details] [diff] [review]
configure test based on testcase

Also, if there are some version of the Forte compiler that pass this test and
some that fail, and this breaks binary compatibility between said versions
(which comment 41 seems to be implying, although I'm not sure if all of those
conditions are known to be true, or even likely to be true), this would also be
a bad approach.

Comment 45

[Andreas Lange]

Attachment: configure.in patch, v2

Moved tmplife as requested by comments, reduced testcase to simply fail and not
try to fix anything.

Comment 46

[Roland Mainz]

David Baron wrote:
> I'm not a big fan of the build stuff in this patch, actually. 

/me , too.

> It seems like 
> it would be cleaner to add --features=tmplife unconditionally for the forte
> compiler (i.e., right after "-xbuiltin=%all"), which lets you make the 
> autoconf test simpler.  (It's also a better approach if anyone ever does a 
> cross-compile with this compiler.)

To clarify the confusion:
My idea was turn "-features=tmplife" "on" for all Sun Workshop/Forte compilers
and add a test after that point which tests _all_ compilers on _all_ plaftorms
whether they conform to the new requirement of the string library.

> Also, all the comments in this bug refer to "-features", but the patch uses
> "--features".  Is there a difference?

I doubt anything with "--*" will work. The Sun compilers use uniformly "-*" as
parameter/argument prefix.

Comment 47

[Andreas Lange]

(In reply to comment #46)
> > I'm not a big fan of the build stuff in this patch, actually. 
> /me , too.
> 
> To clarify the confusion:
> My idea was turn "-features=tmplife" "on" for all Sun Workshop/Forte compilers
> and add a test after that point which tests _all_ compilers on _all_ plaftorms
> whether they conform to the new requirement of the string library.

Ok, then I'm less confused and you seem to agree. :-) 
Is this new patch to your liking?
 
> > Also, all the comments in this bug refer to "-features", but the patch uses
> > "--features".  Is there a difference?
> 
> I doubt anything with "--*" will work. The Sun compilers use uniformly "-*" as
> parameter/argument prefix.

Nah, that was my mistake. Somehow it worked when I tested, but it shouldn't.
Must have messed up somewhere in the patchmaking.

Comment 48

[Roland Mainz]

Andreas wrote:
> (In reply to comment #46)
> > > I'm not a big fan of the build stuff in this patch, actually. 
> > /me , too.
> > 
> > To clarify the confusion:
> > My idea was turn "-features=tmplife" "on" for all Sun Workshop/Forte 
> > compilers
> > and add a test after that point which tests _all_ compilers on _all_ 
> > plaftorms
> > whether they conform to the new requirement of the string library.
>
> Ok, then I'm less confused and you seem to agree. :-) 
> Is this new patch to your liking?

Mhhh... it seems to do what I described in the previous comment, right ?

One minor nit;
-- snip --
+dnl ========================================================
+dnl Test for correct temporary object destruction order
+dnl ========================================================
-- snip --
Please add a reference to this bug and place a comment that this test was added
to ensure that people who use new comilers (or have updated their compiler)
don't run into problems (otherwise this test will likely be removed by accident
in the next big "configure.in"-cleanup... ;-/ ).

Another issue is http://lxr.mozilla.org/nspr/source/nsprpub/configure.in#1687
... I know that NSPR uses plain C but that CXXFLAGS statement has always been
maintained even while it wasn't in use... please keep that in sync with the main
configure.in

Comment 49

[Andreas Lange]

Attachment: Same patch, now with comment

Comment 50

[Andreas Lange]

Attachment: NSPR, keep (unused) CXXFLAGS in sync (checked in; not needed by 1.7)

So, anything else that needs to be done here?

Comment 51

[Kyle Yuan]

Comment on attachment 147895 [details] [diff] [review]
Same patch, now with comment

moving forward dbaron's sr. seeking r= from Roland.

Comment 52

[Roland Mainz]

Comment on attachment 147896 [details] [diff] [review]
NSPR, keep (unused) CXXFLAGS in sync (checked in; not needed by 1.7)

The NSPR part should be go "in" with the main patch... requesting r= ...

Comment 53

[Roland Mainz]

Comment on attachment 147895 [details] [diff] [review]
Same patch, now with comment

The patch is OK for me, r=[e-mail]

Comment 54

[Wan-Teh Chang]

Comment on attachment 147896 [details] [diff] [review]
NSPR, keep (unused) CXXFLAGS in sync (checked in; not needed by 1.7)

r=wtc.

This bug report is very long, so I need
Darin or Kyle's help to answer this question:

(Please ignore the fact that NSPR and NSS
currently don't have C++ code.)

Does the Sun compiler team recommend that
we use -features=tmplife to compile all C++
code?

Comment 55

[Darin Fisher]

wtc: i defer to kyle.

Comment 56

[Darin Fisher]

Comment on attachment 147896 [details] [diff] [review]
NSPR, keep (unused) CXXFLAGS in sync (checked in; not needed by 1.7)

sr=darin

Comment 57

[Wan-Teh Chang]

Comment on attachment 147896 [details] [diff] [review]
NSPR, keep (unused) CXXFLAGS in sync (checked in; not needed by 1.7)

I checked in this patch on the NSPR tip (NSPR 4.6)
and NSPRPUB_PRE_4_2_CLIENT_BRANCH (Mozilla 1.8 alpha).

Comment 58

[Kyle Yuan]

(In reply to comment #54)
> Does the Sun compiler team recommend that
> we use -features=tmplife to compile all C++
> code?
> 

Yes, they do. They do not turn -features=tmplife on by default just for the
backward compatibility.

Comment 59

[Kyle Yuan]

Attachment 147895 [details] [diff] has been checked in trunk. Mark fixed.

Comment 60

[Andreas Lange]

Thanks for getting this into CVS!

Have there been any problems from this? Atleast I haven't seen any. Otherwise,
if it isn't too late, I think this ought to get into 1.7 as well. The 1.7 branch
is compilable but unusable (unless you know about this) at Solaris currently.

Comment 61

[Kyle Yuan]

Comment on attachment 147895 [details] [diff] [review]
Same patch, now with comment

Agree. Since 1.7 is a long-term stable branch, we need to get rid of this
obstacle.

Comment 62

[chris hofmann]

Comment on attachment 147895 [details] [diff] [review]
Same patch, now with comment

a=chofmann for 1.7

Comment 63

[chris hofmann]

Comment on attachment 147896 [details] [diff] [review]
NSPR, keep (unused) CXXFLAGS in sync (checked in; not needed by 1.7)

a=chofmann for 1.7

Comment 64

[Wan-Teh Chang]

Comment on attachment 147896 [details] [diff] [review]
NSPR, keep (unused) CXXFLAGS in sync (checked in; not needed by 1.7)

Do not check this NSPR patch into
the MOZILLA_1_7_BRANCH.  This NSPR
patch modifies dead code and is for
informational purposes only.

Comment 65

[Kyle Yuan]

Comment on attachment 147895 [details] [diff] [review]
Same patch, now with comment

fixed in 1.7 branch.

Comment 66

[beanladen]

In reply to #60:

This caused regression bug #238379 . There's possibly a hidden problem in the
NTLM code exposed by -tmplife. All Solaris builds are now unusable if you have 
to deal with NTLM authentication (mail servers, protected web pages).

Comment 67

[Asa Dotzler [:asa]]

We need to decide, if we can't sort out the regression, which broken behavior we
want to live with for 1.7.

Comment 68

[Roland Mainz]

[e-mail] wrote:
> In reply to #60:
> This caused regression bug #238379 . There's possibly a hidden problem in the
> NTLM code exposed by -tmplife. All Solaris builds are now unusable if you have 
> to deal with NTLM authentication (mail servers, protected web pages).

I strongly doubt that this issue is related to the -tmplife flag at all. A few
hundred patches have been checked in in the meantime into the tree and the
"-tmpfile" was only one of them.

Comment 69

[Andreas Lange]

Bug 245216 opened for the issue that the 1.7-branch's configure did not get
automatically updated for the new configure.in (and thus remains broken).

Comment 70

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

As far as I can tell, this bug is fixed, but it caused regression bug 238379. 
Changing status to reflect that.

Comment 71

[Roland Mainz]

David Baron wrote:
> As far as I can tell, this bug is fixed, but it caused regression bug 238379. 
> Changing status to reflect that.

bug 238379 appeared before this was checked-in. That bug really has nothing to
do with this one.

Comment 72

[Roland Mainz]

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

Comment 73

[Iain MacDonnell]

Someone going to apply this to the aviary branch? I just built firefox 0.9
without the workaround *sigh*