Closed Bug 1280407 Opened 3 years ago Closed 3 years ago

Don't use mozilla::Vector with default alloc policy

Categories

(Core :: JavaScript Engine, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla50
Tracking Status
firefox50 --- fixed

People

(Reporter: jonco, Assigned: jonco)

Details

Attachments

(3 files)

mozilla::Vector's default alloc policy is mozilla::MallocAllocPolicy, which doesn't support simulated OOM testing.  In SM we should be using SystemAllocPolicy instead which is otherwise equivalent but does supports this.
Changes to GC code and jsapi tests.

This caught a problem in debug builds where we tried to release held relocated arenas twice if we hit OOM.
Assignee: nobody → jcoppeard
Attachment #8763171 - Flags: review?(sphink)
Update the stopwatch API.
Attachment #8763172 - Flags: review?(jdemooij)
Update UBINode.  There were a ton of mozilla::Vectors used in the public headers so I made an alias for the full type as JS::ubi::Vector.

I added testcases for the functionality exposed to the shell and had to make a couple of fixes to report the OOM in TestingFunctions.cpp.  Happily this didn't reveal any major bugs.

It does look like the UBI APIs don't do any error reporting themselves and so callers can't readily distinguish OOM from other errors, e.g. something that goes wrong in the handler supplied to BreadthFirst.  This is kind of inconvenient but I'm not sure it's really serious.
Attachment #8763180 - Flags: review?(nfitzgerald)
Attachment #8763171 - Flags: review?(sphink) → review+
Comment on attachment 8763180 [details] [diff] [review]
bug1280407-vector-alloc-policy-ubinode

Review of attachment 8763180 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks! And I thought I was pretty careful about OOM reporting... :-/ errormageddon can't come soon enough
Attachment #8763180 - Flags: review?(nfitzgerald) → review+
Comment on attachment 8763172 [details] [diff] [review]
bug1280407-vector-alloc-policy-stopwatch-api

Review of attachment 8763172 [details] [diff] [review]:
-----------------------------------------------------------------

Good find.
Attachment #8763172 - Flags: review?(jdemooij) → review+
Pushed by jcoppeard@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/a58b9ab5dff0
Use SystemAllocPolicy rather that the default with mozilla::Vector in the JS engine r=sfink r=fitzgen r=jandem
Backed out for errors in testThreadingThread.cpp in SM(nu):

https://hg.mozilla.org/integration/mozilla-inbound/rev/c6f2a2408e4d

Push with failure: https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=a58b9ab5dff082130cfa6d4c8cac8f5d09d204a2
Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=30284292&repo=mozilla-inbound


/home/worker/workspace/build/src/js/src/jsapi-tests/testThreadingThread.cpp:65:36: error: 'SystemAllocPolicy' was not declared in this scope
/home/worker/workspace/build/src/js/src/jsapi-tests/testThreadingThread.cpp:65:53: error: template argument 3 is invalid
/home/worker/workspace/build/src/js/src/jsapi-tests/testThreadingThread.cpp:65:56: error: invalid type in declaration before ';' token
/home/worker/workspace/build/src/js/src/jsapi-tests/testThreadingThread.cpp:67:22: error: request for member 'emplaceBack' in 'v', which is of non-class type 'int'
/home/worker/workspace/build/src/js/src/jsapi-tests/testThreadingThread.cpp:68:22: error: request for member 'length' in 'v', which is of non-class type 'int'
/home/worker/workspace/build/src/js/src/jsapi-tests/testThreadingThread.cpp:70:21: error: no matching function for call to 'begin(int&)'
/home/worker/workspace/gcc/include/c++/4.8.5/bits/range_access.h:58:5: error: request for member 'begin' in '__cont', which is of non-class type 'const int'
/home/worker/workspace/gcc/include/c++/4.8.5/bits/range_access.h:48:5: error: request for member 'begin' in '__cont', which is of non-class type 'int'
/home/worker/workspace/build/src/js/src/jsapi-tests/testThreadingThread.cpp:70:21: error: no matching function for call to 'end(int&)'
/home/worker/workspace/gcc/include/c++/4.8.5/bits/range_access.h:78:5: error: request for member 'end' in '__cont', which is of non-class type 'const int'
/home/worker/workspace/gcc/include/c++/4.8.5/bits/range_access.h:68:5: error: request for member 'end' in '__cont', which is of non-class type 'int'
make[3]: *** [testThreadingThread.o] Error 1
Flags: needinfo?(jcoppeard)
Pushed by jcoppeard@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/afc3c6a5f93a
Use SystemAllocPolicy rather that the default with mozilla::Vector in the JS engine r=sfink r=fitzgen r=jandem
Backed out for mass spidermonkey failures:

https://hg.mozilla.org/integration/mozilla-inbound/rev/0d735d33bd84

Push with failures: https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=afc3c6a5f93a4e5b5309659a13a84e7ec5c8fb2e

js/src/tests/style/BadIncludes.h:10: error:
+js/src/jsapi-tests/testThreadingThread.cpp:8:10: error:
js/src/tests/style/BadIncludesOrder-inl.h:5:6: error:
TEST-UNEXPECTED-FAIL | check_spidermonkey_style.py | actual output does not match expected output; diff is above
Pushed by jcoppeard@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/f29ac79eba91
Use SystemAllocPolicy rather that the default with mozilla::Vector in the JS engine r=sfink r=fitzgen r=jandem
Flags: needinfo?(jcoppeard)
https://hg.mozilla.org/mozilla-central/rev/f29ac79eba91
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla50
You need to log in before you can comment on or make changes to this bug.