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)

Other
Linux
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla29
Tracking Status
firefox28 --- fixed
firefox29 --- fixed

People

(Reporter: stevensn, Assigned: luke)

References

Details

(Whiteboard: [qa-])

Attachments

(4 files, 2 obsolete files)

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.
Blocks: 941827
Does changing "JS_THREADSAFE" to "JS_WORKER_THREADS" for the initialization of cpuCount_ in vm/Runtime.cpp fix the problem?
No I still get the assertion with that Putting the JS_WORKER_THREADS ifdef to return false back in CanCompileOffThread does this this assertion
Attached patch fix-ifdef (deleted) — Splinter Review
Oops, sorry, I had you change the wrong place. I assume this one also fixes the assert?
Assignee: nobody → luke
Status: NEW → ASSIGNED
Attachment #8348798 - Flags: review?(bhackett1024)
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+
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?
It should be fine to wait for the better fix.
Attached patch remove JS_WORKER_THREADS (deleted) — Splinter Review
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)
Comment on attachment 8349458 [details] [diff] [review] remove JS_WORKER_THREADS Nice, thanks!
Attachment #8349458 - Flags: review?(luke) → review+
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla29
This bug is also present on aurora
Attached patch remove JS_WORKER_THREADS Aurora (obsolete) (deleted) — Splinter Review
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?
Attachment #8350947 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Attached patch remove JS_WORKER_THREADS Aurora (obsolete) (deleted) — Splinter Review
Updated patch to fix an #ifdef changed missed in the merge to aurora (Runtime.h)
Attachment #8350947 - Attachment is obsolete: true
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'
Does adding a include jsscriptinlines.h to the top of jsworkers.cpp fix the error?
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.
Unfortunately packaging still fails, so this doesnt fix #953211..
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".
In that case i dont think header ordering matters since JS_THREADSAFE isnt defined on those platforms (from what i remember)
Attached patch add jsscriptinlines include (deleted) — Splinter Review
This patch (for mozilla-central) adds the #include "jsscriptinlines.h"
Attachment #8355969 - Flags: review?(luke)
Attached patch remove JS_WORKER_THREADS Aurora (deleted) — Splinter Review
Updates the patch for aurora so it includes the jsscriptinlines.h include
Attachment #8355830 - Attachment is obsolete: true
Attachment #8355970 - Flags: review?(luke)
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)
Attachment #8355969 - Flags: review?(luke) → review+
8355969 is ready for m-i
Keywords: checkin-needed
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)
Whiteboard: [qa-]
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: