Closed Bug 358975 Opened 14 years ago Closed 14 years ago

Silent failure on Out-of-Memory in Function constructor

Categories

(Core :: JavaScript Engine, defect)

defect
Not set
normal

Tracking

()

VERIFIED FIXED

People

(Reporter: igor, Assigned: igor)

References

Details

(Keywords: verified1.8.0.9, verified1.8.1.1)

Attachments

(2 files)

Function implementation in Function() from jsfun.c fails silently without Out-Of-Memory reporting after failed allocation of space for arguments from the arena. Thus the following test case quits the shell silently with 0 exit status:

var fe="vv";

for (i=0; i<24; i++)
  fe += fe;

var fu=new Function(
  fe, fe, fe, fe, fe, fe, fe, fe, fe, fe, fe, fe, fe, fe, fe, fe, fe,
  fe, fe, fe, fe, fe, fe, fe, fe, fe, fe, fe, fe, fe, fe, fe, fe, fe,
  fe, fe, fe, fe, fe, fe, fe, fe, fe, fe, fe, fe, fe, fe, fe, fe, fe,
  "done"
);

The test case is based on an example provided by Georgi Guninski.
Attached patch Fix v1Splinter Review
Attachment #244269 - Flags: review?(brendan)
Comment on attachment 244269 [details] [diff] [review]
Fix v1

No-brainer for js1.7src and 1.8.1.1.

/be
Attachment #244269 - Flags: review?(brendan)
Attachment #244269 - Flags: review+
Attachment #244269 - Flags: approval1.8.1.1?
I committed the patch from comment 1 to the trunk:

Checking in jsfun.c;
/cvsroot/mozilla/js/src/jsfun.c,v  <--  jsfun.c
new revision: 3.171; previous revision: 3.170
done
Status: NEW → RESOLVED
Closed: 14 years ago
Resolution: --- → FIXED
Comment on attachment 244269 [details] [diff] [review]
Fix v1

The patch applies to 1.8.0 branch as is.
Attachment #244269 - Flags: approval1.8.0.9?
Flags: blocking1.8.1.1?
Flags: blocking1.8.0.9?
Blocks: js1.7src
Flags: in-testsuite+
shell now reports out of memory 1.9 20061103 windows/linux
Status: RESOLVED → VERIFIED
fwiw, I was hoping this would eliminate cases where my log munging would not be able to detect the status of a test, but this has resulted in out of memory errors being reported to the error console but no longer being reported to stderr in debug builds or being able to be captured from the error console and dumped to stdout in opt builds by Spider. Net loss for me. :-(
(In reply to comment #6)
> fwiw, I was hoping this would eliminate cases where my log munging would not be
> able to detect the status of a test, but this has resulted in out of memory
> errors being reported to the error console but no longer being reported to
> stderr in debug builds or being able to be captured from the error console and
> dumped to stdout in opt builds by Spider. Net loss for me. :-(
> 

I think a good way to solve this would be to mark the tests where quiting with out-of-memory is OK so at least jsDriver can check for that.
(In reply to comment #7)

> 
> I think a good way to solve this would be to mark the tests where quiting with
> out-of-memory is OK so at least jsDriver can check for that.
> 

How can I distinguish an out of memory error that causes the script to stop running and one where a bug causes the script to stop running without an error message?
(In reply to comment #8)
> (In reply to comment #7)
> 
> > 
> > I think a good way to solve this would be to mark the tests where quiting with
> > out-of-memory is OK so at least jsDriver can check for that.
> > 
> 
> How can I distinguish an out of memory error that causes the script to stop
> running and one where a bug causes the script to stop running without an error
> message?

Different exit status?  Easy enough to hack into js.c's error reporter.

/be

With the patch shell reports 5 as exit code on out-of memory:

@callisto~/m/trunk/mozilla/js/src> cat ~/m/files/y.js 
var fe="vv";

for (i=0; i<24; i++)
  fe += fe;

new Function(
  fe, fe, fe, fe, fe, fe, fe, fe, fe, fe, fe, fe, fe, fe, fe, fe, fe,
  fe, fe, fe, fe, fe, fe, fe, fe, fe, fe, fe, fe, fe, fe, fe, fe, fe,
  fe, fe, fe, fe, fe, fe, fe, fe, fe, fe, fe, fe, fe, fe, fe, fe, fe,
  "done"
);

print(10);
@callisto~/m/trunk/mozilla/js/src> ./Linux_All_DBG.OBJ/js ~/m/files/y.js
/home/igor/m/files/y.js:10: out of memory
@callisto~/m/trunk/mozilla/js/src> echo $?
5
Attachment #244625 - Flags: review?(brendan)
My problem isn't with the shell, but with the browser. If you want to change the exit code for the shell that is fine with me, but it doesn't help my problem.
Comment on attachment 244625 [details] [diff] [review]
Separated exit code for the out-of-memory error

I like this patch for the shell.  For the browser, the problem may be that you have an uncatchable error, not thrown as an exception. However, is the window.onerror handler run for OOM? Can it succeed? Try hooking it up to a function that doesn't do any allocations.

/be
Attachment #244625 - Flags: review?(brendan) → review+
I committed the shell-only patch from comment 10 to the trunk:

Checking in js.c;
/cvsroot/mozilla/js/src/js.c,v  <--  js.c
new revision: 3.127; previous revision: 3.126
done

I suggest for consistency to apply the patch to 1.8.1 and 1.8.0 branches as well.
(In reply to comment #13)
> I suggest for consistency to apply the patch to 1.8.1 and 1.8.0 branches as
> well.

Sure -- may have to sync js.c versions to trunk, not just land this patch.  You should be able to check in as NPOB (Not Part Of Build).

/be
(In reply to comment #6)
> but this has resulted in out of memory
> errors being reported to the error console but no longer being reported to
> stderr in debug builds or being able to be captured from the error console and
> dumped to stdout in opt builds by Spider.

So out-of-memory and only out-of-memory errors are not reported to stderr in the debug builds?

I can patch http://lxr.mozilla.org/seamonkey/source/dom/src/base/nsJSEnvironment.cpp#248 to always print them to stderr when debugging. 

(In reply to comment #14)
> (In reply to comment #13)
> > I suggest for consistency to apply the patch to 1.8.1 and 1.8.0 branches as
> > well.
> 
> Sure -- may have to sync js.c versions to trunk, not just land this patch.  

That would not work as the shell source on the trunk depends on the content of headers on the trunk.
(In reply to comment #16)
> (In reply to comment #14)
> > (In reply to comment #13)
> > > I suggest for consistency to apply the patch to 1.8.1 and 1.8.0 branches as
> > > well.
> > 
> > Sure -- may have to sync js.c versions to trunk, not just land this patch.  
> 
> That would not work as the shell source on the trunk depends on the content of
> headers on the trunk.

Oh, sorry (I should have diffed).  Just considering the 1.8 branch, is the delta due to stuff slated for js1.7src/1.8.l.1, but not brought over yet?

/be
I committed the shell-only patch from comment 10 to MOZILLA_1_8_BRANCH:

Checking in js.c;
/cvsroot/mozilla/js/src/js.c,v  <--  js.c
new revision: 3.93.2.9; previous revision: 3.93.2.8
done

(In reply to comment #17)
> Just considering the 1.8 branch, is the
> delta due to stuff slated for js1.7src/1.8.l.1, but not brought over yet?

Yes.
I committed the shell-only patch from comment 10 to MOZILLA_1_8_0_BRANCH:

Checking in js.c;
/cvsroot/mozilla/js/src/js.c,v  <--  js.c
new revision: 3.93.2.2.2.2; previous revision: 3.93.2.2.2.1
done

Comment on attachment 244269 [details] [diff] [review]
Fix v1

approved for 1.8/1.8.0 branch, a=dveditz for drivers
Attachment #244269 - Flags: approval1.8.1.1?
Attachment #244269 - Flags: approval1.8.1.1+
Attachment #244269 - Flags: approval1.8.0.9?
Attachment #244269 - Flags: approval1.8.0.9+
Flags: blocking1.8.1.1?
Flags: blocking1.8.1.1+
Flags: blocking1.8.0.9?
Flags: blocking1.8.0.9+
I committed the patch from comment 1 to MOZILLA_1_8_BRANCH:

Checking in jsfun.c;
/cvsroot/mozilla/js/src/jsfun.c,v  <--  jsfun.c
new revision: 3.117.2.25; previous revision: 3.117.2.24
done
Keywords: fixed1.8.1.1
I committed the patch from comment 1 to MOZILLA_1_8_0_BRANCH:

Checking in jsfun.c;
/cvsroot/mozilla/js/src/jsfun.c,v  <--  jsfun.c
new revision: 3.117.2.7.2.12; previous revision: 3.117.2.7.2.11
done
Keywords: fixed1.8.0.9
The following tests needed to be modified to match the new expected exit code in the shell:
Checking in e4x/XML/regress-324422-1.js;
/cvsroot/mozilla/js/tests/e4x/XML/regress-324422-1.js,v  <--  regress-324422-1.js
new revision: 1.4; previous revision: 1.3
done
Checking in js1_5/Array/regress-157652.js;
/cvsroot/mozilla/js/tests/js1_5/Array/regress-157652.js,v  <--  regress-157652.js
new revision: 1.16; previous revision: 1.15
done
Checking in js1_5/Regress/regress-303213.js;
/cvsroot/mozilla/js/tests/js1_5/Regress/regress-303213.js,v  <--  regress-303213.js
new revision: 1.3; previous revision: 1.2
done
Checking in js1_5/Regress/regress-329530.js;
/cvsroot/mozilla/js/tests/js1_5/Regress/regress-329530.js,v  <--  regress-329530.js
new revision: 1.3; previous revision: 1.2
done
Checking in js1_5/Regress/regress-330352.js;
/cvsroot/mozilla/js/tests/js1_5/Regress/regress-330352.js,v  <--  regress-330352.js
new revision: 1.3; previous revision: 1.2
done
Checking in js1_5/Regress/regress-3649-n.js;
/cvsroot/mozilla/js/tests/js1_5/Regress/regress-3649-n.js,v  <--  regress-3649-n.js
new revision: 1.3; previous revision: 1.2
done
verified fixed 1.8.0.9, 1.8.1.1 20061111 windows/linux in browser by checking js console to see out of memory error and in shell to see the exit code 5.
You need to log in before you can comment on or make changes to this bug.