mozilla :: #jsapi

23 Feb 2016
00:42mwuterrence: http://hastebin.com/ejonajijey
00:42* mwu checks if it actually runs
00:43mwushell starts at least
00:48mwuguess I have to hook it into jemalloc for real to see if it's any good
01:14terrencemwu: that looks surprisingly reasonable
01:15terrencemwu: loooong tail
01:15mwuterrence: if you just give up on C strings and things crossing jsapi, it mostly works out.
01:15mwujust annoying to check all the call sites
01:16terrencethanks for doing the work! let's get it landed now
01:16mwu\o/
01:17mwugoing to hook it up to jemalloc first and see if it survives..
01:18bbouvierhello world
01:19bbouvierwell, hello seattle, actually
01:19shuis it raining
01:21bbouviershu: no, clear blue sky, hot sun, perfect weather
01:21shubbouvier: bummer
01:25bbouvierhah, time zone change => can't log in into bugzilla anymore because of 2fa
02:52mrgigglesthe mozilla-inbound tree is now closed (Bug 1250374 - Mac debug build infra bustage)
02:52firebothttps://bugzil.la/1250374 NEW, nobody@mozilla.org Trees closed, Mac debug builds failing when they try to tell taskcluster that they are done
02:52sfink\o/
02:53sfink(landed *just* before the closure)
04:17philorsfink: and as your reward, flames
04:34philorhazard build flames, from your push, that is
06:35sfinkbut but but I have a try push with the hazard builds in them... :(
06:35sfinkoh, right. And loading up TH with mozilla-inbound reliably crashes firefox
06:35sfinkI ought to figure that out sometime
06:35sfinkincluding why I get no crash reports from it
06:36sfinkChrome, here I come
06:38sfinkoh, how nice. It's constantly crashing gcc.
06:42sfinker... ok, maybe that try push didn't have what I thought it had
06:43sfinkguess it's coming back out
11:07nbpWhoa, people are still relying on mozjs-1.8.5 which is about as old as Firefox 4.
11:18Ms2gerNow if only we actually released new versions regularly
11:31Philip_And without completely breaking API compatibility every time
11:46evilpie"completely"
11:47evilpie"API"
14:18mrgigglesthe mozilla-inbound tree is now open
14:43nbpmrgiggles: pun
14:43mrgigglesnbp: An office with many people and few electrical outlets could be in for a power struggle.
14:44nbpThis does not sounds like a pun, but more like a reallity in some places (3/10)
14:55smvvnbp: do you have any idea why my tryserver results are different compared to my test results I get locally? https://pastebin.mozilla.org/8860790 and https://treeherder.mozilla.org/#/jobs?repo=try&revision=afd6ace6afac
14:56smvvnbp: i've also ran the {jit,js}tests with debug mode enabled (--enable-debug --disable-optimize --disable-jemalloc --enable-valgrind)
14:57nbpsmvv: try --enale-gczeal on the configure command line.
14:58smvvfor debug or opt, or both?
14:58nbpsmvv: debug
14:59nbpsmvv: Also, you are not running the jsreftest in the browser
15:00nbpsmvv: the js/src/tests directory is executed twice, I tihnk. Both in the JS Shell, and a second time with jsreftest.html
15:00smvvnbp: how do I do that? This section is empty https://developer.mozilla.org/en-US/docs/Mozilla/Projects/SpiderMonkey/Running_Automated_JavaScript_Tests#Running_JSTest_in_a_browser
15:01nbpsmvv: I do not recall exactly, but I think you have to build the xpcshell to do that
15:01smvvnbp: I cannot start nightly with my patch applied. It is crashing somewhere in xpconnect
15:19smvvnbp: I've rebuild debug with: ../configure --enable-gczeal --disable-optimize --enable--debug, ran the jit and js tests .. but both test suits are still passing
15:20nbpsmvv: hum maybe you can try to see if you can reproduce with the arm simulator
15:20nbpsmvv: which involves cross-compiling to x86 and enabling the arm simulator
15:20smvvnbp: this is on x86-64bit btw
15:21nbpsmvv: I presonnally compile natively to x86 on x86-64 instead of doing some cross compilation.
15:22smvvnbp: with -m32 ?
15:22nbpsmvv: no, I have an x86 toolchain.
15:23nbpsmvv: everything from the libc to gcc is compiled natively to x86 :P
15:23smvvnbp: I'm familiar with the crossdev tool, but before I go down that road, why do you think it has to do with the architecture? The failures on the try server are also on x86-64 with a debug build
15:23nbpsmvv: Others might have ideas on how to cross compile.
15:24nbpsmvv: because the red "arm" is a cross-compiled x86 build with the arm simulator enabled.
15:24smvvnbp: http://crosstool-ng.org/ <-- this works well for a cross compiled toolchain
15:25nbpsmvv: I just use my package manager, which works better than any other custom tool.
15:25smvvnbp: i see what you mean. But the x86-64 bit debug build fails as well. Isn&#39;t it a good idea to start with that one first?
15:26nbpsmvv: cgc is supposed to be --enable-gczeal, if I recall correctly
15:26nbpsmvv: you can also try to find the leaks reported by Valgrind
15:27smvvnbp: it is required to use --enable-valgrind when running under valgrind, right?
15:27smvvnbp: i&#39;ll try running the tests with --valgrind and --enable-valgrind next
15:27nbpsmvv: yes, as well as --smc-check=all-non-file on valgrind command line
15:28smvvnbp: ok
15:28nbpsmvv: the hacking tips page should give you the required flags.
15:29nbpsmvv: I think we also need another one, but only when dealing with the interruption callback of asmjs.
15:42fitzgentill: by chance does the new Promise impl fix the run-to-completion issues the debugger exposed with the current promise impl? The debugger will &quot;pause&quot; and then Promise handlers still run right now
15:43tillfitzgen: I&#39;m not sure, but probably not
15:43tillfitzgen: the way promise events are triggered doesn&#39;t change, and ISTM that would be the deciding factor
15:43fitzgen&quot;probably not fix&quot; or &quot;probably not affected anymore&quot; ?
15:43fitzgenok
15:44tillfitzgen: ejpbruel would probably know more
15:44fitzgentill: so is SM farming out to gecko for microtask queueing then?
15:46tillfitzgen: yep. There really isn&#39;t an alternative as JS doesn&#39;t have the required knowledge
15:47fitzgenok
15:47fitzgentill: thanks!
15:47tillsure
16:10sfinksmvv: looks like you&#39;re getting shell crashes, so I&#39;d tackle those first since they&#39;re faster and easier to run
16:11sfinksmvv: SM(cgc) is run with JS_GC_ZEAL=14, but you have crashes in SM(e), which doesn&#39;t require any special settings
16:11sfinksmvv: if you want to do close to an exact reproduction, you can run |js/src/devtools/automation/autospider.sh warnaserrdebug|
16:12sfinksmvv: but that should be pretty much the same as configuring with just the flag --enable-debug and runnings the jstests in the tests/ dir
16:16smvvsfink: ah thanks. I&#39;ll try that next :)
16:17smvvsfink: what triggers SM(r) ? (The description says &#39;root analysis&#39;)
16:18sfinkjs/src/devtools/automation/autospider.sh rootanalysis
16:18sfinkyou can see the configure flags it uses in js/src/devtools/automation/variants/rootanalysis
16:18sfinkbut it also sets JS_GC_ZEAL=7 directly in autospider.sh
16:24nbpsfink: would be good to have this knowledge on the HAcking tips page, on how to reproduce locally each of the TH configurations.
16:25sfinkyou&#39;re right. I think it&#39;s linked to off of the TH help page, but nobody ever finds it there.
16:25sfinkI&#39;ll put it on the hacking tips page
16:29Waldomrgiggles: pun
16:29mrgigglesWaldo: Old burglars never die they just steal away.
16:29Waldomrgiggles: can JS_GC gc?
16:29mrgigglesWaldo: No, |void JS_GC(JSRuntime*)| cannot GC
16:29Waldothe lol continues
16:30sfinkoh, right. I should get back to that. I reloaded the DB, all 707K functions, and it&#39;s now happily running... with the original 50k.
16:31sfinkmaybe this &quot;commit&quot; thing will do something useful?
16:31Waldobz: super-low-priority ping
16:33Waldolth: you meant to WONTFIX that gcc5 bug?
16:35lthWaldo: until i know for sure what&#39;s going on. (unless you&#39;re talking wontfix vs invalid)
16:35Waldolth: well, if it&#39;s clearly wrong behavior, that seems unquestionably like a bug *somewhere* we have to solve, so I don&#39;t understand resolving it at all, now
16:35lthhrm
16:36lthi just assumed --enable-more-deterministic was broken by design
16:36Waldothat BigNum signed issue also seems like a real problem, if conceivably a red herring for this instantaneous problem
16:36lthanyway i have a new gcc5 build now without that switch
16:36lthWaldo: the bignum warning appears to show up even with gcc4 but i&#39;ve had too many compiles this afternoon to be 100% sure about what i&#39;ve seen
16:37Waldolth: --emd is I believe 99% fully-compliant with the spec, it&#39;s just more restricted behavior in cases where the spec doesn&#39;t guarantee only a single behavior
16:37Waldomight be 100%, but I vaguely recall one or two exceptions
16:37lthhm
16:38lthstand by
16:38lthso. without --enable-more-deterministic the gcc5 build passes all jit-tests
16:38lthso i will reopen the bug but rename it
16:40decoderanyone up who is familiar with Rooting (RootingAPI.h) ?
16:41Waldodecoder: you may fire when ready, gridley
16:41decoder:D
16:41decoderokay so
16:41decoderwe have a problem with recent LLVM + ASan on firefox
16:41decoderstarting with LLVmM 3.8 we have a startup crash
16:41decoder-m
16:41decoderwe crash here: https://dxr.mozilla.org/mozilla-central/source/js/public/RootingAPI.h#639
16:41decoderi bisected the failure on the clang side
16:42decoderit&#39;s related to instrumenting dynamic allocations on the stack
16:42decoderI was wondering
16:42decoderwhat is stack here and is *stack actually safe to do?
16:42sfinkstack is the top of a linked list of Rooted structs
16:42decoderor is this API doing stuff with stack memory that needs exemption from memory correctness checking like asan?
16:43sfinkno, this &#39;stack&#39; is not *the* stack, though everything on that list will be stored on the C stack
16:43* Waldo grooves to Dave Brubeck early in the morning
16:43decodersfink: are these things still touched once the C stack has been unwinded?
16:43sfinkdecoder: they should not be
16:43Waldo~Rooted should handle that
16:43sfinktheir destructors should run, removing them from the list
16:44Waldoand I would expect ASAN screaming bloody murder if the memory were touched after stack unwind
16:44decoderWaldo: yes, asan has explicit use-after-return checking. and the errors im getting show no use-after-return
16:44decoderi either get a SIGSEGV
16:44decoderor some weird stack overflows/underflows
16:44decoderthat make no sense
16:44decoderasan devs be like
16:45decoder&quot;provide minimal testcase blahblah&quot;
16:45decoder>.>
16:45* decoder zips mozilla-central and sends it over
16:45Waldowhat, you can&#39;t minimize SpiderMonkey?
16:45decoder:D
16:45decoderif it was only spidermonkey
16:45decoderthat would be awesome
16:45decoderbut this is the full browser
16:45decoderi should actually try to run the jstests..
16:45Waldo|trollishboo-urns, there&#39;s a limit how many nicks you can put in a nickserv group? boooooooooooooooo-urns
16:46Waldo|trollishwhat, you can&#39;t minimize the browser?
16:46Ms2gerWaldo|trollish, your nick seems to be a pleonasm
16:46* Waldo|trollish jogs memory as to that word&#39;s meaning
16:47sfinkmozilla::EnumeratedArray zero-initializes its entries somehow, I hope?
16:47Waldo|trollishMs2ger: I&#39;ve joined the Redundancy Team responsible for redundancy
16:47sfinkit seems to inherit from mozilla::Array
16:48sfinkwhich just declares a T mArr[Length] member
16:48Waldo|trollishsfink: I think it&#39;s just default-initialization, which isn&#39;t zeroing, but don&#39;t quote me
16:48sfinkwhich, By The Power of C++, does something inscrutable and random
16:48Waldo|trollishI can look it up quickly enough
16:48sfinkWaldo, Keeper of the Inscrutable and Random
16:49* decoder makes a debug build now to figure out in GDB what it is crashing on
16:49sfinkoh
16:49sfinknever mind, RootLists initializes itself
16:49sfinkso yeah, it probably doesn&#39;t initialize that memory, but it doesn&#39;t matter
16:49decoderaccording to their devs, the feature they introduced is supposed to catch dynamic allocations on the stack
16:50decodere.g.
16:50decodervoid access(int index, int len) {
16:50decoder char str[len];
16:50decoder str[index] = &#39;1&#39;;
16:50decoder}
16:50decoderaccess(20, 10);
16:50decoderasan couldnt catch that before
16:50decoderwith that feature it csan
16:50decoder*can
16:50decoderit could also be a bug in their code of course
16:51sfinkdecoder: do you know what address it&#39;s crashing on in the *stack dereference?
16:52Ms2gercsan, huh
16:52sfinkbecause it&#39;s just taking the address of an array member and dereferencing that. It wouldn&#39;t even matter if we botched the initialization; that shouldn&#39;t crash.
16:52sfinks/array member/array element/
16:53sfinkthough that code reads a little weird -- why does it use &#39;this->stack&#39; in one place and a plain &#39;stack&#39; in two?
16:54sfinkmaybe we should create a here() identity method so that can be this->here->stack
16:56decodersfink: not yet, my build didnt have enough debug information in it. im rebuilding right now to find that out
16:56decoderthanks for helping me figuring this one out
16:57sfinkdecoder: you might also need to resort to printf debugging. Print out &roots.stackRoots_[0] and &roots.stackRoots_[JS::RootKind::Limit] or something, and compare those to &#39;stack&#39;
16:58decodersfink: okay. should I print these out right before the deref?
16:58sfinkdecoder: yes
16:59Waldo|trollishsfink: yeah, T arr[N]; where arr is not initialized explicitly in the member-list is default-initialized
16:59sfinkI suppose you could find a place to print them only once if it triggers excessive output
16:59sfinkWaldo|trollish: and default-initialized for PODs is zero, or uninitialized?
17:00Waldo|trollishsfink: and that means *only* that a default constructor is called if T is a class, but otherwise nothing else in it is initialized
17:00Waldo|trollishso no zeroing
17:00sfinkok, that&#39;s what I&#39;d expect, thanks
17:00Waldo|trollishyeah, it was mine too
17:00Waldo|trollishgood old [dcl.init] and [class.base.init]
17:04Waldosfink: you know if we&#39;ve ever considered splitting up the root list into a bunch of &#39;em for different common T? i.e. a list only of objects, only of Value, only of strings, etc.
17:05WaldoI imagine it wouldn&#39;t pay its way, just curious if we&#39;ve ever looked
17:06sfinkWaldo: I don&#39;t understand. That&#39;s how it already is?
17:06sfinkone list for each RootKind http://dxr.mozilla.org/mozilla-central/source/js/public/TraceKind.h#99
17:06Waldosfink: is it? I thought we had one root list, then we switched on a discriminant in each Rooted in the list when tracing
17:07sfinkwe have one array, where each slot is the head of the list for that RootKind
17:07sfinkhttp://dxr.mozilla.org/mozilla-central/source/js/src/jspubtd.h#273
17:08Waldohuh
17:08Waldowhen did that show up?
17:08sfink(of type Rooted<void*>, which is very very dangerous for aliasing since aliasing can easily break LIFO ordering)
17:08terrenceuh, it&#39;s always been like that?
17:08sfinkwe depend on that for type info
17:13nbpWaldo: When have you planned to review Bug 1248412 (again) ? I hope to land branch pruning next week, and this is one of the blocker.
17:13firebothttps://bugzil.la/1248412 NEW, nobody@mozilla.org +169% regression on misc-basic-array-forof (Jan 12).
17:13Waldonbp: I&#39;m about done with a review
17:13nbpWaldo: ok.
17:13Waldonbp: I am deliberating as to whether to complain more about your test or not
17:13Waldonbp: it&#39;s more understandable to me, now
17:14Waldonbp: but I&#39;m not sure whether I&#39;m happy with its scatter-shotty approach to testing, rather than doing targeted testing of exactly all the possible switch-values for the IsTypedArray and IsProxy constraints
17:15nbpWaldo: what would you do in such cases?
17:15nbpWaldo: doing the full spectrum takes way to much time, and I did not wanted to mark the test as slow, as I do not even run these test locally.
17:15Waldonbp: formulate entirely separate code that tests each particular case, in a very glass-box manner of testing that is 100% aware of how the implementation was done
17:16Waldonbp: targeted testing of the 4 (or 16, if we multiply the two) different possibilities needn&#39;t require full-spectrum testing, and it would not be slow
17:16nbpWaldo: then what is covered by the test is minimal?
17:16nbpWaldo: and we might miss cases?
17:17nbpWaldo: should I remove the for-loop then, and do a bunch a manual calls to the test function?
17:17Waldonbp: I think that&#39;s the direction I&#39;d be closer to leaning, yes
17:18Waldonbp: re missing cases, the code doesn&#39;t have *that* much state-space, IMO -- worry about missing things seems lowish to me
17:19nbpWaldo: I hope the comments in the test case helped at understanding the useless boiler plate :/
17:20nbpWaldo: getting into Ion with the right state of TI, is not an easy thing.
17:20Waldonbp: true enough
17:21nbpWaldo: especially in a repeated manner :P
17:29terrencejonco: ping
17:30Waldonbp: oh, I&#39;m certainly not badmouthing the guts of checkSamples :-)
17:33nbpmrgiggles: pun
17:33mrgigglesnbp: A backwards poet writes inverse.
17:37ejpbruelnaveed: does the js team currently have a weekly meeting that i could attend?
17:38ejpbruelterrence: jandem: mccr8: i&#39;m running into gc crashes again in bug 1241841
17:38firebothttps://bugzil.la/1241841 REOPENED, janx@linux.com Assertion failure: isEmpty(), at dist/include/mozilla/LinkedList.h:328
17:38terrenceejpbruel: mtg... bbiab
17:38ejpbrueli posted a patch that reproduces the issue reliably on my machine
17:38ejpbruelcan you help me look into this? this is currently blocking me on multiple fronts wrt worker debugging
17:39ejpbruelterrence: kk
17:39naveedEjpbruel: there isn&#39;t a regular JS meeting. Is there something specific you want to keep up on? A method may already exist for that.
17:40ejpbruelnaveed: i mainly want to bring bug 1241841 to everyones attention. we keep running into it, and its really starting to interfere with worker debugging work.
17:40naveedejpbruel: oh that is important. Thanks for making a repo case for us.
17:41ejpbruelnaveed: sure thing. let me know if there&#39;s anything i can do to help push things forward.
17:42naveedejpbruel: emailing or need infoing me works in the future if a bug seems stuck and is blocking you. Thanks
17:42Waldomrrrgn, jorendorff: &quot;`({x} += {})` should be a ReferenceError (not a SyntaxError -- don&#39;t ask me why).&quot; just FUD on the spec authors&#39; parts
17:43Waldomrrrgn, jorendorff: I intend to experiment with making it a SyntaxError at some point, maybe start of next cycle is the time to pounce
17:43ejpbruelnaveed: good to know!
17:43mrrrgnWaldo: oy, okay I&#39;ll see what I can do while I clean up that patch
17:43Waldomrrrgn: make it a ReferenceError for now, for spec-consistency of the moment
17:44mrrrgnWaldo: ah, okay, I&#39;ll just file a bug then
17:44* mrrrgn wipes forehead
17:44mrrrgn:)
17:44Waldowe should disentangle feature-work from spec-changing, most definitely
17:44jorendorffmrrrgn: agree - it should be easy to follow the spec, because it&#39;s just a constant in js.msg
17:44mrrrgnyeah
17:44jorendorffWaldo: is bterlson on board with changing all early errors to SyntaxErrors if we find that it&#39;s web-compatible?
17:44Waldojorendorff: I think he said so when I groused to him about this, once
17:44jorendorffit would be kind of nice
17:45Waldojorendorff: note also we have some inconsistencies, in that *some* ReferenceError-y syntax we make into a SyntaxError in strict mode, I think for |foo() = 5| or something near it
17:45Waldono one has complained about this inconsistency yet
17:46Waldoor about us making foo() = 5 an early error in strict mode when the spec technically doesn&#39;t say it is, or says it&#39;s RE or something
17:46Waldomemory hazy
17:53evilpieWaldo: is assignment to functions even defined?
17:54Waldoevilpie: ES6 might have aspirationally forbidden it, can&#39;t remember
17:54Waldoevilpie: we tried to forbid and ran into dead code with it at the time, so we made it a runtime error (syntax error in strict mode)
17:54Waldoevilpie: it&#39;s the sort of thing I imagine could be made real in the real world given time for it to die
17:55Waldoevilpie: anyone who thinks it can&#39;t be changed, because it can&#39;t be changed short term, tho, would be failing to consider any possibilities but the most immediate
17:55evilpiehttp://tc39.github.io/ecma262/#prod-AssignmentExpression LeftHandSideExpression includes CallExpresison
17:55Waldoevilpie: they might have reconciled ES7 with reality of the moment
17:58jandemterrence: ping
18:08jorendorffWaldo: world&#39;s stupidest question
18:08jorendorffWaldo: http://mxr.mozilla.org/mozilla-central/source/js/src/frontend/Parser.cpp#9389
18:08Waldojorendorff: world&#39;s stupidest answer
18:08jorendorffWaldo: ...
18:09Waldo:-)
18:09jorendorffWaldo: never mind, i think i got it
18:09jorendorffexprInParens is weird, is the answe
18:09jorendorffr
18:09Waldojorendorff: its always being PredictInvoked, when it&#39;s called for syntactically-parenthesized things, is pretty dumb, if that&#39;s what you referred to
18:10Waldoonly for the primaryExpr caller does that make any sense
18:10jorendorffWaldo: oh my question was way dumber than that
18:10jorendorffalthough yes that does seem kind of dumb
18:10Waldowe need to remove the must-be-paren&#39;d requirement from generator functions at some point
18:11jorendorffWaldo: which requirement?
18:12Waldojorendorff: er, from yield expressions, sorry
18:12jorendorffoh
18:12jorendorffWaldo: I don&#39;t think that&#39;s on file. Do you have a moment to file it?
18:14jandemterrence: i managed to rr record our mysterious gc crash, but not sure how this case is supposed to behave
18:17jorendorffjandem: !!!!
18:17jorendorffjandem: that&#39;s extraordinary. congrats.
18:18jandemit took me all day, partly because rr in vmware fusion gave replay divergences, even though it&#39;s supposed to work
18:18jandembut a physical machine was okay, just took a lot longer to repro
18:18jandembut the nice thing about rr is that you only need one crash and you&#39;re good
18:18jandemjorendorff: thanks
18:23mccr8awesome
18:27sfinkwhoa, wait, which one? FinalizeTypedArenas?
18:28jandemyeah
18:28sfink!
18:28jandembut i need to walk through it with someone who knows the freespan code better
18:28Waldojorendorff: can do
18:29sfinkooh, I knew that once, jonco&#39;s been in it more recently, ehoogeveen might be the most familiar
18:29sfinkand terrence fell off the network
18:29sfinkI&#39;m not sure how familiar he is with it
18:29jandemehoogeveen, jonco: ping
18:29jandemsfink: thanks
18:30joncojandem: pong
18:30jandemjonco: hey do you have a few minutes to talk about this finalize crash? i ahve a recording but i&#39;m not sure how it&#39;s supposed to behave
18:30joncojandem: sure!
18:30jandemjonco: ok, so we trigger a minor gc here, under the ZoneCellIter constructor: https://dxr.mozilla.org/mozilla-central/source/js/src/vm/HelperThreads.cpp#1116
18:31Waldofiled bug 1250589
18:31jandemjonco: as part of that, we allocate a new Arena, and new ArenaHeaders are marked as fully used
18:31firebothttps://bugzil.la/1250589 NEW, nobody@mozilla.org Remove the must-be-parenthesized requirement from yield expressions
18:31terrencejonco, sfink, fitzgen, jimb: sorry, network went down *hard*
18:32jandemjonco: then later on in gc::MergeCompartments, we move this arena (and the rest in the parse-Zone) to the main Zone
18:32joncojandem: right
18:32fitzgenterrence: we wrapped up the meeting, seemed like everyone was done
18:32sfinkterrence: jandem caught the FinalizeTypedArenas crash in rr and is walking throuhg it
18:32terrenceI haven&#39;t had a 5+ minutes service interruption in close to a year now
18:32fitzgen\o/
18:32fitzgenjandem: nice :)
18:32terrencejandem+=Inf
18:33jandemjonco: but then a bit after that, we are in Finalize* code on the background thread, and the FreeSpan is still empty -- what is supposed to update that?
18:33jorendorffWaldo: thanks!
18:34Waldojorendorff: well, technically scumbag-me could/should have filed that years ago, rather than mentally sitting on it, but okay :-)
18:34joncojandem: huh, good question just looking at how this works
18:34jorendorffWaldo: the arc of history is long...
18:34efaustjandem isn&#39;t the hero we deserve...
18:34sfinkuh, isn&#39;t there a sort of &quot;working&quot; free span that the real free span gets moved to while the stored FreeSpan is marked empty? Let me look at this stuff again too...
18:35Waldobending the bug-curve
18:35Waldoideally
18:36terrenceyes, that&#39;s AutoCopySomethingSomething
18:36terrenceAutoCopyFreeListsToArenas
18:37ehoogeveenjandem: I can&#39;t remember this well enough to comment :(
18:37terrenceand those are going out of sync between the background sweeping, the parsing thread, and the main thread?
18:37terrenceI was actually wondering how much that bought us these days?
18:37jandemterrence, sfink, jonco: so maybe we have to copy the free lists from the parse-Zone to the main-Zone?
18:37ehoogeveenjandem: I&#39;ll have a look after dinner if there&#39;s no consensus yet
18:37joncolooks like ZoneCellIter calls ArenaLists::copyFreeListToArena() and clearFreeListInArena() itself
18:38jandemah
18:38joncothat sounds fine though, the problem would be if it didn&#39;t copy the free list to the arena and left it looking like it was allocated when it was infact not
18:38terrencehow does the parsing thread synchronize with GC?
18:39sfinkit does an AutoFinishGC followed by an AutoAssertNoAlloc
18:39terrencedoes AutoFinishGC wait on background sweeping?
18:40terrencewill it after I push my patch that does BG sweeping under the GC?
18:40sfinkit calls rt->gc.waitBackgroundSweepEnd() and rt->gc.nursery.waitBackgroundFreeEnd()
18:41joncojandem: so can you see in rr what happened in ZoneCellIter constructor, specifically if it called copyFreeListToArena() or set lists to null?
18:41terrenceokay... and I guess we can&#39;t trigger more GC in that function?
18:41sfinkactually, we explicitly do a minor GC!
18:41terrenceyeah, make the heap complete
18:41jandemjonco: sure let me try that..
18:41sfinkbut do minor GCs ever kick off background goop?
18:41jandembut the minor gc there ends up allocating the mysterious Arena
18:41terrenceAH!
18:41sfinkwhile we&#39;re within an AutoAssertNoAlloc, no less
18:42terrenceoh, wonderful!
18:42joncohmmm, wait evictNursery() can trigger a major GC?
18:43terrencewait, wha?
18:43terrenceit certainly shouldn&#39;t be able to!
18:43joncothat would be bad
18:43joncook, no I don&#39;t think that can happen
18:44sfinkyeah, I don&#39;t see a path there
18:45jandemyeah it&#39;s just tenuring
18:46jandemjonco: but where are we supposed to add this new arena to the free list?
18:46sfinkjandem: so is it crashing while finalizing the mystery arena?
18:47jandemsfink: yup
18:47jandemthere&#39;s 1 object in it but we think it&#39;s completely full
18:47joncojandem: the free list is the list of free cells in the area
18:47joncojandem: which is sometimes stored outside of the arena for performance reasons
18:48terrenceright, because the free list in the arena header is out of sync with the free list in the arena lists?
18:48joncojandem: and gets copied back and forth when we need to actually examine arenas e.g. when we GC
18:48sfinkbut I would have expected that to get flushed back into the arena header before it went onto a background sweep
18:48jandemjonco: ok so in this case, we allocate an arena and don&#39;t give it its own free list, so we have to add it to the other list right? does that happen on gc or should that happen when we allocate the arena?
18:49terrencejandem: allocation should happen into the arena lists&#39; free list
18:50terrencejandem: I think the free list management (in particular this copy back and forth) is igor&#39;s work
18:50jandemjonco: to answer your question, we call copyFreeListToArena in the constructor
18:50joncojandem: interesting, thanks
18:50terrencejandem: and I&#39;m pretty sure it was done to improve cache performance on allocation
18:51terrencejandem: it also happens to simplify code generation for the jit since we don&#39;t have to lookup and store the actual arena header pointer... we just bake in the arena lists free list address
18:51jandemjonco: oh wait, but that doesn&#39;t matter because kind is OBJECT_GROUP
18:51jandemjonco: and our arena is an OBJECT0 so its free list is unrelated IIUC
18:52jandem(OBJECT0_BACKGROUND)
18:52joncojandem: ah yes
18:52jandemterrence: ah yes that makes sense
18:52terrencejandem: I think we also store the free list packed in the header, so there&#39;s less code around updating it
18:53terrencejandem: but even with all of that, I don&#39;t think any of it makes any sense now
18:53terrencejandem: so as a larger scale fix, one option would be to just remove the indirection
18:53terrencejandem: otoh, we need a patch 2 weeks ago, so let&#39;s see if we can figure out a minimal fix
18:54terrencejandem: sorry, bit of a tangent
18:54joncojandem: it&#39;s interesting though, this code is certainly suspicious
18:54jandemterrence: so we mark the arena as full here, when we init() it: https://dxr.mozilla.org/mozilla-central/source/js/src/gc/Heap.h#664-668
18:54jandembut i don&#39;t see where we add it to the &#39;global&#39; free list
18:55terrencejandem: the free list is stored in the zone
18:55terrencejandem: err, no...
18:56terrencejandem: when an arena is in the zone&#39;s free list it is set as *fully used*
18:56terrencejandem: so I guess we init it as empty and move it there later in allocate?
18:57joncolooks like it&#39;s in allocateFromArenaInner: https://dxr.mozilla.org/mozilla-central/source/js/src/gc/Allocator.cpp#356
18:57joncothe case with !hasFreeThings
18:57joncojandem ^
18:58jandemjonco: oh right, and we allocate the arena here: https://dxr.mozilla.org/mozilla-central/source/js/src/gc/Allocator.cpp#343
18:59joncojandem: yep
18:59jandemjonco: so we should definitely end up in the *FromArenaInner call after it
18:59jandemok so maybe we aren&#39;t merging the free list with the main Zone?
19:00terrencejandem: I missed the start of this conversation because my network was down... what is the sequence of events?
19:00jandemor no, we should have updated the arena that point probably
19:00terrencejandem: we AutoFinishGC... this does evictNursery, which allocates this page
19:01terrencejandem: this page has a *fully used* header and is loaded into the free lists of: what zone exactly?
19:01jandemterrence: &quot;we trigger a minor gc here, under the ZoneCellIter constructor: https://dxr.mozilla.org/mozilla-c...s/src/vm/HelperThreads.cpp#1116&quot;
19:01terrencejandem: I guess it must be the target zone in MergeCompartments?
19:01sfinkterrence: http://logs.glob.uno/?c=mozilla%23jsapi#c701623
19:01terrencejandem: thanks!
19:02terrenceokay, so that covers an object case... now let&#39;s see how the CellIter(ObjectGroup) is involved...
19:03terrencethat grabs a pointer to the parse tasks zone&#39;s arena lists
19:04terrenceAllocKind::OBJECT_GROUP is in BackgroundPhaseShapes, so I guess that it is background finalized, so the iter will wait on the background thread
19:05Waldohmm
19:05Waldothis code looks pre-existingly rotten
19:05terrenceWaldo: like I said above, it&#39;s igor&#39;s work
19:06terrenceWaldo: which we&#39;ve happily plastered over when building incremental sweeping and background parsing
19:06Waldoterrence: I&#39;m talking to myself, but okay :-)
19:06terrenceWaldo: ah!
19:07terrenceright... anyway... then we do a minor GC... again
19:07WaldoI don&#39;t believe igor was responsible for ObjectMayHaveExtraIndexedProperties, tho he might have been
19:07terrencethis second minor GC should be of zero size and return instantly
19:07sfinkjandem: what&#39;s the crash stack?
19:08terrencewe then check if the free list is already sychronized... who wants to bet this is where things get weird?
19:09terrencethis appears to be a heuristic check(!)
19:10terrenceit&#39;s basically checking if the inline free lists are &quot;empty&quot; or if the actual arena header is non-empty
19:10jandemjonco, terrence: so we are in allocateFromArenaInner, but in the !hasFreeThings case.. so in that case we leave the arenaheader&#39;s freespan empty and we add the free span to our Zone&#39;s list.. so then it&#39;s a matter of making sure this free list is merged right?
19:10sfinkthat sounds sensible to me
19:11jandemsfink: let me try to pastebin the stack
19:11terrencejandem: wait, the |initFinal| side of that branch?
19:11jandemterrence: yeah
19:11joncojandem: ArenaLists::adoptArenas calls fromArenaLists->purge() before merging the arenas which should do this
19:12joncojandem: this arena, is it the one that we end up crashing on?
19:12terrenceright, it&#39;s setting the free span as &quot;empty&quot;, so I guess that means Arena::isFullyUsed
19:12terrencealthough I have to admit that I&#39;m a bit lost now
19:12jandemjonco: yes, it never gets its free span set to anything other than empty
19:13joncojandem: ok, and it&#39;s an object group arena at this point?
19:13jandemjonco: no OBJECT0_BACKGROUND
19:13joncoah ok
19:13terrencejonco: I think the object&#39;s first pointer (the group) is just garbage, right?
19:14terrencebut wait... why would it be garbage if we&#39;re copying it over successfully here?
19:14joncoterrence: it sounds like it&#39;s a free cell but that the arenas is still set as fully used somehow
19:14terrenceso is the problem that the sweep iteration thinks the arena is fully used when it&#39;s not?
19:14terrencejonco: right
19:14terrencejonco: so I gather that the first cell *is* allocated
19:14terrencejonco: and we&#39;re crashing on the second cell, which is not actually used
19:15terrencejonco: but that the arena header says it is because our free lists got out of sync between the header and zone somehow
19:15jandemyes
19:15jandemjonco: i can check what adoptArenas does..
19:15terrencejandem: okay, now I&#39;m lost again... question!
19:15sfinkI&#39;m confused by purge()
19:16sfinkoh
19:16terrencejandem: so we&#39;re actually crashing in background finalization... this code waits on the background finalization thread, right?
19:16terrencejandem: or does it not?
19:16sfinkit&#39;s a later bg finalization
19:16terrencesfink: thanks!
19:16jandemterrence: yes i think it waits at the start and this finalization happens later
19:16joncojandem: can you see if we ever set the arena&#39;s firstFreeSpan after we allocate it?
19:16jandemjonco: we never set it
19:16terrenceokay, so that means that MergeCompartments must be corrupting the free lists somehow?
19:17jandemi set a watchpoint and it&#39;s 0 all the time
19:17joncojandem: wow, ok
19:17terrenceokay, or that :-)
19:17sfinkjandem: can you step through adoptArenas to the purge() call and see what it does?
19:17terrenceokay, so what is it&#39;s state after allocation?
19:18terrenceI guess that the arena is &quot;fully used&quot; and the zone&#39;s free lists reflect the allocation
19:18joncojandem: so can we see what the parse zone&#39;s freeLists has for this kind?
19:18terrence... then I presume that merge overwrites the allocaton in the zone&#39;s free list with the background zone&#39;s free list, yes?
19:18Waldomrgiggles: pun
19:18mrgigglesWaldo: Two robbers with clubs went golfing, but they didn&#39;t play the fairway.
19:19Waldobah
19:19sfinkyeah, we need the answer to jonco&#39;s question to confirm
19:19jimbho, that&#39;s pretty great
19:19terrencejonco: I think it just copies... under the assumption that we never allocate in the target zone... ever
19:19jimb9/10
19:19terrencejonco: that would also explain the crash I&#39;m seeing with UID&#39;s, so this may be huge
19:20jandemjonco, terrence: could that maybeStartBGAlloc be doing something here? https://dxr.mozilla.org/mozilla-central/source/js/src/gc/Allocator.cpp#337
19:20joncoterrence: we call purge on the source arena lists which should handle this
19:20jandemor is that harmless
19:20joncoin adoptArenas
19:20sfinkso what should happen is that the arena is marked as fully used, the zone&#39;s free lists should have the allocation, then adoptArenas sets the arena to reflect the single allocation and the free list is reset to empty
19:20terrenceyeah, looks like it&#39;s actually doing something complicated :-(
19:21* jandem tries to figure out what adoptArenas does
19:21jandemi mean, what it&#39;s actually doing
19:21Waldojimb: robbers don&#39;t &quot;play&quot; as an essential part of the concept of being a robber, so seems an ill fit to me as structured
19:21joncojandem: thanks for the remote debugging btw :)
19:23terrencejandem: AutoStartBackgroundAllocation should only touch the chunk lists... it doesn&#39;t even touch the arenas lists threaded through the chunk, I think
19:23jandemjonco: np. It&#39;s an opt build so stepping is a bit annoying to get right
19:24jandemterrence: hm ok so that one we can ignore
19:24terrencejandem: oh, wow... I would not have guessed this was an opt build by how fast your responses have been :-)
19:28terrenceokay, so purge looks sane... after that, our state should be: target zone with all arena headers having the correct free lists.... but I guess if we don&#39;t write to it again that that must be wrong?
19:29terrencejandem: right, so we should be setting the arena&#39;s firstFreeSpan in purgeArenas, right?
19:29sfinkyeah, the purge doesn&#39;t seem to be doing anything for some reason
19:29jandemyes indeed, hm everything in purge is optimized out so this will take a while
19:29jandemi can