Closed Bug 488423 Opened 12 years ago Closed 11 years ago

JavaScript error: , line 0: uncaught exception: [Exception... Component returned failure code: 0x8007000e (NS_ERROR_OUT_OF_MEMORY) [nsIDOMJSWindow.openDialog]

Categories

(Core :: DOM: Core & HTML, defect)

PowerPC
AIX
defect
Not set
major

Tracking

()

RESOLVED FIXED
Tracking Status
status1.9.2 --- .2-fixed
status1.9.1 --- .9-fixed

People

(Reporter: shailen.n.jain, Assigned: shailen.n.jain)

References

Details

(Keywords: verified1.9.0.19, verified1.9.1, verified1.9.2)

Attachments

(4 files, 8 obsolete files)

1.12 KB, patch
jst
: review+
Details | Diff | Splinter Review
877 bytes, patch
jst
: review+
Details | Diff | Splinter Review
1.30 KB, patch
Details | Diff | Splinter Review
1.07 KB, patch
Details | Diff | Splinter Review
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.
OS: Other → AIX
Attached patch Root Cause : (obsolete) — Splinter Review
Attachment #372792 - Flags: superreview?
Attachment #372792 - Flags: review?
Attached patch Patch V 1 (obsolete) — Splinter Review
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.
Attachment #372792 - Attachment is obsolete: true
Attachment #372795 - Flags: superreview?(roc)
Attachment #372795 - Flags: review?(roc)
Attachment #372792 - Flags: superreview?
Attachment #372792 - Flags: review?
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?
Attached patch Patch V 2 (obsolete) — Splinter Review
Made the changes to the file nsJSEnvironment.cpp instead of nsGlobalWindow.cpp
Attachment #372795 - Attachment is obsolete: true
Attachment #372838 - Flags: superreview?(jst)
Attachment #372838 - Flags: review?(jst)
Attachment #372795 - Flags: superreview?(roc)
Attachment #372795 - Flags: review?(roc)
Can someone please review the patch ?
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!
Attachment #372838 - Flags: superreview?(jst)
Attachment #372838 - Flags: superreview+
Attachment #372838 - Flags: review?(jst)
Attachment #372838 - Flags: review+
Keywords: checkin-needed
Assignee: nobody → shailen.n.jain
Status: UNCONFIRMED → ASSIGNED
Component: General → DOM
Ever confirmed: true
Product: Firefox → Core
QA Contact: general → general
Target Milestone: --- → mozilla1.9.2a1
Version: unspecified → Trunk
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.
Please, also attach a hg patch, not a cvs one.
Keywords: checkin-needed
Whiteboard: [needs m-c landing]
Whiteboard: [needs m-c landing]
Attached patch Revised patchSplinter Review
Attachment #372838 - Attachment is obsolete: true
Attachment #418491 - Flags: superreview?(jst)
Attachment #418491 - Flags: review?(jst)
The revised patch created using hg
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.
Blocks: 537588
Attachment #418491 - Flags: approval1.9.2.1?
Attachment #418491 - Flags: approval1.9.1.8?
Attachment #418491 - Flags: approval1.9.2.1?
Attachment #418491 - Flags: approval1.9.1.8?
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.
(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.
Attachment #418491 - Flags: superreview?(jst)
Attachment #418491 - Flags: superreview+
Attachment #418491 - Flags: review?(jst)
Attachment #418491 - Flags: review+
Keywords: checkin-needed
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?
The suggested patch from #c16 looks nice, and is closer to the original code as it also handles argc == 0 case gracefully.
Hardware: Other → PowerPC
Target Milestone: mozilla1.9.2a1 → ---
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.
Attached patch Revised to comments 16 and 18 (obsolete) — Splinter Review
Revised patch for trunk
Attachment #429240 - Flags: review?
Attachment #429240 - Flags: review? → review?(jst)
same as trunk patch, only context adjustment
Attachment #429241 - Flags: review?
Attachment #429241 - Flags: review? → review?(jst)
Branch 1.9.1 has source file moved to different directory
Attachment #429242 - Flags: review?(jst)
Attached patch branch patch for 1.9.0 (obsolete) — Splinter Review
created with CVS
Attachment #429248 - Flags: review?(jst)
Attachment #429241 - Attachment is obsolete: true
Attachment #429250 - Flags: review?(jst)
Attachment #429241 - Flags: review?(jst)
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.
(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.
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.
-  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.
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.
Attachment #430731 - Flags: superreview+
Attachment #430731 - Flags: review+
Checked in, marking FIXED.

http://hg.mozilla.org/mozilla-central/rev/c05d0ac56307
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
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.
Attachment #429240 - Flags: review?(jst) → review-
Attachment #429242 - Flags: review?(jst) → review-
Attachment #429248 - Flags: review?(jst) → review-
Attachment #429250 - Flags: review?(jst) → review-
Comment on attachment 430731 [details] [diff] [review]
Shailen's patch with a fix.

Fine to have this fixed :-)
Requesting approval for branches.
Attachment #430731 - Flags: approval1.9.2.2?
Attachment #430731 - Flags: approval1.9.1.9?
Attachment #430731 - Flags: approval1.9.0.19?
Attachment #429250 - Attachment is obsolete: true
Attachment #429248 - Attachment is obsolete: true
Attachment #429242 - Attachment is obsolete: true
Attachment #429240 - Attachment is obsolete: true
unit moved to different src dir and context adjusted. for CVS
applied trunk patch in moved src dir with patch -p3 and recreated with hg for context adjustmend. 1.9.1 branch
Attachment #430731 - Flags: approval1.9.1.9?
Attachment #430731 - Flags: approval1.9.0.19?
Attachment #430815 - Flags: approval1.9.0.19?
Attachment #430816 - Flags: approval1.9.1.9?
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.
Attachment #430816 - Flags: approval1.9.1.9? → approval1.9.1.9+
Attachment #430815 - Flags: approval1.9.0.19? → approval1.9.0.19+
Attachment #430731 - Flags: approval1.9.2.2? → approval1.9.2.2+
Keywords: checkin-needed
Whiteboard: [checkin to cvs] [checkin to mozilla-1.9.1] [checkin to mozilla-1.9.2]
Keywords: checkin-needed
Whiteboard: [checkin to cvs] [checkin to mozilla-1.9.1] [checkin to mozilla-1.9.2]
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.