Silent failure on Out-of-Memory in Function constructor

VERIFIED FIXED

Status

()

defect
VERIFIED FIXED
13 years ago
13 years ago

People

(Reporter: igor, Assigned: igor)

Tracking

({verified1.8.0.9, verified1.8.1.1})

Trunk
Points:
---
Dependency tree / graph
Bug Flags:
blocking1.8.1.1 +
blocking1.8.0.9 +
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments)

Assignee

Description

13 years ago
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.
Assignee

Comment 1

13 years ago
Posted 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?
Assignee

Comment 3

13 years ago
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: 13 years ago
Resolution: --- → FIXED
Assignee

Comment 4

13 years ago
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?
Assignee

Updated

13 years ago
Flags: blocking1.8.1.1?
Flags: blocking1.8.0.9?
Assignee

Updated

13 years ago
Blocks: js1.7src

Updated

13 years ago
Flags: in-testsuite+

Comment 5

13 years ago
shell now reports out of memory 1.9 20061103 windows/linux
Status: RESOLVED → VERIFIED

Comment 6

13 years ago
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. :-(
Assignee

Comment 7

13 years ago
(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

13 years ago
(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

Assignee

Comment 10

13 years ago
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)

Comment 11

13 years ago
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+
Assignee

Comment 13

13 years ago
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
Assignee

Comment 15

13 years ago
(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. 

Assignee

Comment 16

13 years ago
(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
Assignee

Comment 18

13 years ago
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

Assignee

Comment 19

13 years ago
(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.
Assignee

Comment 20

13 years ago
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+
Assignee

Comment 22

13 years ago
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
Assignee

Comment 23

13 years ago
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

Comment 24

13 years ago
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

13 years ago
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.