(Technically I suppose it would work if I had a board with more memory, but my board has 512MB and that is still fairly large for an ARM platform.) I've created this bug from a comment I made in bug 507425, as I see this as a separate issue. My original comment was as follows: ---- If this is the only way to achieve the test condition then I'm not sure what can be done to resolve this, but note that testBug507425 fails on ARM* with an "out of memory" error with i == 27. * By "on ARM" I mean on a Cortex-A8 development board with 512MB of RAM. I'm running Ubuntu 9.04, and I'm using a debug build of Trace Monkey. This wouldn't be an issue, except that it halts the test run, and so I can't run trace-test without hacking this test out. This is the tail end of the output I get: TEST-PASS | trace-test.js | testOwnPropertyWithInOperator TEST-PASS | trace-test.js | testBug501690 TEST-PASS | trace-test.js | testObjectVsPrototype TEST-PASS | trace-test.js | testEliminatedGuardWithinAnchor TEST-PASS | trace-test.js | testNativeSetter trace-test.js:5620: out of memory ---- I'm not sure how to proceed here. The discussion in the bug implies that this is the only way to trigger the test condition, but if it kills the VM then it also kills the test run, masking out any potential failures after the test. In addition, I would argue that no JavaScript file should be able to kill the VM in this way, but I don't know how it is integrated into the browser and it's likely that it runs in a kind of sandbox to stop the browser crashing when it hits such a script. ---- Jason, yet again I am copying you on this bug as you filed the patch for bug 507425.

If your 32-bit system can allocate 256MiB and live to tell the tale, you get an exception (InternalError: allocation size overflow) because 256Mi > JSString::MAX_LENGTH. If malloc says no, you get the "out of memory" error instead. It is not a catchable exception. JS stops executing that script. In the case of the JS shell, of course, there's only one script, so it just exits. But in the case of Firefox/Fennec, we would not kill the browser.

Ok, so it's no security concern. (I didn't think it would be.) Still, have you any idea how I can tidy this up? One one hand, I want to run the full test suite on ARM so I don't want to mask out the test for small platforms*, but on the other hand I don't know how else to trigger the test condition. * I'm not even sure if this is feasible; getting an accurate "IsSmallPlatform" test will be tricky, to say the least! Currently, the only thing I can think of is to remove the test from the ARM platform, but that's a really ugly solution.

I suggest not running the test as part of make check. We already do this for the Mandelbrot tests because mobile platforms are unable to cope. Cc-ing dmandelin in case he can fix this trivially in bug 505588 (by putting this one test in the 'slow' directory; maybe it's already there). I am not hopeful about fixing the test. There is one other possible way to trigger that condition, via js_Enumerate -> js_AddAsGCBytes -> js_CanLeaveTrace. So you'd have to somehow reach js_Enumerate from a function that can be called directly from trace without cx->bailExit being set. I don't see any way that can happen offhand.

Another way out is to make "out of memory" an acceptable outcome for these tests. To do that I would suggest waiting for bug 505588 to land, then working with the new test-runner script.

This adds an "allow_omm" directory alongside "basic" and "slow". I've also moved the offending test into it, and it now passes on ARM. Also note that I've swapped a few ".match" calls for ".search", as ".match" will only match expressions at the beginning of the string, and I don't think this is what we want. (stat_re, for example, includes an explicit "^".)

Comment on attachment 394515 [details] [diff] [review] Create an "allow_oom" category to allow testBug507425 to pass on small platforms. Forwarding review to dmandelin, as he knows the rationale behind the existing conventions. A few python comments: >+ allow_oom = re.compile(r'allow_oom').search(path) You can just say: allow_oom = 'allow_oom' in path > assert_re = re.compile(r'Assertion failed:') > stat_re = re.compile(r'^Trace stats check failed') >+oom_re = re.compile(r': out of memory$') Likewise, all these can be replaced with simple `"..." in line` or `line.startswith("...")` tests. >+ # Assume no out-of-memory by default, but then scan through stderr and >+ # update hit_oom if necessary. >+ hit_oom = False >+ for line in err.split('\n'): >+ if oom_re.search(line): >+ hit_oom = True >+ break Here you could just write: hit_oom = ':out of memory\n' in err and it should probably go under the `if rc != 0:` test, to save some CPU--although none of our existing tests generate much output. >+ if not allow_oom or not hit_oom: > return False Maybe `return allow_oom and hit_oom` is more straightforward?

(In reply to comment #6) > hit_oom = ':out of memory\n' in err (oops, you would need a space after the : though)

>>+ allow_oom = re.compile(r'allow_oom').search(path) >You can just say: allow_oom = 'allow_oom' in path Cool, I didn't know that. I assumed that strings were treated as sequences for |in|. Anyway, regarding the patch, before reviewing the code itself, I have two design questions: (a) Is OOM to be counted as a pass on certain tests only for mobile platforms, or for all platforms? (b) On mobile platforms, is OOM to be counted as a pass for all tests, or only for specified tests?

(a) all platforms. (That's the intent of the patch, anyway, and it's OK with me.) (b) only for specified tests

Comment on attachment 394515 [details] [diff] [review] Create an "allow_oom" category to allow testBug507425 to pass on small platforms. I second jorendorff's comments. I agree that the test for OOM should go under the 'rc != 0' at the beginning of the current version of the function, not only for efficiency, but also for clarity--it took me some thought to realize that this inner if was correct: >+ if rc != 0: >+ # Allow a non-zero return code if we want to allow OOM, but only if we >+ # actually got OOM. >+ if not allow_oom or not hit_oom: > return False > > return True Putting it at the top and expressing it more like # compute hit_oom return hit_oom and allow_oom is much more obvious, I think. Otherwise, looks good.

> (a) Is OOM to be counted as a pass on certain tests only for mobile > platforms, or for all platforms? As Jason said, it would be a pass for all platforms. Hypothetically, there could be a non-mobile platform with insufficient memory to run the test. Perhaps more realistically for 2009, there could be also mobile platforms which are able to run the test fully. In any case, detecting whether or not a platform is "mobile" is difficult, but at least with this method we don't get failures when everything is actually working properly. > (b) On mobile platforms, is OOM to be counted as a pass for all tests, or > only for specified tests? Only for specified tests, as Jason said. The tests in the allow_oom/ directory will pass if OOM is hit (and no other error occurs). (A whole category may be overkill for one test case, but it seemed to be the simplest way to implement it.) ---- Python is magic! I've moved things around as you recommended.

I committed this to Trace Monkey on 2009-08-18, but forgot to update the bug. http://hg.mozilla.org/tracemonkey/rev/b15e0362b563