358975 - Silent failure on Out-of-Memory in Function constructor

Status: VERIFIED FIXED

(Core :: JavaScript Engine, defect)

Description

[Igor Bukanov]

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.

Comment 1

[Igor Bukanov]

Attachment: Fix v1

Comment 2

[Brendan Eich [:brendan]]

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

No-brainer for js1.7src and 1.8.1.1.

/be

Comment 3

[Igor Bukanov]

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

Comment 4

[Igor Bukanov]

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

The patch applies to 1.8.0 branch as is.

Comment 5

[Bob Clary [:bc] (inactive)]

shell now reports out of memory 1.9 20061103 windows/linux

Comment 6

[Bob Clary [:bc] (inactive)]

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. :-(

Comment 7

[Igor Bukanov]

(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.

Comment 8

[Bob Clary [:bc] (inactive)]

(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?

Comment 9

[Brendan Eich [:brendan]]

(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

Comment 10

[Igor Bukanov]

Attachment: Separated exit code for the out-of-memory error

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

Comment 11

[Bob Clary [:bc] (inactive)]

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 12

[Brendan Eich [:brendan]]

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

Comment 13

[Igor Bukanov]

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.

Comment 14

[Brendan Eich [:brendan]]

(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

Comment 15

[Igor Bukanov]

(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. 

Comment 16

[Igor Bukanov]

(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.

Comment 17

[Brendan Eich [:brendan]]

(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

Comment 18

[Igor Bukanov]

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

Comment 19

[Igor Bukanov]

(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.

Comment 20

[Igor Bukanov]

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 21

[Daniel Veditz [:dveditz]]

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

approved for 1.8/1.8.0 branch, a=dveditz for drivers

Comment 22

[Igor Bukanov]

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

Comment 23

[Igor Bukanov]

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

Comment 24

[Bob Clary [:bc] (inactive)]

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

Comment 25

[Bob Clary [:bc] (inactive)]

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.