Last Comment Bug 488423 - JavaScript error: , line 0: uncaught exception: [Exception... Component returned failure code: 0x8007000e (NS_ERROR_OUT_OF_MEMORY) [nsIDOMJSWindow.openDialog]
: JavaScript error: , line 0: uncaught exception: [Exception... Component retur...
Status: RESOLVED FIXED
: verified1.9.0.19, verified1.9.1, verified1.9.2
Product: Core
Classification: Components
Component: DOM (show other bugs)
: Trunk
: PowerPC AIX
: -- major (vote)
: ---
Assigned To: Shailen
:
Mentors:
Depends on:
Blocks: 537588
  Show dependency treegraph
 
Reported: 2009-04-14 21:29 PDT by Shailen
Modified: 2010-04-25 07:29 PDT (History)
6 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
.2-fixed
.9-fixed


Attachments
Root Cause : (1.10 KB, patch)
2009-04-14 21:37 PDT, Shailen
no flags Details | Diff | Splinter Review
Patch V 1 (1.10 KB, patch)
2009-04-14 21:54 PDT, Shailen
no flags Details | Diff | Splinter Review
Patch V 2 (1.26 KB, patch)
2009-04-15 03:06 PDT, Shailen
jst: review+
jst: superreview+
Details | Diff | Splinter Review
Revised patch (1.12 KB, patch)
2009-12-19 02:26 PST, Shailen
jst: review+
jst: superreview+
Details | Diff | Splinter Review
Revised to comments 16 and 18 (848 bytes, patch)
2010-02-26 14:43 PST, Uli Link (:ul-mcamafia)
jst: review-
Details | Diff | Splinter Review
Same patch, context adjusted for branch 1.9.2 (2.70 KB, patch)
2010-02-26 14:45 PST, Uli Link (:ul-mcamafia)
no flags Details | Diff | Splinter Review
Same patch, moved file for branch 1.9.1 (864 bytes, patch)
2010-02-26 14:47 PST, Uli Link (:ul-mcamafia)
jst: review-
Details | Diff | Splinter Review
branch patch for 1.9.0 (1.08 KB, patch)
2010-02-26 15:03 PST, Uli Link (:ul-mcamafia)
jst: review-
Details | Diff | Splinter Review
branch patch for 1.9.2, context adjusted only to trunk patch (848 bytes, patch)
2010-02-26 15:07 PST, Uli Link (:ul-mcamafia)
jst: review-
Details | Diff | Splinter Review
Shailen's patch with a fix. (877 bytes, patch)
2010-03-05 14:18 PST, Johnny Stenback (:jst, jst@mozilla.com)
jst: review+
jst: superreview+
mbeltzner: approval1.9.2.2+
Details | Diff | Splinter Review
fixed patch adjusted for branch 1.9.0 (1.30 KB, patch)
2010-03-06 02:26 PST, Uli Link (:ul-mcamafia)
mbeltzner: approval1.9.0.19+
Details | Diff | Splinter Review
fixed patch adjusted for branch 1.9.1 (1.07 KB, patch)
2010-03-06 02:29 PST, Uli Link (:ul-mcamafia)
mbeltzner: approval1.9.1.9+
Details | Diff | Splinter Review

Description Shailen 2009-04-14 21:29:25 PDT
User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.0.8) Gecko/2009032609 Firefox/3.0.8
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.0.8) Gecko/2009032609 Firefox/3.0.8

Launch the browser Firefox on AIX, and click on Help->About Mozilla Firefox, it gives the following error

'Uncaugth exception : "Component returned failur code : 0x8007000e  (NS_ERROR_OUT_OF_MEMORY)
(nsIDOMJSWINDOW.openDialog)" nsresult: 0x8007000e  (NS_ERROR_OUT_OF_MEMORY) location "JS frame ::
chrome://browser/content/utilityOverlay.js :: openAboutDialog :: line 340" data: no]

Reproducible: Always

Steps to Reproduce:
1. Build Mozilla Firefox Trunk build on AIX with Gnome RPMS of 64 bit
version.
2. Launch the browser 
3. Click on Help->About Mozilla Firefox
4. The Dialog box does not comeup.

Actual Results:  
1. Click on Tools->Java Console
2. The error is displayed as stated above.

Expected Results:  
Should display the dialog box without giving any error.
Comment 1 Shailen 2009-04-14 21:37:07 PDT
Created attachment 372792 [details] [diff] [review]
Root Cause :
Comment 2 Shailen 2009-04-14 21:54:48 PDT
Created attachment 372795 [details] [diff] [review]
Patch V 1

Root Cause : The second parameter(argc-argOffset) of the function NS_CreateJSArgv evaluates to 0 (zero) which inturn results in calling the function PR_CALLOC with 0 (zero) as the parameter. 

As per the path, the above condition does not occur.
Comment 3 Robert O'Callahan (:roc) (email my personal email if necessary) 2009-04-14 22:16:43 PDT
I don't think this patch is correct. I think the bug is here:
http://mxr.mozilla.org/mozilla-central/source/dom/base/nsJSEnvironment.cpp#4000
It should return NS_OK if argc is 0. I'm not the right reviewer for this code though, try jst?
Comment 4 Shailen 2009-04-15 03:06:33 PDT
Created attachment 372838 [details] [diff] [review]
Patch V 2

Made the changes to the file nsJSEnvironment.cpp instead of nsGlobalWindow.cpp
Comment 5 Shailen 2009-04-27 22:25:29 PDT
Can someone please review the patch ?
Comment 6 Robert O'Callahan (:roc) (email my personal email if necessary) 2009-04-27 23:51:00 PDT
jst?
Comment 7 Johnny Stenback (:jst, jst@mozilla.com) 2009-04-28 16:18:04 PDT
Comment on attachment 372838 [details] [diff] [review]
Patch V 2

-  mArgv = (jsval *) PR_CALLOC(argc * sizeof(jsval));
-  if (!mArgv) {
-    *prv = NS_ERROR_OUT_OF_MEMORY;
-    return;
+  if (argc > 0) {
+    mArgv = (jsval *) PR_CALLOC(argc * sizeof(jsval));
+    if (!mArgv) {
+      *prv = NS_ERROR_OUT_OF_MEMORY;
+      return;
+    }
   }
-

Looks good, but I'd leave the empty space in place here.

r+sr=jst, thanks for the patch!
Comment 8 Phil Ringnalda (:philor) 2009-05-03 23:40:26 PDT
I pushed this to the try server, and both Linux and Windows unit tests claimed that they crashed in mochitest-browser-chrome. Maybe coincidence, but I wouldn't risk pushing it without getting a clean try server run first.
Comment 9 Serge Gautherie (:sgautherie) 2009-05-15 16:07:13 PDT
Please, also attach a hg patch, not a cvs one.
Comment 10 Shailen 2009-12-19 02:26:30 PST
Created attachment 418491 [details] [diff] [review]
Revised patch
Comment 11 Shailen 2009-12-19 02:27:14 PST
The revised patch created using hg
Comment 12 Uli Link (:ul-mcamafia) 2010-01-03 14:11:45 PST
Shailen: can you request approval for 1.9.1 branch.

I have tested this patch with Firefox 3.0.17, 3.5.7, Seamonkey 2.0.1 and Thunderbird 3.0. They all need this patch.
Seamonkey cannot start the MailNews Client without this patch.
Comment 13 Daniel Veditz [:dveditz] 2010-01-08 13:49:36 PST
Comment on attachment 418491 [details] [diff] [review]
Revised patch

Please don't ask for branch approval until we've got a fully-reviewed and tested patch.
Comment 14 Uli Link (:ul-mcamafia) 2010-01-09 01:44:22 PST
(In reply to comment #13)
> (From update of attachment 418491 [details] [diff] [review])
> Please don't ask for branch approval until we've got a fully-reviewed and
> tested patch.

https://bugzilla.mozilla.org/attachment.cgi?id=372838 had both r + sr.
As the only difference the newer patch was created using hg.
Since May no further reaction to https://bugzilla.mozilla.org/show_bug.cgi?id=488423#c8 appeared. Maybe shailen better not checked the obsolete button for the new patch, but then there maybe a confusion which patch to use when it comes to checkin.
Comment 16 The Written Word 2010-02-26 12:50:48 PST
We used this patch to work around the same issue (seamonkey-2.0.3 on AIX, trying to open Mail & News or chatzilla from the menu does nothing, and in a debug build prints the NS_ERROR_OUT_OF_MEMORY error).

Index: mozilla/dom/src/base/nsJSEnvironment.cpp
===================================================================
--- mozilla/dom/src/base/nsJSEnvironment.cpp.orig       2010-02-06 01:03:55.000000000 +0000
+++ mozilla/dom/src/base/nsJSEnvironment.cpp    2010-02-26 20:34:57.625303594 +0000
@@ -4104,7 +4104,7 @@
 {
   // copy the array - we don't know its lifetime, and ours is tied to xpcom
   // refcounting.  Alloc zero'd array so cleanup etc is safe.
-  mArgv = (jsval *) PR_CALLOC(argc * sizeof(jsval));
+  mArgv = (jsval *) PR_CALLOC(argc ? argc * sizeof(jsval): sizeof(jsval));
   if (!mArgv) {
     *prv = NS_ERROR_OUT_OF_MEMORY;
     return;


PR_CALLOC(0) seems to be expected to give a non-zero result, however, a quick test on AIX 6.1 using calloc(0,4) shows that it returns 0, calloc returning 0 is allowed - http://www.opengroup.org/onlinepubs/9699919799/functions/calloc.html

Should the implementation of PR_Calloc instead be fixed to never return NULL?
Comment 17 Uli Link (:ul-mcamafia) 2010-02-26 14:00:17 PST
The suggested patch from #c16 looks nice, and is closer to the original code as it also handles argc == 0 case gracefully.
Comment 18 Wan-Teh Chang 2010-02-26 14:07:49 PST
malloc/calloc/realloc all have this issue when their 'size' argument is 0.
The return value in that case is implementation-dependent.

It's best to avoid calling these functions with a 'size' of 0, as the
patch does.
Comment 19 Uli Link (:ul-mcamafia) 2010-02-26 14:43:30 PST
Created attachment 429240 [details] [diff] [review]
Revised to comments 16 and 18

Revised patch for trunk
Comment 20 Uli Link (:ul-mcamafia) 2010-02-26 14:45:40 PST
Created attachment 429241 [details] [diff] [review]
Same patch, context adjusted for branch 1.9.2

same as trunk patch, only context adjustment
Comment 21 Uli Link (:ul-mcamafia) 2010-02-26 14:47:48 PST
Created attachment 429242 [details] [diff] [review]
Same patch, moved file for branch 1.9.1

Branch 1.9.1 has source file moved to different directory
Comment 22 Uli Link (:ul-mcamafia) 2010-02-26 15:03:00 PST
Created attachment 429248 [details] [diff] [review]
branch patch for 1.9.0

created with CVS
Comment 23 Uli Link (:ul-mcamafia) 2010-02-26 15:07:24 PST
Created attachment 429250 [details] [diff] [review]
branch patch for 1.9.2, context adjusted only to trunk patch
Comment 24 Johnny Stenback (:jst, jst@mozilla.com) 2010-02-26 15:50:26 PST
I just pushed the initial patch to the try server since I'd like to understand
why it's not good enough. Allocating a sizeof(jsval) that'll never be used
seems unnecessary to me, so I'd prefer to understand why the original patch
didn't stick.
Comment 25 The Written Word 2010-02-26 16:14:55 PST
(In reply to comment #24)
> I just pushed the initial patch to the try server since I'd like to understand
> why it's not good enough.

Ok. We figured a NULL mArgv was not right when we saw:

nsresult
nsJSArgArray::GetArgs(PRUint32 *argc, void **argv)
{
  if (!mArgv) {
    NS_WARNING("nsJSArgArray has no argv!");
    return NS_ERROR_UNEXPECTED;
  }

in the same file.
Comment 26 Phil Ringnalda (:philor) 2010-02-26 18:41:36 PST
Conveniently enough, I pushed it about five minutes after you, and we both agree: failed all four flavors of mochitest, on both Windows and Linux.
Comment 27 Uli Link (:ul-mcamafia) 2010-02-27 01:13:42 PST
-  mArgv = (jsval *) PR_CALLOC(argc * sizeof(jsval));
-  if (!mArgv) {
-    *prv = NS_ERROR_OUT_OF_MEMORY;
-    return;
+  if (argc = 0) {
+     *prv = NS_OK;
+     return;
+  }
+  if (argc > 0) {
+    mArgv = (jsval *) PR_CALLOC(argc * sizeof(jsval));
+    if (!mArgv) {
+      *prv = NS_ERROR_OUT_OF_MEMORY;
+      return;
+    }
   }

So it will return for the argc = 0 case and also not call calloc with 0 which will return a NULL pointer on AIX.
Only this case is different for the original patch compared to the repository.
Slightly different to the TWW proposal from comment #16

Cross checked with aCC on HP-UX: same behaviour as Linux or Windows.
So if this also fails the try server, I suggest a #ifdef __IBMCPP__'ed workaround as a last resort.
Comment 28 Johnny Stenback (:jst, jst@mozilla.com) 2010-03-05 14:18:12 PST
Created attachment 430731 [details] [diff] [review]
Shailen's patch with a fix.

This is Shailen's fix with the additional change to not set mArgv to argv so that it remains null when calling this code with 0 passed as argc. W/o that additional change any such calls end up freeing the passed in argv when the nsJSArgArray gets destroyed.
Comment 29 Johnny Stenback (:jst, jst@mozilla.com) 2010-03-05 17:06:18 PST
Checked in, marking FIXED.

http://hg.mozilla.org/mozilla-central/rev/c05d0ac56307
Comment 30 Johnny Stenback (:jst, jst@mozilla.com) 2010-03-05 17:06:58 PST
Comment on attachment 429240 [details] [diff] [review]
Revised to comments 16 and 18

If we need to do anything for this for the branches, we should do the same as what landed on the trunk, not this approach.
Comment 31 Uli Link (:ul-mcamafia) 2010-03-06 01:52:01 PST
Comment on attachment 430731 [details] [diff] [review]
Shailen's patch with a fix.

Fine to have this fixed :-)
Requesting approval for branches.
Comment 32 Uli Link (:ul-mcamafia) 2010-03-06 02:26:11 PST
Created attachment 430815 [details] [diff] [review]
fixed patch adjusted for branch 1.9.0

unit moved to different src dir and context adjusted. for CVS
Comment 33 Uli Link (:ul-mcamafia) 2010-03-06 02:29:41 PST
Created attachment 430816 [details] [diff] [review]
fixed patch adjusted for branch 1.9.1

applied trunk patch in moved src dir with patch -p3 and recreated with hg for context adjustmend. 1.9.1 branch
Comment 34 Mike Beltzner [:beltzner, not reading bugmail] 2010-03-08 10:25:45 PST
Comment on attachment 430816 [details] [diff] [review]
fixed patch adjusted for branch 1.9.1

a=beltzner for branches, please get someone to check these in ASAP.
Comment 35 Johnny Stenback (:jst, jst@mozilla.com) 2010-03-08 12:27:35 PST
Fixed on branches and in CVS.

http://hg.mozilla.org/releases/mozilla-1.9.2/rev/44ff90620a84
http://hg.mozilla.org/releases/mozilla-1.9.1/rev/76d7fe0b8680

Note You need to log in before you can comment on or make changes to this bug.