536343 - [OS/2] Build break in nsSigHandlers.cpp

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Rich Walsh]

Attachment: fixes nsAppRunner & nsSigHandlers

Bug 533035 kills the OS/2 build with "#error No signal handling implementation for this platform".  As it happens, OS/2 has had an f/p exception handler in XRE_main() for over 5 years now, so the #error is inappropriate.  The attached patch fixes the problem.

Comment 1

[Andreas Gal :gal]

Comment on attachment 418804 [details] [diff] [review]
fixes nsAppRunner & nsSigHandlers

The !define(XP_OS2) is kinda brittle. Maybe #elif defined(XP_OS2)
/* Nothing. */
#else
#error No signal ...
#endif

Both are kinda ugly. Up to you.

Comment 2

[Rich Walsh]

Attachment: fix nsAppRunner & nsSigHandlers - v2

(In reply to comment #1)
> The !define(XP_OS2) is kinda brittle. Maybe #elif defined(XP_OS2)
> /* Nothing. */
> #else
> #error No signal ...
> #endif
>
> Both are kinda ugly. Up to you.

I didn't like that either.  I changed it to:

+#elif defined(XP_OS2)
+/* OS/2's FPE handler is implemented in NSPR */
+
 #else
 #error No signal handling implementation for this platform.

If this is OK, could you check it in?

Comment 3

[Andreas Gal :gal]

Comment on attachment 420269 [details] [diff] [review]
fix nsAppRunner & nsSigHandlers - v2

Just out of curiosity, how come OS/2 implements the FPE handler in NSPR and I wonder whether all of this code could wander into NSPR.

Comment 4

[Walter Meinl]

Attachment: patch for the trunk, doesn't crash at startup with SIGFPE

(In reply to comment #2)
> Created an attachment (id=420269) [details]
> fix nsAppRunner & nsSigHandlers - v2

> If this is OK, could you check it in?
Rich, while your patch works fine on the 1.9.2 branch it won't apply cleanly on trunk anymore.
The following patches were pushed before Christmas to trunk only
first:
http://hg.mozilla.org/mozilla-central/rev/c63aea2f8c16
second:
http://hg.mozilla.org/mozilla-central/rev/6e9b8529e66b
since then, even when I made your first patch to apply, Minefield would crash immediately at start-up with a SIGFPE (when started with /console).
I can work around the crash when I leave the 'ScopedFPHandler handler' line where it was until now and exclude OS/2 from calling InstallSignalHandlers(progname);.
Dunno, however if that's right, but with it Minefield is stable again.

Comment 5

[Rich Walsh]

Comment on attachment 420876 [details] [diff] [review]
patch for the trunk, doesn't crash at startup with SIGFPE

I don't know if I'm really the one who should be approving this, but your patch WFM.

BTW... shouldn't the original patch be marked as obsolete?

Comment 6

[Andreas Gal :gal]

You can ask bsmedberg for approval. He is cc'ed.

Comment 7

[Walter Meinl]

(In reply to comment #5)
> (From update of attachment 420876 [details] [diff] [review])
> I don't know if I'm really the one who should be approving this, but your patch
> WFM.
Why not, who except OS/2 users could test it? I mean, while your patch compiled, the problem comes when starting the application. Since we have no automated testing environment, how shall we treat such problems?
> 
> BTW... shouldn't the original patch be marked as obsolete?
I didn't obsolete it, because it applies and works on the branch, since I wasn't really sure, that I really do the right thing, I didn't want to mark it obsolete as it already was r+.
BTW, do you have an idea, why the movements of code in nsAppRunner after creation of your patch, would cause us to crash. Maybe because the invocation of our FP handler comes too late with the new code?

Comment 8

[Walter Meinl]

Comment on attachment 420876 [details] [diff] [review]
patch for the trunk, doesn't crash at startup with SIGFPE

per suggestion of gal

Comment 9

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Clearing checkin-needed due to outstanding review request.  Please add it back if I've done so in error.

Comment 10

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

http://hg.mozilla.org/mozilla-central/rev/f17cb708eab0

Comment 11

[Walter Meinl]

Attachment: same patch for 1.9.2 just a context change

practically zero risk for main platforms #ifdef'd for OS/2

Comment 12

[Walter Meinl]

Comment on attachment 434687 [details] [diff] [review]
same patch for 1.9.2 just a context change

Due to the Lorentz merge 1.9.2-branch needs the same patch now as formerly the trunk (aka 1.9.3) clearing approval request for this patch

Comment 13

[LegNeato]

Comment on attachment 420876 [details] [diff] [review]
patch for the trunk, doesn't crash at startup with SIGFPE

We will not be taking this for 3.6.6. Moving approval request forward.

Comment 14

[LegNeato]

Comment on attachment 420876 [details] [diff] [review]
patch for the trunk, doesn't crash at startup with SIGFPE

a=LegNeato for 1.9.2.9.

Comment 15

[Daniel Veditz [:dveditz]]

http://hg.mozilla.org/releases/mozilla-1.9.2/rev/0b29d59c03ea