[Windows] After bug 551254, Firefox/SeaMonkey fails to build or run with error like "?_Throw@std@@YAXABVexception@stdext@@@Z"

RESOLVED FIXED in mozilla1.9.3a5

Status

()

defect
--
blocker
RESOLVED FIXED
9 years ago
9 years ago

People

(Reporter: sgautherie, Assigned: cjones)

Tracking

({regression})

Trunk
mozilla1.9.3a5
x86
Windows 2000
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(blocking2.0 -)

Details

Attachments

(6 attachments, 4 obsolete attachments)

4.20 KB, patch
kairo
: review+
cjones
: feedback+
Callek
: feedback+
Details | Diff | Splinter Review
1.43 KB, patch
benjamin
: review+
sgautherie
: feedback+
neil
: feedback+
Details | Diff | Splinter Review
35.71 KB, patch
benjamin
: superreview+
Details | Diff | Splinter Review
419 bytes, patch
cjones
: review+
Details | Diff | Splinter Review
1.30 KB, patch
kairo
: review+
Details | Diff | Splinter Review
3.81 KB, patch
cjones
: review+
Details | Diff | Splinter Review
I downloaded the "most recent" build and discovered this failure.

I haven't verified but I assume this is a regression from bug 551254.
[Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.9.3a4pre) Gecko/20100403 SeaMonkey/2.1a1pre] (nightly)

Fwiw, latest (optimized) nightly works fine.
The oldest available build was already broken:
/pub/seamonkey/tinderbox-builds/comm-central-trunk-win32-debug/1270270824
"20100402220024 a0eee84feb71"

The newest optimized  build works fine:
/pub/seamonkey/tinderbox-builds/comm-central-trunk-win32/1270349077
[Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.9.3a4pre) Gecko/20100403 SeaMonkey/2.1a1pre]
I assume this should help c-c ... If only we were told beforehand!

First reviewer will have my thanks.
(Anyone can check this in if I don't do it first.)
Attachment #436904 - Flags: review?(kairo)
Attachment #436904 - Flags: review?(bugspam.Callek)
Attachment #436904 - Flags: feedback?(jones.chris.g)
Comment on attachment 436904 [details] [diff] [review]
(Av1-CC) Copy (the useful part of) it
[Checkin: See comment 6]

did not do a "real" review; but as a skim this looks good; I either want to review myself; or have someone else review before we land this though.
Attachment #436904 - Flags: feedback+
(In reply to comment #4)
> (From update of attachment 436904 [details] [diff] [review])
> did not do a "real" review; but as a skim this looks good; I either want to
> review myself; or have someone else review before we land this though.

For the record; if Chris Jones wants to do a review on this [rather than simply feedback], and thinks this will fix us, I'm happy to accept his r+ as "ok to land"
Attachment #436904 - Flags: review?(kairo) → review+
Comment on attachment 436904 [details] [diff] [review]
(Av1-CC) Copy (the useful part of) it
[Checkin: See comment 6]


http://hg.mozilla.org/comm-central/rev/8c3e917767fa
+
http://hg.mozilla.org/comm-central/rev/d0cd47f97d7c
(Bv1) Add dumb 'true'.
Attachment #436904 - Attachment description: (Av1) Copy (the useful part of) it → (Av1) Copy (the useful part of) it [Checkin: See comment 6]
Attachment #436904 - Flags: review?(bugspam.Callek)
The port is done :-) but it made no difference :-(
Blocks: 557120
An optimized SeaMonkey local compilation fails too:
{
Building deps for .../modules/libpr0n/build/nsImageModule.cpp
nsImageModule.cpp
.../modules/libpr0n/build/../decoders/bmp\nsBMPDecoder.h(123) : warning C4005: 'BI_RLE8' : macro redefinition
        X:\Program Files\Microsoft Platform SDK for Windows Server 2003 R2\include\wingdi.h(750) : see previous definition of 'BI_RLE8'
.../modules/libpr0n/build/../decoders/bmp\nsBMPDecoder.h(124) : warning C4005: 'BI_RLE4' : macro redefinition
        X:\Program Files\Microsoft Platform SDK for Windows Server 2003 R2\include\wingdi.h(751) : see previous definition of 'BI_RLE4'
.../modules/libpr0n/build/../decoders/bmp\nsBMPDecoder.h(125) : warning C4005: 'BI_BITFIELDS' : macro redefinition
        X:\Program Files\Microsoft Platform SDK for Windows Server 2003 R2\include\wingdi.h(752) : see previous definition of 'BI_BITFIELDS'
Creating Resource file: module.res
   Creating library fake.lib and object fake.exp
imglib2_s.lib(imgLoader.obj) : error LNK2019: unresolved external symbol "__declspec(dllimport) void __cdecl std::_Throw(class std::exception const &)" (__imp_?_Throw@std@@YAXABVexception@1@@Z) referenced in function "protected: virtual void __thiscall std::bad_alloc::_Doraise(void)const " (?_Doraise@bad_alloc@std@@MBEXXZ)
imglib2_s.lib(imgLoader.obj) : error LNK2019: unresolved external symbol "__declspec(dllimport) public: void __thiscall std::exception::_Raise(void)const " (__imp_?_Raise@exception@std@@QBEXXZ) referenced in function "protected: static void __cdecl std::vector<class nsRefPtr<class imgCacheEntry>,class std::allocator<class nsRefPtr<class imgCacheEntry> > >::_Xlen(void)" (?_Xlen@?$vector@V?$nsRefPtr@VimgCacheEntry@@@@V?$allocator@V?$nsRefPtr@VimgCacheEntry@@@@@std@@@std@@KAXXZ)

imglib2.dll : fatal error LNK1120: 2 unresolved externals
make[6]: *** [imglib2.dll] Error 96
}
Previous summary was:
[Debug Windows SeaMonkey 2.1] Startup fails with Windows error dialog "?_Throw@std@@YAXABVexception@stdext@@@Z entry point not found in MSVCP80D.dll"

****

I just tried a Firefox build (Win2000 + 2003R2 SDK + ...):
{
Building deps for nsRDFResource.cpp
nsRDFResource.cpp
X:\Program Files\Microsoft Visual Studio 8\VC\INCLUDE\string.h(205) : warning C4273: 'moz_strdup' : inconsistent dll linkage
        ...\dist\include\mozilla/mozalloc.h(138): see previous definition of 'moz_strdup'
Creating Resource file: module.res
   Creating library xul.lib and object xul.exp
xulapp_s.lib(exception_handler.obj) : error LNK2019: unresolved external symbol "__declspec(dllimport) void __cdecl std::_Throw(class std::exception const &)" (__imp_?_Throw@std@@YAXABVexception@1@@Z) referenced in function "protected: virtual void __thiscall std::bad_alloc::_Doraise(void)const " (?_Doraise@bad_alloc@std@@MBEXXZ)
imglib2.lib(imgLoader.obj) : error LNK2001: unresolved external symbol "__declspec(dllimport) void __cdecl std::_Throw(class std::exception const &)" (__imp_?_Throw@std@@YAXABVexception@1@@Z)
xulapp_s.lib(exception_handler.obj) : error LNK2019: unresolved external symbol "__declspec(dllimport) public: void __thiscall std::exception::_Raise(void)const " (__imp_?_Raise@exception@std@@QBEXXZ) referenced in function "protected: static void __cdecl std::vector<class google_breakpad::ExceptionHandler *,class std::allocator<class google_breakpad::ExceptionHandler *> >::_Xran(void)" (?_Xran@?$vector@PAVExceptionHandler@google_breakpad@@V?$allocator@PAVExceptionHandler@google_breakpad@@@std@@@std@@KAXXZ)
imglib2.lib(imgLoader.obj) : error LNK2001: unresolved external symbol "__declspec(dllimport) public: void __thiscall std::exception::_Raise(void)const " (__imp_?_Raise@exception@std@@QBEXXZ)
xul.dll : fatal error LNK1120: 2 unresolved externals
make[4]: *** [xul.dll] Error 96
make[4]: Leaving directory `.../toolkit/library'
}

The failure happens at a different location but seems to be basically the same issue.
Summary: [Debug Windows SeaMonkey 2.1] Startup fails with Windows error dialog "?_Throw@std@@YAXABVexception@stdext@@@Z entry point not found in MSVCP80D.dll" → [Windows] After bug 551254, Firefox/SeaMonkey fails to build or run with error like "?_Throw@std@@YAXABVexception@stdext@@@Z"
I was only able to get imgLoader.cpp and nptest.cpp to link by adding STL_FLAGS= and WRAP_STL_INCLUDES= to the appropriate Makefiles :-(
Works for VC9/Win7 SDK, VC9/server2k3 SDK gives redefinition error for std::exception::_Raise (which is expected).  Not sure that this will work, because my local build doesn't seem to be using the xpcom/glue/stdthrow.h definition of _Throw (might be a __declspec issue).

Serge, I'd really appreciate it if you could try this patch locally.  I'll resuming poking at this tomorrow.
Assignee: nobody → jones.chris.g
Attachment #437803 - Flags: feedback?(sgautherie.bz)
Comment on attachment 436904 [details] [diff] [review]
(Av1-CC) Copy (the useful part of) it
[Checkin: See comment 6]

Looks OK to me.
Attachment #436904 - Flags: feedback?(jones.chris.g) → feedback+
(In reply to comment #10)
> I was only able to get imgLoader.cpp and nptest.cpp to link by adding
> STL_FLAGS= and WRAP_STL_INCLUDES= to the appropriate Makefiles :-(

The nptest problem is known, patch awaiting review.  Is the imgLoader error you saw what Serge reports above?
Comment on attachment 437803 [details] [diff] [review]
Attempt to fix win2k bustage, v1


With this patch:
{
Building deps for .../modules/libpr0n/src/imgLoader.cpp
imgLoader.cpp
../../../dist/include\mozilla/stdthrow.h(69) : error C2084: function 'void std::exception::_Raise(void) const' already has a body
        X:\Program Files\Microsoft Visual Studio 8\VC\INCLUDE\exception(260) : see previous definition of '_Raise'
make[5]: *** [imgLoader.obj] Error 2
}
Attachment #437803 - Flags: feedback?(sgautherie.bz) → feedback-
Same compilation environment?  This error doesn't make sense to me given

(In reply to comment #9)
> I just tried a Firefox build (Win2000 + 2003R2 SDK + ...):
> {
[snip]
> xulapp_s.lib(exception_handler.obj) : error LNK2019: unresolved external symbol
> "__declspec(dllimport) public: void __thiscall
> std::exception::_Raise(void)const " (__imp_?_Raise@exception@std@@QBEXXZ)
> referenced in function "protected: static void __cdecl std::vector<class
> google_breakpad::ExceptionHandler *,class std::allocator<class
> google_breakpad::ExceptionHandler *> >::_Xran(void)"
[snip]
> }
(In reply to comment #15)

> Same compilation environment?

Very same.

> This error doesn't make sense to me given

I'm not sure what you mean would make more sense?
(In reply to comment #16)
> (In reply to comment #15)
> > This error doesn't make sense to me given
> 
> I'm not sure what you mean would make more sense?

The original error message complained about std::exception::_Raise being undefined at link time.  I guessed that _Raise wasn't inlined or exported by your VC/SDK/CRT, so the v1 patch added a definition of _Raise.  But with patch applied, MSVC complained about _Raise being multiply defined, which means it *is* being defined inline in your toolkit's headers (the second definition comes from the v1 patch).  That confuses me.
Little more heavyweight solution, but has some ancillary benefits.  Eliminates the dependency on std::_Throw in my tests.
Attachment #437803 - Attachment is obsolete: true
Attachment #438139 - Flags: feedback?(sgautherie.bz)
Comment on attachment 438139 [details] [diff] [review]
Attempt to fix win2k bustage, v2


This patch fixes the _Throw cases, but not the _Raise ones:
{
nsRDFResource.cpp
Creating Resource file: module.res
   Creating library xul.lib and object xul.exp
xulapp_s.lib(exception_handler.obj) : error LNK2019: unresolved external symbol "__declspec(dllimport) public: void __thiscall std::exception::_Raise(void)const " (__imp_?_Raise@exception@std@@QBEXXZ) referenced in function "protected: static void __cdecl std::vector<class google_breakpad::ExceptionHandler *,class std::allocator<class google_breakpad::ExceptionHandler *> >::_Xran(void)" (?_Xran@?$vector@PAVExceptionHandler@google_breakpad@@V?$allocator@PAVExceptionHandler@google_breakpad@@@std@@@std@@KAXXZ)
imglib2.lib(imgLoader.obj) : error LNK2001: unresolved external symbol "__declspec(dllimport) public: void __thiscall std::exception::_Raise(void)const " (__imp_?_Raise@exception@std@@QBEXXZ)
xul.dll : fatal error LNK1120: 1 unresolved externals
make[4]: *** [xul.dll] Error 96
make[4]: Leaving directory `.../toolkit/library'
}

Nit: The nsprpub/configure diff should not be in this patch.
Attachment #438139 - Flags: feedback?(sgautherie.bz) → feedback-
Sigh, had hoped fixing _Throw() would make these magically go away.

This error still doesn't make sense to me.  Serge, may I ask a favor?  Would you run

 $ make -C $objdir/modules/libpr0n/src imgLoader.i

and upload that?  (Warning: it'll be pretty large.)
Thanks Serge.  So, the problem is what I feared: these VC/SDK headers are idiotically declaring

  class __declspec(dllimport) exception {

even though all the methods of "exception" are inline.  __declspec(dllimport) semantics (which I learned about after the failed attempt of comment 11) say that even if a __declspec(dllimport) symbol has an inline definition, the translation unit retains a dependency on the __declspec(dllimport) symbol.  This means that even though std::exception::_Raise() is defined inline to call std::_Throw(), and we've interposed _Throw() successfully, libxul retains a meaningless dependency on the std::exception::_Raise symbol (would never be used).

This is good and bad news.  Good news is that while the v2 patch above won't compile on MSVC7/2k3SDK, tinderbox and release builds including it *will* run on win2k, because they won't emit the braindead dependency on std::exception::_Raise() and will have the interposed std::_Throw().  So I'll clean up that patch and get the ball rolling.  (The interposed _Throw() is useful for other reasons too.)

Bad news is that it looks to be much harder to work around the _Raise issue than the _Throw one.  I think the best solution there is just to disable STL wrappers for older build environments.

Serge, you said you were using the Win2k3 server SDK, but which compiler?  VC7?  (I tried the 2k3 SDK on my machine with MSVC9, but couldn't repro the bustage, so I think these headers are coming from MSVC.)
(In reply to comment #21)

> I think the best solution there is just to disable STL
> wrappers for older build environments.

Might be: it seems Neil (successfully) tried that in comment 10.

> Serge, you said you were using the Win2k3 server SDK, but which compiler?

'Visual C++ 8 Express' + 'Platform SDK for Windows Server 2003 R2' +
{
ac_add_options --with-windows-version=502
ac_add_options --disable-ctypes
ac_add_options --disable-ipc
}
(r? -> ehsan for the MSVC bits, if he wishes to review them.)

Before getting into the means of this patch, consider the ends

 - Mozilla-interposed exception handling on MSVC and gcc.  We can define whatever "throwing an exception from the STL" should mean.
 - Centralized "throw" logic; MSVC and gcc share the same.
 - Exception safety (on Tier I's) for all of Gecko, including ipc/chromium/, for the first time ever AFAIK

Background: For code compiled by MSVC with _HAS_EXCEPTIONS=0, its headers call |std::_Throw(e)| instead of |throw e| when an exception is to be "thrown" from VC headers.  std::_Throw() prints an error message and then aborts().

Means: Let's pretend that this bug didn't exist, and std::_Throw() worked on all windows versions.  According to http://hg.mozilla.org/mozilla-central/file/b66b7330b505/xpcom/base/nsDebugImpl.cpp#l374, abort() can't be trusted to invoke breakpad on windows.  We definitely want breakpad for STL exceptions, so interposing a Mozilla std::_Throw() would be propitious.  This is a bug in its own right.

Interposing a Mozilla std::_Throw() is gross, granted.  This patch does |#define _Throw(e) mozilla_Throw(e)| before including <exception> (which declares std::_Throw), then exports std::mozilla_Throw from mozalloc.dll.  The indirection includes a compile-time guard that prevents any Gecko code from linking against std::_Throw(), so new code can't regress the interposition.

std::mozilla_Throw() does the same thing as the interposed std::__throw_[exception]()s on gcc: calls mozalloc_abort().  mozalloc_handle_oom() also calls that now.

This fixes Serge's bug because msvcrt7.dll (or whatever the win2k CRT is) doesn't export std::_Throw().  I think this omission is analogous to gcc's "support" for -fshort-wchar (and others): "Oh, you just need to recompile the entire runtime."  Luckily it's been fixed in later versions.
Attachment #438139 - Attachment is obsolete: true
Attachment #438239 - Flags: superreview?(benjamin)
Attachment #438239 - Flags: review?(ehsan)
Optimized builds work fine:
[Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.9.3a5pre) Gecko/20100410 Minefield/3.7a5pre] (nightly)
[Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.9.3a5pre) Gecko/20100410 Minefield/3.7a5pre] (mozilla-central-win32/1270897138)
+
[Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.9.3a5pre) Gecko/20100410 SeaMonkey/2.1a1pre] (nightly)
[Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.9.3a5pre) Gecko/20100410 SeaMonkey/2.1a1pre] (comm-central-trunk-win32/1270896177)

Debug builds fail to start:
Bug 557060 '_Throw' (+ Bug 558559 'RtlCaptureStackBackTrace') : [FF] (mozilla-central-win32-debug/1270891739)
Bug 557060 '_Throw' : [SM] (comm-central-trunk-win32-debug/1270896370)

As a reminder, there's 2 issues (wrt this bug) on (my) Windows 2000:
*Starting "official" Debug builds.
*Compiling local (Optimized) builds.


(In reply to comment #23)
> This fixes Serge's bug because msvcrt7.dll (or whatever the win2k CRT is)
> doesn't export std::_Throw().

(I'm not sure what this patch is fixing for me.)

Debug builds are loading msvcr80d.dll (v8.0.50727.42).
It's the only Debug dll I have, whereas I have (msvcr71.dll,) msvcr80.dll and msvcr90.dll.
Notice that Debug builds package their own msvcr80.dll (v8.0.50727.762, which they're not using):
Is it worth they package the Optimized dlls?
Should they package the 8.0 Debug dlls instead, fwiw!?
Should they moreover package 9.0 Debug dlls, if that exists??

NB: Optimized builds load their own mozcrt19.dll...


(In reply to comment #24)
> Do you mind trying it out?

[Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.9.3a5pre) Gecko/20100409 Minefield/3.7a5pre] (cjones@mozilla.com-try-0291b4f0fc92)
"--enable-optimize --disable-debug --enable-jemalloc"

This Optimized try build _still_ works :-)

Yet, this (new) patch gives me the same comment 19 errors.
NB: Fwiw, my local builds are without --enable-jemalloc.
Which debug build fails to start?

To repeat what I said in comment 21, the latest patch won't fix your local build errors.  A configure.in patch is needed for that.
(In reply to comment #26)
> Which debug build fails to start?

Tinderbox ones.
OK.  That shouldn't have changed, since they're not building with attachment 438239 [details] [diff] [review].
This (should) fix compilation errors by disabling STL_WRAPPERS for older build environments.  (Test wfm locally in that it doesn't detect the bug.)
Attachment #438279 - Flags: feedback?(sgautherie.bz)
Attachment #438279 - Flags: feedback?(neil)
Comment on attachment 438239 [details] [diff] [review]
Interpose _Throw() on MSVC and centralize pseudo-throw code in mozalloc

>-COMPILE_CXXFLAGS = $(VISIBILITY_FLAGS) $(STL_FLAGS) $(DEFINES) $(INCLUDES) $(DSO_CFLAGS) $(DSO_PIC_CFLAGS) $(CXXFLAGS) $(RTL_FLAGS) $(OS_COMPILE_CXXFLAGS)
>+COMPILE_CXXFLAGS = $(STL_FLAGS) $(VISIBILITY_FLAGS) $(DEFINES) $(INCLUDES) $(DSO_CFLAGS) $(DSO_PIC_CFLAGS) $(CXXFLAGS) $(RTL_FLAGS) $(OS_COMPILE_CXXFLAGS)

Why are these changes necessary?

>diff --git a/memory/mozalloc/mozalloc_abort.cpp b/memory/mozalloc/mozalloc_abort.cpp
>+#if defined(_WIN32)
>+#  if !defined(WINCE)
>+    //This should exit us
>+    raise(SIGABRT);
>+#  endif

Would it make sense here to call RaiseException, at least for Windows CE?

>+    //If we are ignored exit this way..
>+    _exit(3);
>+#elif defined(XP_UNIX) || defined(XP_OS2) || defined(XP_BEOS)
>+    abort();
>+#else
>+#  warning not attempting to abort() on this platform

Shouldn't this be an #error?

>+#endif
>+
>+    // Still haven't aborted?  Try dereferencing null.
>+    // (Written this way to lessen the likelihood of it being optimized away.)
>+    gDummyCounter += *((int*) 0); // TODO annotation saying we know 
>+    // this is crazy

In order to make sure that we'll cause a crash, there is a better technique.

  __debugbreak();

This is guaranteed to work, and won't get optimized away.

>diff --git a/memory/mozalloc/throw_msvc.h b/memory/mozalloc/throw_msvc.h
>+#define _Throw(_e)  mozilla_Throw(_e)

Change this to:

#define _Throw mozilla_Throw

to make sure that any possible crazy code which attempts to get the address of std::_Throw is also handled.

>diff --git a/memory/mozalloc/throw_msvc.cpp b/memory/mozalloc/throw_msvc.cpp
>+namespace std {
>+
>+__declspec(dllexport) void mozilla_Throw(const exception& e);
>+
>+void
>+mozilla_Throw(const exception& e)
>+{
>+    mozalloc_abort(e.what());
>+}
>+
>+}  // namespace std

Since C++ programs aren't officially supposed to touch the std namespace, maybe add a comment here explaining why this is OK.  The same thing goes for the gcc version of this code.
Comment on attachment 438279 [details] [diff] [review]
Check for |class __declspec(dllimport) exception| bug in older VC/SDKs
[Checkin: Comment 46]

This successfully disables the STL bits for me.
Attachment #438279 - Flags: feedback?(neil) → feedback+
Attachment #438279 - Flags: feedback?(sgautherie.bz) → feedback+
Comment on attachment 438279 [details] [diff] [review]
Check for |class __declspec(dllimport) exception| bug in older VC/SDKs
[Checkin: Comment 46]


With this patch alone, I can build Firefox again :-)

Does this have to be an AC_TRY_LINK() or could it be based on the SDK version or --with-windows-version value for example?
I'm not sure what disabling this "feature" means: would it make sense to explicitly "warn" the developer?
(In reply to comment #32)
> (From update of attachment 438279 [details] [diff] [review])
> Does this have to be an AC_TRY_LINK() or could it be based on the SDK version
> or --with-windows-version value for example?

It's not clear to me which VC/SDKs trigger the failure.  Based on what I've seen so far, it appears that

  VC9/win7:    fine (my build configuration)
  VC9/win2k3:  fine (I tested this)
  VC8/win7:    fine (build slaves)
  VC8/win2k3   broken (Serge's build config)

So, I prefer the feature test.

> I'm not sure what disabling this "feature" means: would it make sense to
> explicitly "warn" the developer?

It means the build won't be exception safe.  Not sure if it's worth warning about, since it's only disabled for pretty old build setups.
(In reply to comment #33)

> So, I prefer the feature test.

Thanks for the list, it's just fine ftr ;-)

> It means the build won't be exception safe.

Ftr, could you give an example of such an exception (case) and how an unsafe build will behave?
(Also for "me" to know if/when it happens with such a build and not mistake that for a bug...)
(In reply to comment #34)
> (In reply to comment #33)
> > It means the build won't be exception safe.
> 
> Ftr, could you give an example of such an exception (case) and how an unsafe
> build will behave?
> (Also for "me" to know if/when it happens with such a build and not mistake
> that for a bug...)

Well TBH, Gecko has never been exception safe, so if you haven't seen any bugs yet you probably won't see new ones ;).  The problem with exceptions is plugins and binary extensions: if one installs an exception handler and calls into Gecko, and Gecko throws an exception, then the plugin/extension will let execution resume even though the heap is in an unknown and possibly inconsistent/corrupted state.

But, if your plugins and binary extensions don't do that, then in practice there's no difference between "exception safe" and "exception unsafe" builds.
(In reply to comment #30)
> (From update of attachment 438239 [details] [diff] [review])
> >-COMPILE_CXXFLAGS = $(VISIBILITY_FLAGS) $(STL_FLAGS) $(DEFINES) $(INCLUDES) $(DSO_CFLAGS) $(DSO_PIC_CFLAGS) $(CXXFLAGS) $(RTL_FLAGS) $(OS_COMPILE_CXXFLAGS)
> >+COMPILE_CXXFLAGS = $(STL_FLAGS) $(VISIBILITY_FLAGS) $(DEFINES) $(INCLUDES) $(DSO_CFLAGS) $(DSO_PIC_CFLAGS) $(CXXFLAGS) $(RTL_FLAGS) $(OS_COMPILE_CXXFLAGS)
> 
> Why are these changes necessary?
> 

gcc needs to find headers in $DIST/stl_wrappers before $DIST/system_wrappers, because the system_wrappers/ headers change symbol visibility, and stl_wrappers/ headers include other Gecko headers.

> >diff --git a/memory/mozalloc/mozalloc_abort.cpp b/memory/mozalloc/mozalloc_abort.cpp
> >+#if defined(_WIN32)
> >+#  if !defined(WINCE)
> >+    //This should exit us
> >+    raise(SIGABRT);
> >+#  endif
> 
> Would it make sense here to call RaiseException, at least for Windows CE?
> 

I don't know, would it?  I copied and pasted this code from xpcom/base/nsDebugImpl.cpp:Abort().

> >+    //If we are ignored exit this way..
> >+    _exit(3);
> >+#elif defined(XP_UNIX) || defined(XP_OS2) || defined(XP_BEOS)
> >+    abort();
> >+#else
> >+#  warning not attempting to abort() on this platform
> 
> Shouldn't this be an #error?
> 

Why do you think so?  We fall through to code that dereferences NULL and then calls _exit(), so I'm not sure that the lack of abort() adds risk.

> >+#endif
> >+
> >+    // Still haven't aborted?  Try dereferencing null.
> >+    // (Written this way to lessen the likelihood of it being optimized away.)
> >+    gDummyCounter += *((int*) 0); // TODO annotation saying we know 
> >+    // this is crazy
> 
> In order to make sure that we'll cause a crash, there is a better technique.
> 
>   __debugbreak();
> 
> This is guaranteed to work, and won't get optimized away.
> 

Interesting!  This should be fixed in nsDebugImpl.cpp as well.  Mind if we do that in a followup bug?

> >diff --git a/memory/mozalloc/throw_msvc.h b/memory/mozalloc/throw_msvc.h
> >+#define _Throw(_e)  mozilla_Throw(_e)
> 
> Change this to:
> 
> #define _Throw mozilla_Throw
> 
> to make sure that any possible crazy code which attempts to get the address of
> std::_Throw is also handled.
> 

The CRT doesn't do that (sez grep).  Would you r+ a Gecko patch that tried to take the address of a private MSVC-only STL symbol? ;)
(In reply to comment #36)
> (In reply to comment #30)
> > (From update of attachment 438239 [details] [diff] [review] [details])
> > >-COMPILE_CXXFLAGS = $(VISIBILITY_FLAGS) $(STL_FLAGS) $(DEFINES) $(INCLUDES) $(DSO_CFLAGS) $(DSO_PIC_CFLAGS) $(CXXFLAGS) $(RTL_FLAGS) $(OS_COMPILE_CXXFLAGS)
> > >+COMPILE_CXXFLAGS = $(STL_FLAGS) $(VISIBILITY_FLAGS) $(DEFINES) $(INCLUDES) $(DSO_CFLAGS) $(DSO_PIC_CFLAGS) $(CXXFLAGS) $(RTL_FLAGS) $(OS_COMPILE_CXXFLAGS)
> > 
> > Why are these changes necessary?
> > 
> 
> gcc needs to find headers in $DIST/stl_wrappers before $DIST/system_wrappers,
> because the system_wrappers/ headers change symbol visibility, and
> stl_wrappers/ headers include other Gecko headers.
> 

Oh, and I should add that the reason why *that's* important is that STL headers are in both the stl-headers and system-wrappers list, because we build code that uses our hidden-visibility flags but can't use our STL wrappers.
Flags: wanted-fennec1.0?
Er, not sure how I set that flag.  Unintentional.
Flags: wanted-fennec1.0?
(In reply to comment #36)
> > >diff --git a/memory/mozalloc/mozalloc_abort.cpp b/memory/mozalloc/mozalloc_abort.cpp
> > >+#if defined(_WIN32)
> > >+#  if !defined(WINCE)
> > >+    //This should exit us
> > >+    raise(SIGABRT);
> > >+#  endif
> > 
> > Would it make sense here to call RaiseException, at least for Windows CE?
> > 
> 
> I don't know, would it?  I copied and pasted this code from
> xpcom/base/nsDebugImpl.cpp:Abort().

I think we should do this, yes.

> > >+    //If we are ignored exit this way..
> > >+    _exit(3);
> > >+#elif defined(XP_UNIX) || defined(XP_OS2) || defined(XP_BEOS)
> > >+    abort();
> > >+#else
> > >+#  warning not attempting to abort() on this platform
> > 
> > Shouldn't this be an #error?
> > 
> 
> Why do you think so?  We fall through to code that dereferences NULL and then
> calls _exit(), so I'm not sure that the lack of abort() adds risk.

Oh, right.  My oversight.

> > >+#endif
> > >+
> > >+    // Still haven't aborted?  Try dereferencing null.
> > >+    // (Written this way to lessen the likelihood of it being optimized away.)
> > >+    gDummyCounter += *((int*) 0); // TODO annotation saying we know 
> > >+    // this is crazy
> > 
> > In order to make sure that we'll cause a crash, there is a better technique.
> > 
> >   __debugbreak();
> > 
> > This is guaranteed to work, and won't get optimized away.
> > 
> 
> Interesting!  This should be fixed in nsDebugImpl.cpp as well.  Mind if we do
> that in a followup bug?

Sure, no problem.  Will you file that or do you want me to?

> > >diff --git a/memory/mozalloc/throw_msvc.h b/memory/mozalloc/throw_msvc.h
> > >+#define _Throw(_e)  mozilla_Throw(_e)
> > 
> > Change this to:
> > 
> > #define _Throw mozilla_Throw
> > 
> > to make sure that any possible crazy code which attempts to get the address of
> > std::_Throw is also handled.
> > 
> 
> The CRT doesn't do that (sez grep).  Would you r+ a Gecko patch that tried to
> take the address of a private MSVC-only STL symbol? ;)

No!  But I wasn't talking about Gecko code, I was talking about STL code (in a future VC version, for example.)  Better safe than sorry, right?
(In reply to comment #39)
> (In reply to comment #36)
> > > >diff --git a/memory/mozalloc/mozalloc_abort.cpp b/memory/mozalloc/mozalloc_abort.cpp
> > > >+#if defined(_WIN32)
> > > >+#  if !defined(WINCE)
> > > >+    //This should exit us
> > > >+    raise(SIGABRT);
> > > >+#  endif
> > > 
> > > Would it make sense here to call RaiseException, at least for Windows CE?
> > > 
> > 
> > I don't know, would it?  I copied and pasted this code from
> > xpcom/base/nsDebugImpl.cpp:Abort().
> 
> I think we should do this, yes.
> 
> > > >+#endif
> > > >+
> > > >+    // Still haven't aborted?  Try dereferencing null.
> > > >+    // (Written this way to lessen the likelihood of it being optimized away.)
> > > >+    gDummyCounter += *((int*) 0); // TODO annotation saying we know 
> > > >+    // this is crazy
> > > 
> > > In order to make sure that we'll cause a crash, there is a better technique.
> > > 
> > >   __debugbreak();
> > > 
> > > This is guaranteed to work, and won't get optimized away.
> > > 
> > 
> > Interesting!  This should be fixed in nsDebugImpl.cpp as well.  Mind if we do
> > that in a followup bug?
> 
> Sure, no problem.  Will you file that or do you want me to?
> 

Filed 558928 for these two items.

> > > >diff --git a/memory/mozalloc/throw_msvc.h b/memory/mozalloc/throw_msvc.h
> > > >+#define _Throw(_e)  mozilla_Throw(_e)
> > > 
> > > Change this to:
> > > 
> > > #define _Throw mozilla_Throw
> > > 
> > > to make sure that any possible crazy code which attempts to get the address of
> > > std::_Throw is also handled.
> > > 
> > 
> > The CRT doesn't do that (sez grep).  Would you r+ a Gecko patch that tried to
> > take the address of a private MSVC-only STL symbol? ;)
> 
> No!  But I wasn't talking about Gecko code, I was talking about STL code (in a
> future VC version, for example.)  Better safe than sorry, right?

*shrug*.  I tend to disagree, but it's not worth getting hung up on ;).
Comment on attachment 438239 [details] [diff] [review]
Interpose _Throw() on MSVC and centralize pseudo-throw code in mozalloc

I thought I'd set the flag here.  I'd prefer that you change the _Throw macro, but I won't hold up the review on that.  :-)
Attachment #438239 - Flags: review?(ehsan) → review+
No problem.  I was going to post a new patch updated per your comments after fixing a mac tinderbox issue, but I can't repro locally so hopefully I pushed a bad base rev.
(In reply to comment #42)
> No problem.  I was going to post a new patch updated per your comments after
> fixing a mac tinderbox issue, but I can't repro locally so hopefully I pushed a
> bad base rev.

Yup, re-pushed with a different base rev and got all green.  Guess I'll chalk up the earlier errors to cosmic rays.
Attachment #438239 - Flags: superreview?(benjamin)
No longer blocks: 557050
Attachment #438279 - Flags: review?(benjamin) → review+
Comment on attachment 438679 [details] [diff] [review]
Interpose _Throw() on MSVC and centralize pseudo-throw code in mozalloc, v2
[Checkin: Comment 46]

The only worry I have about this is making it more future-proof: what if a future version of MSVC doesn't use std::_Throw? Could we add a configure test to ensure that *calling* std::_Throw compiles, to at least detect this case?
Attachment #438679 - Flags: superreview?(benjamin) → superreview+
That future problem is "now" -- VS2010 does not have _Throw and doesn't define it in <exception>, so the configure check fails; not possible to build with VS2010 after this landed.
(In reply to comment #47)
> That future problem is "now" -- VS2010 does not have _Throw and doesn't define
> it in <exception>, so the configure check fails; not possible to build with
> VS2010 after this landed.

Now filed as bug 560723.
After f0ccd76765b9, I got the following error on Linux.

../../dist/include/mozilla/throw_gcc.h: In function ‘void std::__throw_system_error(int)’:
../../dist/include/mozilla/throw_gcc.h:147: error: ‘strerror’ was not declared in this scope
Add string.h to #include.
Attachment #440417 - Flags: review?(benjamin)
Attachment #438279 - Attachment description: Check for |class __declspec(dllimport) exception| bug in older VC/SDKs → Check for |class __declspec(dllimport) exception| bug in older VC/SDKs [Checkin: Comment 46]
Attachment #438679 - Attachment description: Interpose _Throw() on MSVC and centralize pseudo-throw code in mozalloc, v2 → Interpose _Throw() on MSVC and centralize pseudo-throw code in mozalloc, v2 [Checkin: Comment 46]
Attachment #438239 - Attachment is obsolete: true
Attachment #436904 - Attachment description: (Av1) Copy (the useful part of) it [Checkin: See comment 6] → (Av1-CC) Copy (the useful part of) it [Checkin: See comment 6]
(Whoever review it first...)
Attachment #440450 - Flags: review?(bugzilla)
Attachment #440450 - Flags: review?(bugspam.Callek)
(Whoever review it first...)
Attachment #440452 - Flags: review?(bugzilla)
Attachment #440452 - Flags: review?(bugspam.Callek)
Target Milestone: --- → mozilla1.9.3a5
Comment on attachment 440450 [details] [diff] [review]
(Fv1-CC) Copy config.mk part of m-c fix
[Checkin: Comment 55]

Callek can, I'm busy with lots of other things.
Attachment #440450 - Flags: review?(bugzilla)
Attachment #440450 - Flags: review?(kairo)
Attachment #440452 - Flags: review?(kairo)
Changes to configure.in from this bug broke mingw build. std::_Throw and |class __declspec(dllimport) exception| are specific to MSVC, so they should be moved to MSVC-specific part.
Attachment #440457 - Flags: review?(jones.chris.g)
Attachment #440450 - Flags: review?(kairo)
Attachment #440450 - Flags: review?(bugspam.Callek)
Attachment #440450 - Flags: review+
Attachment #440452 - Flags: review?(kairo)
Attachment #440452 - Flags: review?(bugspam.Callek)
Attachment #440452 - Flags: review+
Comment on attachment 440450 [details] [diff] [review]
(Fv1-CC) Copy config.mk part of m-c fix
[Checkin: Comment 55]


http://hg.mozilla.org/comm-central/rev/7d00aae318b9
Attachment #440450 - Attachment description: (Fv1-CC) Copy config.mk part of m-c fix → (Fv1-CC) Copy config.mk part of m-c fix [Checkin: Comment 55]
Attachment #440417 - Flags: review?(benjamin) → review?(jones.chris.g)
Attachment #440417 - Flags: review?(jones.chris.g) → review+
Attachment #440457 - Flags: review?(jones.chris.g) → review+
Keywords: checkin-needed
Comment on attachment 440452 [details] [diff] [review]
(Gv1-CC) Copy (the useful part of) m-c configure.in fix
[Supeseded by bug 562655]


Looking for "approval-thunderbird3.1b2=?" actually.
Attachment #440452 - Flags: feedback?(dmose)
Attachment #440452 - Flags: feedback?(bugzilla)
Attachment #440452 - Flags: feedback?(dmose)
Attachment #440452 - Flags: feedback?(bugzilla)
Comment on attachment 440452 [details] [diff] [review]
(Gv1-CC) Copy (the useful part of) m-c configure.in fix
[Supeseded by bug 562655]

Without any description as to why this should get approval to land in a code freeze you're not going to get it - especially given that it doesn't even affect comm-1.9.2 which is what we're frozen for.
(In reply to comment #58)
> especially given that it doesn't even
> affect comm-1.9.2 which is what we're frozen for.

Indeed: that's exactly why I don't see why a "c-1.9.2" freeze should block this c-c sync with m-c :-/
Attachment #440417 - Attachment description: Fix for comment #49 → Fix for comment #49 [Checkin: Comment 56]
Attachment #440457 - Attachment description: mingw fix → mingw fix [Checkin: Comment 56]
Chris, is it possible that this broke XULRunner? See bug 559447 comment 8?
Depends on: 561408, 560935
Keywords: checkin-needed
Comment on attachment 440452 [details] [diff] [review]
(Gv1-CC) Copy (the useful part of) m-c configure.in fix
[Supeseded by bug 562655]


No comment :-|
Attachment #440452 - Attachment description: (Gv1-CC) Copy (the useful part of) m-c configure.in fix → (Gv1-CC) Copy (the useful part of) m-c configure.in fix [Supeseded by bug 562655]
Attachment #440452 - Attachment is obsolete: true
Depends on: 562655
Depends on: 562820
blocking2.0: ? → -
Ftr, I just found
http://code.google.com/p/chromiumembedded/issues/detail?id=67
"Issue 67: Build cefclient with vc 2005 express edition"
You need to log in before you can comment on or make changes to this bug.