Closed Bug 950513 Opened 10 years ago Closed 10 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-ifdefSplinter 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.
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+
https://hg.mozilla.org/mozilla-central/rev/b41d9ac46315
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla29
This bug is also present on aurora
Attached patch remove JS_WORKER_THREADS Aurora (obsolete) — 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) — 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)
This patch (for mozilla-central) adds the #include "jsscriptinlines.h"
Attachment #8355969 - Flags: review?(luke)
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.