Closed
Bug 950513
Opened 11 years ago
Closed 11 years ago
assertion in StartOffThreadParseScript when JS_WORKER_THREADS is false
Categories
(Core :: JavaScript Engine, defect)
Tracking
()
RESOLVED
FIXED
mozilla29
People
(Reporter: stevensn, Assigned: luke)
References
Details
(Whiteboard: [qa-])
Attachments
(4 files, 2 obsolete files)
(deleted),
patch
|
bhackett1024
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
luke
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
luke
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
Details | Diff | Splinter Review |
Starting firefox (mozilla-central)
I get an assertion in
js::StartOffThreadParseScript
MOZ_ASSUME_UNREACHABLE("Off thread compilation not available in non-THREADSAFE builds");
This is in the JS_WORKER_THREADS undef version of the function
JS::CompileOffThread is calling this. JS::CanCompileOffThread returns true
because
parallelParsingEnabled_ is set to true on line 303 of Runtime.cpp and isn't ever set to false.
This is on a non-ion build on ppc64.
Assignee | ||
Comment 1•11 years ago
|
||
Does changing "JS_THREADSAFE" to "JS_WORKER_THREADS" for the initialization of cpuCount_ in vm/Runtime.cpp fix the problem?
Reporter | ||
Comment 2•11 years ago
|
||
No I still get the assertion with that
Putting the JS_WORKER_THREADS ifdef to return false back in CanCompileOffThread does this this assertion
Assignee | ||
Comment 3•11 years ago
|
||
Oops, sorry, I had you change the wrong place. I assume this one also fixes the assert?
Comment 4•11 years ago
|
||
Comment on attachment 8348798 [details] [diff] [review]
fix-ifdef
Review of attachment 8348798 [details] [diff] [review]:
-----------------------------------------------------------------
This will disable the background sweeping thread when !defined(JS_ION), so isn't ideal. JS_WORKER_THREADS is really a total mess, I'll try to write a patch today or tomorrow to remove it entirely.
Attachment #8348798 -
Flags: review?(bhackett1024) → review+
Assignee | ||
Comment 5•11 years ago
|
||
Good point, and that sounds like a much nicer fix. Shall I just wait for that patch then or do you think it is worth landing this?
Comment 6•11 years ago
|
||
It should be fine to wait for the better fix.
Comment 7•11 years ago
|
||
Remove JS_WORKER_THREADS and replace it with JS_THREADSAFE, using JS_ION in places where Ion-specific stuff is handled by the worker threads. This builds and passes jit-tests with all combinations of {--disable-ion, --disable-threadsafe}. In doing so this fixes an existing issue where --disable-threadsafe builds did not pass asm.js jit-tests on MacOS due to IsAsmJSCompilationAvailable being out of sync with EstablishPreconditions.
Attachment #8349458 -
Flags: review?(luke)
Assignee | ||
Comment 8•11 years ago
|
||
Comment on attachment 8349458 [details] [diff] [review]
remove JS_WORKER_THREADS
Nice, thanks!
Attachment #8349458 -
Flags: review?(luke) → review+
Comment 9•11 years ago
|
||
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla29
Reporter | ||
Comment 11•11 years ago
|
||
This bug is also present on aurora
status-firefox28:
--- → affected
Reporter | ||
Comment 12•11 years ago
|
||
I have updated the patch so it applies on Aurora
[Approval Request Comment]
Bug caused by (feature/regressing bug #): 941827
User impact if declined: Non-ion builds will abort on assertion at startup
Testing completed (on m-c, etc.): This has been on m-c for a few days
Risk to taking this patch (and alternatives if risky): Low risk only #ifdef changes
String or IDL/UUID changes made by this patch: None
Attachment #8350947 -
Flags: approval-mozilla-aurora?
Updated•11 years ago
|
Attachment #8350947 -
Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Comment 13•11 years ago
|
||
status-firefox29:
--- → fixed
Comment 14•11 years ago
|
||
This hit asserts on debug builds. Backed out.
https://hg.mozilla.org/releases/mozilla-aurora/rev/fefdc280c829
https://tbpl.mozilla.org/php/getParsedLog.php?id=32530865&tree=Mozilla-Aurora
Reporter | ||
Comment 15•11 years ago
|
||
Updated patch to fix an #ifdef changed missed in the merge to aurora (Runtime.h)
Attachment #8350947 -
Attachment is obsolete: true
Reporter | ||
Comment 16•11 years ago
|
||
Comment 17•11 years ago
|
||
I think something (maybe this) has regressed sparc64 builds, because now m-c builds fail with:
../libjs_static.a(jsworkers.o)(.text+0xcb0): In function `js::WorkerThreadState::finishParseTask(JSContext*, JSRuntime*, void*)':
/home/buildslave/mozilla-central-sparc64/build/js/src/jsworkers.cpp:663: undefined reference to `JSScript::global() const'
collect2: ld returned 1 exit status
when linking the js shell - and if i transplant https://hg.mozilla.org/releases/mozilla-aurora/rev/d56d13a32786 on my aurora tree (when trying to fix #953211, ie aurora builds but fails to package/xpcshell blows) then aurora builds fail with the exact same failure:
../../../dist/bin/libnspr4.so.1.0: warning: sprintf() is often misused, please use snprintf()
../libjs_static.a(jsworkers.o)(.text+0xcf8): In function `js::WorkerThreadState::finishParseTask(JSContext*, JSRuntime*, void*)':
/home/buildslave/mozilla-aurora-sparc64/build/js/src/jsworkers.cpp:650: undefined reference to `JSScript::global() const'
Reporter | ||
Comment 18•11 years ago
|
||
Does adding a include jsscriptinlines.h to the top of jsworkers.cpp fix the error?
Comment 19•11 years ago
|
||
Yes that helps (ie adding #include "jsscriptinlines.h" just below #include "jsworkers.h"), js shell is built - i'll see in some hours if packaging succeeds on aurora branch - and i'll try the same chunk to see if it fixes the build on m-c.
Comment 20•11 years ago
|
||
Unfortunately packaging still fails, so this doesnt fix #953211..
Comment 21•11 years ago
|
||
Does it fail on the same thing? Note that a script is run to enforce include ordering in js/src, so #include "jsscriptinlines.h" should be right after #include "jsobjinlines.h".
Comment 22•11 years ago
|
||
In that case i dont think header ordering matters since JS_THREADSAFE isnt defined on those platforms (from what i remember)
Reporter | ||
Comment 23•11 years ago
|
||
This patch (for mozilla-central) adds the #include "jsscriptinlines.h"
Attachment #8355969 -
Flags: review?(luke)
Reporter | ||
Comment 24•11 years ago
|
||
Updates the patch for aurora so it includes the jsscriptinlines.h include
Attachment #8355830 -
Attachment is obsolete: true
Attachment #8355970 -
Flags: review?(luke)
Comment 25•11 years ago
|
||
I can confirm that adding that #include "jsscriptinlines.h" on top of jsworkers.cpp fixes the build issue on m-c i was seeing on sparc64, which packages fine (see http://buildbot.rhaalovely.net/builders/mozilla-central-sparc64/builds/664)
It fixes the build on aurora too, but the latter still fails during make package (cf #953211)
Assignee | ||
Updated•11 years ago
|
Attachment #8355969 -
Flags: review?(luke) → review+
Comment 27•11 years ago
|
||
Keywords: checkin-needed
Comment 28•11 years ago
|
||
Comment on attachment 8355970 [details] [diff] [review]
remove JS_WORKER_THREADS Aurora
A bustage fix and previously-r+ed changes doesn't say "needs a new review" AFAICT. Feel free to yell at me if I'm wrong :)
Attachment #8355970 -
Flags: review?(luke)
Comment 29•11 years ago
|
||
You need to log in
before you can comment on or make changes to this bug.
Description
•