551254 - Investigate safety of STL containers from code compiled with exceptions off

Status: RESOLVED FIXED

(Core :: XPCOM, defect)

Description

[Zack Weinberg (:zwol)]

We've been assuming that we can't make direct use of the STL because it throws exceptions rather than returning error codes; cjones has been implementing wrappers that e.g. make use of our infallible-malloc support.

However, I observe that GCC's STL (which is what we get on OSX and Linux) can be relied on to call std::terminate, rather than crashing in an unpredictable way, when it wants to throw an exception but it's been called from code compiled with -fno-exceptions.  Thus, if we are confident that all C++ implementations we care about have this behavior (i.e. we need to check MSVC++'s behavior with /EH-) and we are comfortable treating all STL exceptions as fatal, we don't need the wrappers.

Based on a quick skim of C++98, I think these are the exceptions that STL containers and strings can throw:

 bad_alloc   -- malloc failure; if we're ok with all STL container usage being
                abort-on-memory-exhaustion, this is fine
 length_error
 overflow
 invalid_argument -- various conditions amounting to assertion failures, e.g.
                     "you cannot possibly allocate an object of size 2**68".
                     Should be safe to treat as always-fatal.
 out_of_range -- Checked container accessors throw this when the index is
                 out of range for the container.  Should be safe to treat as
                 always-fatal

so I'm comfortable with treating STL exceptions as fatal.  Thoughts?

Comment 1

[Boris Zbarsky [:bzbarsky]]

I'd think we would want to have bad_alloc turned into a memory-pressure notification and the like.  Or would it only happen on malloc failure after our hacked malloc has had a crack at things?

Comment 2

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

There are two issues: logic-error "exceptions" and bad_alloc.  I'm perfectly fine with logic errors being fatal via std::terminate().  bad_alloc I'm not so sure about.

The inchoate plan for memory pressure is to monitor it perhaps with a dedicated thread, set a flag if detected, possibly free some ballast, and possibly notify a callback to trigger cleanup.  It's always possible that malloc() could return NULL before we detect memory pressure.  IMHO it would be nice to attempt reclamation on OOM the same as we would for critical memory (but not OOM), i.e. not std::terminate().  I don't think anyone knows whether this will fly in practice though.

So IMHO, a reasonable option might be using the STL datatypes unwrapped, but included so as to pick up mozalloc ::operator new().

Comment 3

[Zack Weinberg (:zwol)]

My (very limited) prior experience with that kind of scheme, on old-skool Mac System 7, was that it was a nice idea in theory but did not work in practice.  The odds of emergency reclaim freeing enough memory to allow the memory-hungry operation that had provoked allocation failure to complete were very low.  I'd lay money that the same is true today, or possibly even more so.

(One possible exception is when the memory-hungry operation is generating large amounts of cyclic garbage, so its *actual* memory requirement is small but we haven't gotten around to reclaiming yet.  But that might be better addressed by making it possible to run the cycle collector incrementally from *any* malloc.  Similarly for the Javascript GC heap.)

Ensuring that everything gets mozalloc ::operator new seems like a good idea to me regardless.

Comment 4

[(no longer active)]

So, I tried compiling the simplest program which would throw std::out_of_range on MSVC:

#include <vector>

int main() {
  std::vector<int> v;
  v.at(1);
}

And here is what I got:

C:\Users\Ehsan>cl.exe test.cpp
Microsoft (R) 32-bit C/C++ Optimizing Compiler Version 15.00.30729.01 for 80x86
Copyright (C) Microsoft Corporation.  All rights reserved.

test.cpp
C:\Program Files (x86)\Microsoft Visual Studio 9.0\VC\INCLUDE\xstring(2092) : wa
rning C4530: C++ exception handler used, but unwind semantics are not enabled. S
pecify /EHsc
        C:\Program Files (x86)\Microsoft Visual Studio 9.0\VC\INCLUDE\xstring(20
83) : while compiling class template member function 'void std::basic_string<_El
em,_Traits,_Ax>::_Copy(unsigned int,unsigned int)'
        with
        [
            _Elem=char,
            _Traits=std::char_traits<char>,
            _Ax=std::allocator<char>
        ]
        C:\Program Files (x86)\Microsoft Visual Studio 9.0\VC\INCLUDE\stdexcept(
47) : see reference to class template instantiation 'std::basic_string<_Elem,_Tr
aits,_Ax>' being compiled
        with
        [
            _Elem=char,
            _Traits=std::char_traits<char>,
            _Ax=std::allocator<char>
        ]
Microsoft (R) Incremental Linker Version 9.00.30729.01
Copyright (C) Microsoft Corporation.  All rights reserved.

/out:test.exe
test.obj

C:\Users\Ehsan>

So, I guess compiling STL with exceptions turned off is not possible on MSVC.

Have we ever considered using STLport?

Comment 5

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

(In reply to comment #3)
> My (very limited) prior experience with that kind of scheme, on old-skool Mac
> System 7, was that it was a nice idea in theory but did not work in practice. 
> The odds of emergency reclaim freeing enough memory to allow the memory-hungry
> operation that had provoked allocation failure to complete were very low.  I'd
> lay money that the same is true today, or possibly even more so.
> 

Not sure what you mean by "memory hungry", but we're not planning on using infallible allocators at large-allocation sites.  Sites that allocate small, fixed amounts of memory in loops are problematic though.  The inchoate goal is to have those loops check the memory-critical flag, if we identify them.

> (One possible exception is when the memory-hungry operation is generating large
> amounts of cyclic garbage, so its *actual* memory requirement is small but we
> haven't gotten around to reclaiming yet.  But that might be better addressed by
> making it possible to run the cycle collector incrementally from *any* malloc. 
> Similarly for the Javascript GC heap.)
> 

That sounds really cool, but note that we might detect critical-memory or OOM on any thread, so we'd probably still need a bailing scheme on the main thread to run these.

Comment 6

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

(In reply to comment #4)
> So, I tried compiling the simplest program which would throw std::out_of_range
> on MSVC:
[snip]
> /out:test.exe
> test.obj
> 

Looks like it worked, but printed some nasty warnings, no?  Can you try running this program and see what happens on out_of_range?

> So, I guess compiling STL with exceptions turned off is not possible on MSVC.
> 

If we can't get std::terminate() exits on MSVC-without-exceptions, I would be nervous about shipping code using unwrapped std:: data structures.  Sounds like we might be adding another attack vector.

> Have we ever considered using STLport?

Not familiar, will read.

Comment 7

[(no longer active)]

(In reply to comment #6)
> (In reply to comment #4)
> > So, I tried compiling the simplest program which would throw std::out_of_range
> > on MSVC:
> [snip]
> > /out:test.exe
> > test.obj
> > 
> 
> Looks like it worked, but printed some nasty warnings, no?  Can you try running
> this program and see what happens on out_of_range?

You're right, don't know why I didn't see the exe before!

> > So, I guess compiling STL with exceptions turned off is not possible on MSVC.
> > 
> 
> If we can't get std::terminate() exits on MSVC-without-exceptions, I would be
> nervous about shipping code using unwrapped std:: data structures.  Sounds like
> we might be adding another attack vector.

Running test.exe prints the following to the console:

This application has requested the Runtime to terminate it in an unusual way.
Please contact the application's support team for more information.

This comes from abort.

So, I thought I'd resort to CRT's source code.  The STL source files for MSVC throw exceptions using the _THROW macro, which ends up calling std::_Throw when exceptions are not available (_HAS_EXCEPTIONS is 0).  std::_Throw (defined in stdthrow.cpp) just calls std::_Debug_message, which used fputs to output a message and then goes on to call C's abort function.

I don't know if that's good enough for us.

> > Have we ever considered using STLport?
> 
> Not familiar, will read.

STLport was once _the_ STL library to use.  It was several orders of magnitudes faster than VC's STL library and a fair bit faster than gcc's.  It also had decent debugging support, and by using it you were basically ensuring that you're using the same STL code across platforms.  It's been a while since I used it, so I'm not sure about the project's current status.

Comment 8

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

(In reply to comment #7)
> > > So, I guess compiling STL with exceptions turned off is not possible on MSVC.
> > > 
> > 
> > If we can't get std::terminate() exits on MSVC-without-exceptions, I would be
> > nervous about shipping code using unwrapped std:: data structures.  Sounds like
> > we might be adding another attack vector.
> 
> Running test.exe prints the following to the console:
> 
> This application has requested the Runtime to terminate it in an unusual way.
> Please contact the application's support team for more information.
> 
> This comes from abort.
> 
> So, I thought I'd resort to CRT's source code.  The STL source files for MSVC
> throw exceptions using the _THROW macro, which ends up calling std::_Throw when
> exceptions are not available (_HAS_EXCEPTIONS is 0).  std::_Throw (defined in
> stdthrow.cpp) just calls std::_Debug_message, which used fputs to output a
> message and then goes on to call C's abort function.
> 
> I don't know if that's good enough for us.
> 

That sounds just about perfect, actually!  That's pretty much gcc does IIRC.  Makes me wonder, though, why we get unhandled exceptions in breakpad on std::out_of_memory ... maybe it's a different path?

Comment 9

[(no longer active)]

(In reply to comment #8)
> (In reply to comment #7)
> > > > So, I guess compiling STL with exceptions turned off is not possible on MSVC.
> > > > 
> > > 
> > > If we can't get std::terminate() exits on MSVC-without-exceptions, I would be
> > > nervous about shipping code using unwrapped std:: data structures.  Sounds like
> > > we might be adding another attack vector.
> > 
> > Running test.exe prints the following to the console:
> > 
> > This application has requested the Runtime to terminate it in an unusual way.
> > Please contact the application's support team for more information.
> > 
> > This comes from abort.
> > 
> > So, I thought I'd resort to CRT's source code.  The STL source files for MSVC
> > throw exceptions using the _THROW macro, which ends up calling std::_Throw when
> > exceptions are not available (_HAS_EXCEPTIONS is 0).  std::_Throw (defined in
> > stdthrow.cpp) just calls std::_Debug_message, which used fputs to output a
> > message and then goes on to call C's abort function.
> > 
> > I don't know if that's good enough for us.
> > 
> 
> That sounds just about perfect, actually!  That's pretty much gcc does IIRC. 
> Makes me wonder, though, why we get unhandled exceptions in breakpad on
> std::out_of_memory ... maybe it's a different path?

Hmm, I'm not sure.  Firstly note that this is only valid for code using the _THROW macro.  I don't know how the compiler handles the throw keyword.  Also, I noticed that I get unhandled exceptions whenever I run the code with debugger attached.  I didn't have time to investigate this further, but perhaps someone can try stepping through the assembly instructions and see what's really happening?

Comment 10

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

(In reply to comment #9)
> Hmm, I'm not sure.  Firstly note that this is only valid for code using the
> _THROW macro.  I don't know how the compiler handles the throw keyword.  Also,
> I noticed that I get unhandled exceptions whenever I run the code with debugger
> attached.  I didn't have time to investigate this further, but perhaps someone
> can try stepping through the assembly instructions and see what's really
> happening?

I looked at the CRT source (newop.cpp), and its operator new() is using the _RAISE macro, which also expands to std::_Throw when exceptions are disabled ... BUT, because operator new() is compiled into the CRT, it's almost certainly being build with exceptions enabled, which would explain the discrepancy.  We could avoid this particular case by setting our own new_handler, which is invoked before _RAISE'ing.

But in general, this implies that we need to distinguish between exceptions thrown by headers and exceptions originating from code compiled into the CRT.  Hopefully all the logic-error exceptions are thrown from headers.

Comment 11

[Zack Weinberg (:zwol)]

(In reply to comment #5)
>
> Not sure what you mean by "memory hungry", but we're not planning on using
> infallible allocators at large-allocation sites.  Sites that allocate small,
> fixed amounts of memory in loops are problematic though.  The inchoate goal is
> to have those loops check the memory-critical flag, if we identify them.

It doesn't matter whether the memory is consumed in large or small chunks.  The empirical observation is that *anything* that runs a computer out of memory these days is almost certain to be either an infinite-memory-consumption bug or a task that requires *lots* more memory than you have.  Either way, no last-chance pool will save you.

(In math: if A is available memory, and R is memory required, then given that R > A, Pr(R < A + delta) is zero in practice for finite delta.)

(In reply to comment #7)
> Running test.exe prints the following to the console:
> 
> This application has requested the Runtime to terminate it in an unusual way.
> Please contact the application's support team for more information.
> 
> This comes from abort.
> 
> So, I thought I'd resort to CRT's source code.  The STL source files for MSVC
> throw exceptions using the _THROW macro, which ends up calling std::_Throw when
> exceptions are not available (_HAS_EXCEPTIONS is 0).  std::_Throw (defined in
> stdthrow.cpp) just calls std::_Debug_message, which used fputs to output a
> message and then goes on to call C's abort function.
> 
> I don't know if that's good enough for us.

This is good for us at runtime, I think, but we need to find a way to turn the warning off or it'll get really annoying.  Knowing Microsoft, there's a way.

> STLport was once _the_ STL library to use.  It was several orders of magnitudes
> faster than VC's STL library and a fair bit faster than gcc's.  It also had
> decent debugging support, and by using it you were basically ensuring that
> you're using the same STL code across platforms.  It's been a while since I
> used it, so I'm not sure about the project's current status.

Last I heard (which was some time ago) it hadn't been updated in a long time, the only remaining large user was OpenOffice and they were stuck with it for binary compatibility reasons.

Comment 12

[(no longer active)]

(In reply to comment #11)
> This is good for us at runtime, I think, but we need to find a way to turn the
> warning off or it'll get really annoying.  Knowing Microsoft, there's a way.

#pragma warning( disable : 4530 )

> > STLport was once _the_ STL library to use.  It was several orders of magnitudes
> > faster than VC's STL library and a fair bit faster than gcc's.  It also had
> > decent debugging support, and by using it you were basically ensuring that
> > you're using the same STL code across platforms.  It's been a while since I
> > used it, so I'm not sure about the project's current status.
> 
> Last I heard (which was some time ago) it hadn't been updated in a long time,
> the only remaining large user was OpenOffice and they were stuck with it for
> binary compatibility reasons.

Hmm, yes, the latest version seems to be from 2008-12-10.  :-(

Comment 13

[Boris Zbarsky [:bzbarsky]]

> almost certain to be either an infinite-memory-consumption bug or
> a task that requires *lots* more memory than you have.  Either way, no
> last-chance pool will save you.

Sure.  The point of a last-ditch pool is to service the allocation being made right now, set an out-of-memory flag, and return.  Consumers who might end up in situations where they're doing lots of allocations (frame construction, some DOM algorithms, etc) should be checking said flag and unwind back to the event loop.

At least that's the theory.  Maybe you're right that in practice that can't be done sanely...

Comment 14

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

I'd like us to resolve this so we can move forward.  Does anyone object to the following summary so far

 (1) We're OK with the "exceptions" the STL is known to "throw" being fatal, if they're indeed reliably fatal
 (2) For MSVC:
   a) exceptions "thrown" from headers included in code built without unwind semantics abort() rather than propagate
   b) exceptions thrown from the CRT are known to propagate
   c) the only such known exception is std::bad_alloc
 (3) We should set up STL types to use mozalloc ::operator new() so that we don't have to worry about bad_alloc and have the option of attempting OOM recovery

IMHO, this means we only have one TODO item before signing off on MSVC

  TODO 1: verify (2c) above, that std::bad_alloc is the only exception thrown from the CRT


(In reply to comment #0)
> However, I observe that GCC's STL (which is what we get on OSX and Linux) can
> be relied on to call std::terminate, rather than crashing in an unpredictable
> way, when it wants to throw an exception but it's been called from code
> compiled with -fno-exceptions.

I just found out that this might be only partially true.  So, the issue (as you know) is that Gecko is built with -fno-exceptions, but system libstdc++ is built with -fexceptions (shades of -fshort-wchar fun).  This means that gcc's STL is actually |throw|ing at runtime.  It appears to me that we're seeing std::terminate() not because -fno-exceptions code stops exceptions from propagating, but rather because libstdc++ runs out of stack before finding a handler.  (The libstdc++ source was too much for me to grok quickly.)  Here's a test that seems to confirm that hypothesis

[testexc.cc]
#include <vector>
#include <stdexcept>
extern void throw_something();
int main() {
    try { throw_something(); } catch (const std::out_of_range& e) { }
    return 0;
}
[throw_something.cc]
#include <vector>
void throw_something() {
    std::vector<int> i;
    i.at(1);
}
cjones@beast:~/gcc-dehydra/gcc-4.3.0[c_process_decl.diff]$ cd /tmp/
cjones@beast:/tmp[]$ g++ -fno-exceptions -c throw_something.cc -o throw_something.o
cjones@beast:/tmp[]$ g++ testexc.cc -o testexc throw_something.o 
cjones@beast:/tmp[]$ ./testexc
cjones@beast:/tmp[]$ 

The exception propagates to main() and is caught, and there's no abort().

If I remove |try { ... } catch()| from main(), then

cjones@beast:/tmp[]$ g++ testexc.cc -o testexc throw_something.o 
cjones@beast:/tmp[]$ ./testexc
terminate called after throwing an instance of 'std::out_of_range'
  what():  vector::_M_range_check
Aborted
cjones@beast:/tmp[]$ g++ -fno-exceptions testexc.cc -o testexc throw_something.o 
cjones@beast:/tmp[]$ ./testexc
terminate called after throwing an instance of 'std::out_of_range'
  what():  vector::_M_range_check
Aborted

The runtime behavior doesn't change with -fno-exceptions.


So, IMHO there are two more TODOs for gcc:

 TODO 2: is propagating exceptions our of -fno-exception code a potential security/stability problem?

(I'm not a security guy, but I've been assuming "yes", because (i) our code isn't exception safe; (ii) binary extensions and plugins can install exception handlers.  And yes, they can throw exceptions too, but we attempt to handle that for plugins.)

  TODO 3: if gcc exceptions propagating out of Gecko is problematic, can we avoid it?

gcc's STL throws all exceptions using helper functions in <bits/funcexcept.h> AFAICT, e.g. std::__throw_out_of_range("foo").  So .... I can think we can define our own header in lieu of funcexcept.h, with inlined versions of all these functions.  For example

  namespace std {
  inline void __throw_out_of_range(const char* msg) { abort(msg); }
  ...
  }

(That's what the -fno-exceptions version of __throw_out_of_range() in libstdc++ does.)  Other ideas?


It's also worth including

  TODO 4: Would STLport work for us?  Is there another similar library that's still actively maintained?

Comment 15

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

(In reply to comment #14)
>   TODO 1: verify (2c) above, that std::bad_alloc is the only exception thrown
> from the CRT
> 
>  TODO 2: is propagating exceptions our of -fno-exception code a potential
> security/stability problem?
> 

Another thought on this.  If plugins are going out of process, and binary extensions are going away entirely (is that still the plan?), then maybe this concern is going away.

>   TODO 3: if gcc exceptions propagating out of Gecko is problematic, can we
> avoid it?
> 

Another solution is to LD_PRELOAD __cxa_throw with a version that abort()s.  This is appealing, but if we currently load in-process plugins or binary extensions that |throw| regularly, then we'll be shooting ourselves in our collective feet.

Comment 16

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

(In reply to comment #15)
> (In reply to comment #14)
> >   TODO 1: verify (2c) above, that std::bad_alloc is the only exception thrown
> > from the CRT
> > 
> >  TODO 2: is propagating exceptions our of -fno-exception code a potential
> > security/stability problem?
> > 
> 
> Another thought on this.  If plugins are going out of process, and binary
> extensions are going away entirely (is that still the plan?), then maybe this
> concern is going away.
> 

Mook points out that the upgrade path for binary extensions might be jsctypes in some cases, which means the exception problem isn't going away, like, ever.  So I don't think we can rely on exceptions being uncaught and terminating Gecko.

Comment 17

[(no longer active)]

(In reply to comment #14)
>   TODO 4: Would STLport work for us?  Is there another similar library that's
> still actively maintained?

Apache's stdcxx may also be a good option to look into:

http://stdcxx.apache.org/

It hasn't had any recent releases, though.

Comment 18

[Zack Weinberg (:zwol)]

(In reply to comment #16)
> (In reply to comment #15)
> > (In reply to comment #14)
> > >   TODO 1: verify (2c) above, that std::bad_alloc is the only exception thrown
> > > from the CRT
> > > 
> > >  TODO 2: is propagating exceptions our of -fno-exception code a potential
> > > security/stability problem?
> > > 
> > 
> > Another thought on this.  If plugins are going out of process, and binary
> > extensions are going away entirely (is that still the plan?), then maybe this
> > concern is going away.
> > 
> 
> Mook points out that the upgrade path for binary extensions might be jsctypes
> in some cases, which means the exception problem isn't going away, like, ever. 
> So I don't think we can rely on exceptions being uncaught and terminating
> Gecko.

As far as I can tell, interposing our own .o file that defines the various __throw_* functions appropriately for our purposes should be enough.  There don't appear to be any direct calls to those functions from inside the libstdc++ binary.

Comment 19

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

I've thought some more about importing a cross-platform STL impl.  I'd like to give the compiler-specific versions a try first.  I can think of numerous hypotheticals in support of importing a cross-platform lib, but we don't have any (compelling) concrete reasons for doing so yet, IMHO.  Weighing that against the maintenance burden of importing a foreign impl, I think the balance is on the side of compiler impls for now.

That said, if we set things up right, if problems arise with compiler impls we could just drop in an external impl.

Anyone object?

If not, I'd like to propose the following "STL policy".

To "enable" a standard header H
 o H's exception behavior on gcc and MSVC must be reviewed (in its own bug?)
 o The headers H depends on must be reviewed
 o The CRT/libstdc++ code H depends on must be reviewed

For a compilation unit to use STL headers
 o Must be in #if MOZALLOC_HAVE_XMALLOC code (i.e. infallible ::operator new)
 o For MSVC, must not be in #if _HAS_EXCEPTIONS
 o For gcc
   - Must not be in #if __EXCEPTIONS
   - Must be linked against "gcc_interpose_throw.cpp"

I propose enforcing the above policy with these mechanisms.

 o For gcc
   - Re-use the system_wrappers framework to wrap STL headers (which means we would still include vector as |#include <vector>|
   - Enforce feature requirements in the STL header wrappers
   - Add a new |make check| target that looks for unresolved __throw_* symbols in libraries we "care about", e.g. libxul.so for --enable-libxul.  --disable-libxul is more complicated.  (I have a script for this that I used for mozalloc.)
 o For MSVC
   - Enable system_wrappers just for STL headers
   - Enforce feature requirements in the STL header wrappers

Using wrapped headers additionally means we can enable DEBUG-only checking when available.

Thoughts, objections?

Comment 20

[(no longer active)]

We should also have solutions for Tier 2 platforms as well here.  In addition to that, it would be nice to think of a fallback for Tier 3 platforms, because I'm sure most of them lack very good STL libraries.

Comment 21

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

A point absent from comment 19 was the policy on exception behavior for non MSVC/gcc.  My opinion is "don't care".  The main worry about exceptions is the security issue when they're thrown in the presence of binary extensions and plugins.  Because the attack surface for non-MSVC/gcc platforms is so small, I think the concern should be similarly small.

Looking over https://developer.mozilla.org/en/Supported_build_configurations, the Tier IIs are gcc or MSVC except for OpenSolaris, which has a modern C++ compiler AIUI.  The Tier IIIs seem mostly OK except for aCC, HP cc, and Compaq cc.  If a Tier III compiler has a poor STL impl, then IMHO the remedy should be the Tier III maintainers' responsibility.  I don't think we should slow this project down for compilers that don't implement C++98.  If the solution there is importing a third-party STL implementation, then great!  We'll have found a volunteer to maintain that code.

Comment 22

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

So, I just realized that we can't do #include <vector> to get our wrapper, and have the wrapper find the system header, because MSVC doesn't have an equivalent to #include_next.  Instead, I'd like to propose

#include <std/vector>

where std/ will be a directory in $(DIST)/include in which our headers are located.

Objections?

Comment 23

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Attachment: Allow whitelisted STL headers to be included through <std/foo>

This implements the policy in comment 19.  r? spamming to all parties I think might be interested.  I included <algorithm> in the initial whitelist because we already use it in libpr0n and I wanted a testcase, but we should still review <algorithm>.

Interestingly enough, we currently build libpr0n with _HAS_EXCEPTIONS on MSVC, and it uses <vector> and <algorithm>.  Oops.

Comment 24

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

I'm going to start filing bugs dependent on this one for checking specific headers.  Before doing so, I'd like to append a clause to the proposed policy

 To "enable" a standard header H
  o H's exception behavior on gcc and MSVC must be reviewed (in its own bug?)
  o The headers H depends on must be reviewed
  o The CRT/libstdc++ code H depends on must be reviewed
+ o H cannot be enabled if
+   - H throws an exception that cannot be disabled or converted to an abort()
+   - H calls a CRT function that throws an exception and the function can't be replaced

Can anyone think of other reasons why we might reject a particular header?

Comment 25

[(no longer active)]

Comment on attachment 433695 [details] [diff] [review]
Allow whitelisted STL headers to be included through <std/foo>

>diff --git a/config/msvc-stl-wrapper.template.h b/config/msvc-stl-wrapper.template.h
>+// We know that code won't be able to catch exceptions, but that's OK
>+// because we're not throwing them.
>+#pragma warning( disable : 4530 )

I think we should push and pop the warnings state, so that we don't inadvertently disable this warning elsewhere.  r=me with that.

Comment 26

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

(In reply to comment #25)
> (From update of attachment 433695 [details] [diff] [review])
> >diff --git a/config/msvc-stl-wrapper.template.h b/config/msvc-stl-wrapper.template.h
> >+// We know that code won't be able to catch exceptions, but that's OK
> >+// because we're not throwing them.
> >+#pragma warning( disable : 4530 )
> 
> I think we should push and pop the warnings state, so that we don't
> inadvertently disable this warning elsewhere.  r=me with that.

Hmm, I had assumed the warnings came from the template instantiations rather than the header code.  If not, push/pop is a great suggestion.

Comment 27

[(no longer active)]

(In reply to comment #26)
> (In reply to comment #25)
> > (From update of attachment 433695 [details] [diff] [review] [details])
> > >diff --git a/config/msvc-stl-wrapper.template.h b/config/msvc-stl-wrapper.template.h
> > >+// We know that code won't be able to catch exceptions, but that's OK
> > >+// because we're not throwing them.
> > >+#pragma warning( disable : 4530 )
> > 
> > I think we should push and pop the warnings state, so that we don't
> > inadvertently disable this warning elsewhere.  r=me with that.
> 
> Hmm, I had assumed the warnings came from the template instantiations rather
> than the header code.  If not, push/pop is a great suggestion.

Good point.  Warnings do come from template instantiations, but I think that the compiler correctly manages this.  I'm not 100% sure though, so you might want to test it if you have a Windows box handy (if not, I can test in a VM, just let me know.)

Comment 28

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

(In reply to comment #22)
> So, I just realized that we can't do #include <vector> to get our wrapper, and
> have the wrapper find the system header, because MSVC doesn't have an
> equivalent to #include_next.  Instead, I'd like to propose
> 
> #include <std/vector>
> 
> where std/ will be a directory in $(DIST)/include in which our headers are
> located.
> 

A (rather hacky/ugly) possibility for MSVC might be that, in the script that generates these headers, read the $INCLUDE environment variable and resolve where <foo> comes from.  Then in the emitted wrapper, do |#include <path/resolved/to/foo>|.

A fairly important reason for preserving #include <foo> that I forgot to mention above is that if we can make that work, then all the chromium code we've imported will use our wrappers without any modification.

Comment 29

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

(In reply to comment #27)
> (In reply to comment #26)
> > (In reply to comment #25)
> > > (From update of attachment 433695 [details] [diff] [review] [details] [details])
> > > >diff --git a/config/msvc-stl-wrapper.template.h b/config/msvc-stl-wrapper.template.h
> > > >+// We know that code won't be able to catch exceptions, but that's OK
> > > >+// because we're not throwing them.
> > > >+#pragma warning( disable : 4530 )
> > > 
> > > I think we should push and pop the warnings state, so that we don't
> > > inadvertently disable this warning elsewhere.  r=me with that.
> > 
> > Hmm, I had assumed the warnings came from the template instantiations rather
> > than the header code.  If not, push/pop is a great suggestion.
> 
> Good point.  Warnings do come from template instantiations, but I think that
> the compiler correctly manages this.  I'm not 100% sure though, so you might
> want to test it if you have a Windows box handy (if not, I can test in a VM,
> just let me know.)

So push/pop does do the right thing.  I'll update the patch.

(In reply to comment #28)
> (In reply to comment #22)
> > So, I just realized that we can't do #include <vector> to get our wrapper, and
> > have the wrapper find the system header, because MSVC doesn't have an
> > equivalent to #include_next.  Instead, I'd like to propose
> > 
> > #include <std/vector>
> > 
> > where std/ will be a directory in $(DIST)/include in which our headers are
> > located.
> > 
> 
> A (rather hacky/ugly) possibility for MSVC might be that, in the script that
> generates these headers, read the $INCLUDE environment variable and resolve
> where <foo> comes from.  Then in the emitted wrapper, do |#include
> <path/resolved/to/foo>|.
> 

Gross and ugly, but I got this to work.  I think this is a better approach.  Any objections?

Comment 30

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

So, with the new approach that allows us to sneak dist/stl_wrapper/foo into chromium code, I found that some of the gcc debug containers require RTTI :(.  I don't think this is a problem we need to solve to close out this bug, but the simplest solution would be just building -frtti in DEBUG/gcc/STL-wrapped builds.

Comment 31

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Comment on attachment 433695 [details] [diff] [review]
Allow whitelisted STL headers to be included through <std/foo>

The v2 approach revealed a flaw in this one build-system-wise: on every top-level rebuild, a rebuild of all STL-wrapper-using code is triggered.  I tried using a sentinel to guard regenerating that dir, but found that on clobber builds $(DIST) doesn't exist when the python script is run.  (The system_wrapper method doesn't work because windows can't do symlinks.)

Other than that, everything is looking good locally.  I had to remove all XPCOM dependencies from the STL wrappers and xpcom/glue/functexcept.h because of the prtypes.h/basictypes.h fun, but that's not a big deal.

Should be able to get v2 up tomorrow.

Comment 32

[(not currently active) Ted Mielczarek]

(In reply to comment #31)
> (From update of attachment 433695 [details] [diff] [review])
> The v2 approach revealed a flaw in this one build-system-wise: on every
> top-level rebuild, a rebuild of all STL-wrapper-using code is triggered.  I
> tried using a sentinel to guard regenerating that dir, but found that on
> clobber builds $(DIST) doesn't exist when the python script is run.  (The
> system_wrapper method doesn't work because windows can't do symlinks.)

The system_wrapper method should work fine. nsinstall preserves mtime on Windows (which is how we avoid global rebuilds despite blowing away dist/ on top-level rebuilds).

Comment 33

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Attachment: Allow reviewed/approved STL headers to be included through <foo>

Needed nsinstall.py sauce.

One might perhaps call this a bit hackier than the last patch, but the end results are much better IMHO.

Comment 34

[(not currently active) Ted Mielczarek]

Comment on attachment 434276 [details] [diff] [review]
Allow reviewed/approved STL headers to be included through <foo>

>diff --git a/config/Makefile.in b/config/Makefile.in
>--- a/config/Makefile.in
>+++ b/config/Makefile.in
>@@ -132,16 +132,36 @@ export::
> 		-DMOZ_NATIVE_PNG=$(MOZ_NATIVE_PNG) \
> 		-DMOZ_NATIVE_JPEG=$(MOZ_NATIVE_JPEG) \
> 		$(srcdir)/system-headers | $(PERL) $(topsrcdir)/nsprpub/config/make-system-wrappers.pl system_wrappers
> 	$(INSTALL) system_wrappers $(DIST)
> 
> GARBAGE_DIRS += system_wrappers
> endif
> 
>+ifdef WRAP_GCC_STL_INCLUDES
>+stl_compiler = gcc
>+else
>+ifdef WRAP_MSVC_STL_INCLUDES
>+stl_compiler = msvc
>+endif
>+endif

I think you should just have a single WRAP_STL_INCLUDES var. We already have GNU_{CC,CXX} and _MSC_VER makefile vars that you can use to determine whether you're using GCC or MSVC.

>diff --git a/config/make-stl-wrappers.py b/config/make-stl-wrappers.py
>new file mode 100644
>--- /dev/null
>+++ b/config/make-stl-wrappers.py
>@@ -0,0 +1,39 @@

This probably needs a license header.

>+import os, string, sys
>+
>+if 5 != len(sys.argv):

For build system Python scripts, I like them to all have a main() method of some sort, and then something like:
if __name__ == '__main__':
  main()
instead of running code at the top-level scope. That way if we want to make them modules at some point in the future, we can do so fairly easily.

>+    print >>sys.stderr, """Usage:
>+  python %s OUT_DIR ('msvc'|'gcc') TEMPLATE_FILE HEADER_LIST_FILE
>+"""% (sys.argv[0])
>+    sys.exit(1)
>+
>+_, outdir, compiler, template_file, header_list_file = sys.argv
>+
>+def find_in_windows_path(file, searchpath):
>+    for dir in searchpath.split(';'):

You could use os.pathsep here and make this not a Windows-specific function if you wanted.

>+        if os.path.exists(os.path.join(dir, header)):
>+            return dir

I think you should just return os.path.join(dir, header) here instead of making the caller do the join.

>+    return ''
>+
>+def header_path(header):
>+    if compiler == 'gcc':
>+        # we use include_next on gcc
>+        return header
>+    elif compiler == 'msvc':
>+        return (find_in_windows_path(header,
>+                                     os.environ.get('INCLUDE', ''))
>+                +'\\'+ header)

Why aren't you using os.path.join here? Worried about the cross-compile case? (You use it above, does that work for your wine cross-compiles?)

>+    else:
>+        # hope someone notices this ...
>+        raise NotImplementedError, compiler
>+
>+if not os.path.isdir(outdir):
>+    os.mkdir(outdir)
>+
>+template = open(template_file, 'r').read()
>+
>+for header in open(header_list_file, 'r'):
>+    header = header[:-1]                # chomp trailing \n

You want header.rstrip().

>+    path = header_path(header)
>+    open(os.path.join(outdir, header), 'w').write(
>+        string.Template(template).substitute(
>+            HEADER=header, HEADER_PATH=path))

I think this is a little too functional for my Python taste. Especially since you're using it in a loop, and I'm not sure that the file will be closed until the file object gets GCed or the script exits. How about:
try:
  f = open(...)
  f.write(...)
finally:
  f.close()

(I'd suggest |with open() as f:| but I don't think you can use that in Python 2.4 and we haven't raised our minimum requirements yet.)

>diff --git a/config/stl-headers b/config/stl-headers
>new file mode 100644
>--- /dev/null
>+++ b/config/stl-headers
>@@ -0,0 +1,1 @@
>+algorithm

Can you put a comment block in this file explaining what it's for? (I know system-headers lacks one, but we shouldn't leave things a mystery if we don't need to.)

I didn't review the actual C++ bits, I think you have enough C++ expertise in the other reviewers here.

r=me with those nits fixed.

Comment 35

[Zack Weinberg (:zwol)]

Comment on attachment 434276 [details] [diff] [review]
Allow reviewed/approved STL headers to be included through <foo>

The GCC-specific bits are good as far as I can tell.  Some minor kibitzes:

+// NB: auto generated, please don't edit by hand

Could you please have the Python script insert this line? It's confusing to see it in the template files.  Also, the template files are complicated enough that maybe they should have license boilerplate.

+inline void NS_NORETURN
+__throw_bad_exception(void)
+{
+    MOZ_FE_ABORT("bad_exception: this should never be thrown");
+}

Throughout, I'm not sure the phrasing "should never be thrown" is appropriate.  Maybe "fatal: STL threw bad_exception" or whatever?

+inline void NS_NORETURN
+__throw_system_error(int err) 
+{
+    MOZ_FE_ABORT("system_error: this should never be thrown");
+}

It can happen in a follow-up, but please find a way to include either the decimal value of that error code, or its strerror() string, in the message.  (GCC 4.4 appears to use this only for <mutex>, which is shiny new C++0x that we probably can't pick up yet, so it's not a priority.)

Comment 36

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Attachment: Updated per comments [Checkin: Comment 42]

Fixed a problem on MSVC(9); need to build -D_HAS_EXCEPTIONS=0 rather than setting that in the header.  Also added <vector> to stl-headers because we already use it unsafely in imgLoader, but we should still review it nonetheless.

Unfortunately this patch bounced on tryserver/MSVC8 :(

http://tinderbox.mozilla.org/showlog.cgi?log=MozillaTry/1270179793.1270181552.13780.gz

Looks like MSVC8 is failing to compile its own header, but since this works without the patch there might a problem with the stl_wrappers/.  Installing MSVC8 to check.

Comment 37

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Attachment: Simple (disabled) test of STL wrappers [Checkin: Comment 42]

Comment 38

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

(In reply to comment #36)
> Unfortunately this patch bounced on tryserver/MSVC8 :(

Aha, this is an opt build issue, not an MSVC8 issue.  Pretty sure it's disabling _SECURE_SCL in wrappers that's causing it (because other headers have it enabled).

Comment 39

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

OK, commenting out the |#undef _SECURE_SCL| fixed things.  Ready to land!

Filed bug 556707 on restoring this undef, if there's a perf win.

Comment 40

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

http://hg.mozilla.org/mozilla-central/rev/cec7b12eb5e3
http://hg.mozilla.org/mozilla-central/rev/ac5cbcb2feb2

Comment 41

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Bounced on windows/debug/trace-malloc, why not.

Comment 42

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Hopefully stuck this time.

http://hg.mozilla.org/mozilla-central/rev/5f922fafc730
http://hg.mozilla.org/mozilla-central/rev/5514c8b5448e

Comment 43

[Serge Gautherie (:sgautherie)]

(In reply to comment #42)
> http://hg.mozilla.org/mozilla-central/rev/5f922fafc730
> http://hg.mozilla.org/mozilla-central/rev/5514c8b5448e

Linux64 SeaMonkey is broken again:

http://tinderbox.mozilla.org/showlog.cgi?log=SeaMonkey/1270239918.1270241430.26182.gz
Linux x86-64 comm-central-trunk build on 2010/04/02 13:25:18
{
collect2: ld returned 1 exit status
make[7]: *** [libnptest.so] Error 1
}

Comment 44

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Not sure why this isn't also failing on m-c, but it's a one line fix.

Comment 45

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Attachment: testplugin shouldn't use STL wrappers

Comment 46

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Attachment: testplugin shouldn't use STL wrappers

Something was garbled in transmission apparently.

Comment 47

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

OK, screw it, this file is not empty locally.  Here's the patch

diff --git a/modules/plugin/test/testplugin/Makefile.in b/modules/plugin/test/testplugin/Makefile.in
--- a/modules/plugin/test/testplugin/Makefile.in
+++ b/modules/plugin/test/testplugin/Makefile.in
@@ -46,16 +46,19 @@ MODULE       = nptest
 LIBRARY_NAME = nptest
 MODULE_NAME  = TestPlugin
 
 
 # Need to custom install plugins
 NO_DIST_INSTALL	= 1
 NO_INSTALL = 1
 
+# testplugin isn't gecko code and so shouldn't use its STL wrappers
+STL_FLAGS =
+
 CPPSRCS   =  \
   nptest.cpp \
   nptest_utils.cpp \
   $(NULL)
 
 ifeq ($(MOZ_WIDGET_TOOLKIT),cocoa)
 CMMSRCS = nptest_macosx.mm
 endif

Comment 48

[Serge Gautherie (:sgautherie)]

Comment on attachment 436776 [details] [diff] [review]
testplugin shouldn't use STL wrappers 


I don't have a local build atm.
Given what the fix is, it should not be worth a MoMe Try build:
a check-in as bustage fix should do (ftb at least), I think.

Comment 49

[Serge Gautherie (:sgautherie)]

(In reply to comment #48)
> Given what the fix is, it should not be worth a MoMe Try build:

(Hum, MoMe Try can't test m-c patch anyway :-<)

Comment 50

[Serge Gautherie (:sgautherie)]

(In reply to comment #47)

The other implicit question is why does it affect Linux64 SeaMonkey/c-c only?

Comment 51

[[e-mail]]

This broke gfxWindowsPlatform.cpp for me because it (uselessly) includes <string> which includes a bunch of other stuff which expects exceptions, but something previously disabling exceptions was already included.

The generation of wrapper include files fails silently on my cross-compile build because $INCLUDE uses windows paths but python uses posix paths. Even after fixing that, I find that TestSTLWrappers.exe doesn't link due to four exceptional unresolved externals.

Comment 52

[Jacek Caban]

Attachment: mingw test fix

The test has broken mingw compilation, because xpcom/tests are compiled with ENABLE_CXX_EXCEPTIONS defined, so exception guard in STL wrappers report errors. It was unnoticed on other builds because:

- On non-Windows builds, ENABLE_CXX_EXCEPTIONS has no effect according to config/rules.mk (it seems dangerous to depend on it).

- On MSVC build exceptions are enabled, but _HAS_EXCEPTIONS is not defined, so it works. I think this is a risky configuration, so I think we should consider making it clear and always define _HAS_EXCEPTIONS when exceptions are enabled (a patch from bug 550371 cleans up exception config a bit, so it might be a good idea to extend it).

I'm attaching a patch that follows current configuration and disables compilation on non-MSVC Windows builds (that is a cleaner way to disable it on mingw).

Comment 53

[[e-mail]]

Sigh, my "normal" build also fails, it can't link libpr0n due to three exceptional unresolved externals...

Comment 54

[Serge Gautherie (:sgautherie)]

(In reply to comment #50)
> The other implicit question is why does it affect Linux64 SeaMonkey/c-c only?

Ftr,
{
[[e-mail] - 2010/04/03 08:38:59]
clobber didn't help, this is genuine bustage in the testplugin.
}

Comment 55

[Serge Gautherie (:sgautherie)]

Comment on attachment 436640 [details] [diff] [review]
Simple (disabled) test of STL wrappers
[Checkin: Comment 42]

>+      fputs("TEST-FAIL | TestSTLWrappers.cpp | caught an exception!\n",

This test needs a TEST-PASS line too.

Comment 56

[Serge Gautherie (:sgautherie)]

Comment on attachment 436863 [details] [diff] [review]
mingw test fix


Nit: I would reverse the if+else order.

Comment 57

[Serge Gautherie (:sgautherie)]

(In reply to comment #43)
> Linux64 SeaMonkey is broken again:

I eventually filed bug 557125.

Comment 58

[Serge Gautherie (:sgautherie)]

(In reply to comment #53)
> Sigh, my "normal" build also fails, it can't link libpr0n due to three
> exceptional unresolved externals...

This looks like what I just filed as bug 557060 comment 8.

*****

It's been 4 days this check-in caused various bustages:
why isn't it fixed or backed out yet??
PLEASE do so asap!

Comment 59

[Shawn Wilsher :sdwilsh]

(In reply to comment #58)
> It's been 4 days this check-in caused various bustages:
> why isn't it fixed or backed out yet??
> PLEASE do so asap!
Has it caused bustage in any tier 1 platforms?

Comment 60

[Serge Gautherie (:sgautherie)]

(In reply to comment #59)
> Has it caused bustage in any tier 1 platforms?

You quoted the wrong line: bug 557060 comment 8 is 'win32/x86 (msvc)', from
https://developer.mozilla.org/en/Supported_build_configurations

Comment 61

[Boris Zbarsky [:bzbarsky]]

Please don't reopen bugs unless the patch has actually been backed out.  There's a separate bug or bugs tracking the regression(s) in question, right?

Comment 62

[Shawn Wilsher :sdwilsh]

(In reply to comment #60)
> You quoted the wrong line: bug 557060 comment 8 is 'win32/x86 (msvc)', from
> https://developer.mozilla.org/en/Supported_build_configurations
I don't see any bustage on tinderbox, which means the issues should be dealt with in a follow-up bug (like bz said).

Comment 63

[Serge Gautherie (:sgautherie)]

(In reply to comment #62)
> I don't see any bustage on tinderbox, which means the issues should be dealt
> with in a follow-up bug (like bz said).

(What is this game of changing your question after each of my answer?
Tinderboxes don't cover all the supported end-user platforms.!.)

I don't care in which bug this issue is dealt with:
bug 557060 has been filed 3 days ago and saw no progress yet!

Comment 64

[Boris Zbarsky [:bzbarsky]]

> Tinderboxes don't cover all the supported end-user platforms.!.)

They do cover the tier 1 ones last I checked....

Comment 65

[Benjamin Smedberg]

It was filed over the weekend in a configuration that is not tier-1. That means that the developers will look at it and usually help fix it, but you should not expect blocker-level response times. Obviously firefox x86-64 continues to build (which has basically become a tier-1 platform), so there is some interesting disconnect going on that is being examined.

Comment 66

[Serge Gautherie (:sgautherie)]

(In reply to comment #64)
> > Tinderboxes don't cover all the supported end-user platforms.!.)
> 
> They do cover the tier 1 ones last I checked....

I assume it depends on what you call "cover", partial or complete:
*compile env: afaik, none builds with 2003R2 SDK.
*execution env: afaik, none tests on Windows 2000.
*though both are supported as 'win32/x86 (msvc)' tier-1...


(In reply to comment #65)
> It was filed over the weekend in a configuration that is not tier-1.

Because it was pushed on Friday...
Not tier-1: aren't SeaMonkey 2.1 tinderboxes 'win32/x86 (msvc)'??

> that the developers will look at it and usually help fix it, but you should not

Well, when the bug is called "investigate ..." and a first issue is reported "a-s-a" the patch is pushed, I would expect a little more than posting 1-2 comments then leave (for a 3-day w-e) :-/

> expect blocker-level response times. Obviously firefox x86-64 continues to
> build (which has basically become a tier-1 platform), so there is some
> interesting disconnect going on that is being examined.

It's good that the near-tier-1 Linux64 issue (which causes a red on SeaMonkey tinderbox) is being looked at :-)
Yet, I still see no progress at all on the actually-tier-1 Windows issue (which I've just now explicitly confirmed with Firefox too) :-(

Comment 67

[Justin Wood (:Callek)]

(In reply to comment #66)
> Because it was pushed on Friday...
> Not tier-1: aren't SeaMonkey 2.1 tinderboxes 'win32/x86 (msvc)'??

Please strip this down into a coherent sentence. I am not sure what you are saying/asking here. (double-tripple-negatives make this harder)

Comment 68

[Serge Gautherie (:sgautherie)]

(In reply to comment #67)
> Please strip this down into a coherent sentence.

Eh ... Let me try again then:
Why does Benjamin write that Windows SeaMonkey tinderbox(es) is a "not tier-1 configuration"?

Comment 69

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

(In reply to comment #66)
> (In reply to comment #64)
> > that the developers will look at it and usually help fix it, but you should not
> 
> Well, when the bug is called "investigate ..." and a first issue is reported
> "a-s-a" the patch is pushed, I would expect a little more than posting 1-2
> comments then leave (for a 3-day w-e) :-/

... including a patch (comment 47)?  If seamonkey devs had more important things to do over the weekend than test the patch, why do you think I didn't?

Comment 70

[Serge Gautherie (:sgautherie)]

(In reply to comment #69)

> ... including a patch (comment 47)?  If seamonkey devs had more important

As a comment, it looked more like a possible workaround than a long-term fix:
you didn't ask for review, nor even for feedback/testing...

> things to do over the weekend than test the patch, why do you think I didn't?

SeaMonkey devs are not responsible for the failures...
I have no access to Linux64 to test any solution on that platform.

Why are you talking about SeaMonkey when Firefox is affected by failures too?
Why are you and Benjamin focusing on Linux64 when I'm asking about Windows in the first place and the two bugs I filed are only part on the failures anyway?

You'd rather apologize to the other people who spent their time trying to figure out and report the failures your push caused!

Comment 71

[Robert Kaiser]

(In reply to comment #69)
> ... including a patch (comment 47)?  If seamonkey devs had more important
> things to do over the weekend than test the patch, why do you think I didn't?
(In reply to comment #70)
> You'd rather apologize to the other people who spent their time trying to
> figure out and report the failures your push caused!

Hey, please calm down, let's concentrate on solving things instead of heating up discussions. It's hard enough to get deep-going changes like what's done here into the source anyhow, so it's fully OK to have it stick when Firefox passes and actively work on fixing other cases in specific followup bugs for every problem that's still seen. And from what I see, this is happening as well.

Let's focus on gathering all info needed to fix those bustages it caused, and everyone will be happy. Working together and finding solutions is the only real way out, not accusing each other of whatever.

Thanks for Serge for testing and reporting the problems and thanks to cjones for helping to fix them.

Comment 72

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Comment on attachment 436863 [details] [diff] [review]
mingw test fix

Hi Jacek, apparently the only reason ENABLE_CXX_EXCEPTIONS is used in xpcom/tests is for an archaic subtest in  http://hg.mozilla.org/mozilla-central/annotate/6cba83fe343b/xpcom/tests/TestCOMPtr.cpp#l325.  We don't support exceptions anyway, so this test is kinda pointless.

I'd prefer to just delete that subtest and remove ENABLE_CXX_EXCEPTIONS from xpcom/tests/Makefile.in.

You're quite right about ENABLE_CXX_EXCEPTIONS being rather broken now, but we should deal with that in a followup.

Comment 73

[Jacek Caban]

Attachment: Remove ENABLE_CXX_EXCEPTIONS from XPCOM tests [Checked in: Comment 74]

Hi Chris, thanks for the review. I've attached a patch that removes ENABLE_CXX_EXCEPTIONS and tests that required it (I'm not sure if this patch deserves a separated bug, I hope it's find to attach it here).

Comment 74

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

http://hg.mozilla.org/mozilla-central/rev/6742813074a9 (Neglected to add r=cjones to the commit message, oops.)

Let's please let this bug die.  Please file new bugs for new issues that arise.

Comment 75

[Jan Horak [:jhorak]]

We're experiencing problem with visibility of STL header ostream. It fails to build on GCC 4.1.2 which is used for Red Hat Enterprise Linux 5. Assign 'ostream' to 'stl-headers' fix this issue. I'm unsure if 'ostream' could be reviewed and added to 'stl-headers'.

Comment 76

[(no longer active)]

(In reply to comment #75)
> We're experiencing problem with visibility of STL header ostream. It fails to
> build on GCC 4.1.2 which is used for Red Hat Enterprise Linux 5. Assign
> 'ostream' to 'stl-headers' fix this issue. I'm unsure if 'ostream' could be
> reviewed and added to 'stl-headers'.

Please file a new bug for that?

Comment 77

[Jan Horak [:jhorak]]

Okay, filled bug 806382. Thanks.