mozilla :: #jsapi

20 Apr 2017
00:26Manishearthsfink: stil fails :|
00:26Manishearthhttps://treeherder.mozilla.org/#/jobs?repo=try&revision=c23a19850c69b06f752f259272a4ae5eaf78c7f1
00:26ManishearthI whitelisted memchr
00:26ManishearthError: External function
00:26ManishearthLocation: *memchr$void* memchr(void*, int32, uint64)
00:26ManishearthStack Trace:
00:26Manishearth_ZL9FindChar1PKcjiDsi$nsStringObsolete.cpp:int32 FindChar1(int8*, uint32, int32, uint16, int32) @ xpcom/string/nsStringObsolete.cpp#82 ### SafeArguments: aTrimTrailing
00:27Manishearthoh, should it have been the other one?
02:33Manishearthsfink: still doesn't work :|
02:33Manishearthhttps://treeherder.mozilla.org/#/jobs?repo=try&revision=ae1b331a6dbfe9cfa2ea8951691918b051fbbf4e
02:33Manishearthi added memchr to the whitelist
02:33ManishearthError: External function
02:33ManishearthLocation: *memchr$void* memchr(void*, int32, uint64)
09:32Yoricshu: With a few trivial optimizations, we're down to *1.56 (gzipped size, still).
09:33YoricMore precisely, *1.56 for Facebook, *0.67 for Devtools.
09:33Yoricdjvj: ^
10:26Yoricstandups: Down to *1.23 for Facebook, *0.54 for Devtools.
10:26standupsOk, submitted #44998 for https://www.standu.ps/user/Yoric/
12:29djvjYoric: nice.
13:18nbpstandups: JS start-up bytecode cache should land (disabled) within a few days. Still a tail of Encoding/Decoding bugs to fix, and some heuristics to tune before enabling it by default.
13:18standupsOk, submitted #45004 for https://www.standu.ps/user/nbp/
13:19nbph4writer: jandem: ^
13:19nbpnaveed: ^ (as well)
13:19jandemnbp: nice
13:41decodernbp: if you expose some kind of interface, then we can probably do binary fuzzing on the decoder
13:41decoderif that helps you
13:42nbpdecoder: You are already doing it, by fuzzing the JS test cases under the jit-test/tests/xdr directory.
13:42nbpdecoder: but I tihnk you might have black-listed the evalWithCache function, is that correct?
13:43decoderhm, no i havent. but im also not explicitly calling that
13:43decoderthere might be more direct ways to force this code to be used
13:43nbpdecoder: I would have expected fuzzers to find the regexp issue with the off-main-thread decoding sooner than by testing it by hand.
13:43decoderwhich issue is that?
13:44nbpdecoder: Bug 1351357
13:44firebothttps://bugzil.la/1351357 ASSIGNED, nicolas.b.pierron@mozilla.com OffMainThread XDR Decoding fails to decode RegExp
13:45nbpdecoder: I think I can make a JS shell test case.
13:46nbpdecoder: also the bits flowing into the decoder are necessary coming from the encoder. Altering these bits require having control over the content stored by the necko cache on disk.
13:46decodernbp: what about e10s? same situation?
13:46decoderassuming you control a tab
13:47decoderinside the sandbox
13:47decodernot sure where this xdr encoding/decoding is used everywhere
13:49nbpdecoder: encoding and decoding are done on the content process, so the parent process will save whatever flow in a stream and stream it back to the content process.
13:50nbpdecoder: But I don't think there is any way to protect us in such cases.
16:03shujandem: jonco: ping
16:03joncoshu: hey
16:04joncoshu: where's the meeting?
16:04shujonco: i see you joined my room but you can't seem to hear me
16:04jandemis there a meeting?
16:04shujonco: i'm face muted
16:05shujandem: if there's something to update about what you're currently working on
16:05joncoshu: ok I'm there but I can't hear you
16:05shuwhat the hell
16:06jandemshu: sorry forgot all about it :( Nothing important here, just more optimizations to various parts of the engine
16:07shujandem: okay that's fine then
16:14Yoricshu: djvj: This needs serious checking, but I believe I have managed to reduce the Facebook gzipped growth to *1.05 and the DevTools growth to * 0.47.
16:14shuYoric: oh wow
16:14Yoric(so effectively twice smaller for DevTools)
16:14shuYoric: how come devtools is so much smaller?
16:15Yoricshu: No idea, I haven't checked.
16:38Manishearthsfink: is it easier to set up that analysis on ubuntu? wiki indicates it's fiddly, but perhaps just this analysis will be easier
16:39sfinkManishearth: yes, the only difficulty I know of right now with ubuntu is with the gcc plugin, which shouldn't be a problem if you're using the pregenerated xdbs
16:40sfinkbut I guess I can't promise that compiling sixgill will be completely smooth. Others have done it successfully, though, so it might be fine.
16:40Manishearthheh
16:43sfinkjonco: ping
16:43joncosfink: hey
16:43sfinkjonco: I was hoping you could read bug 1353351 comment 3 (that I just now posted)
16:43firebotBug https://bugzil.la/1353351 is not accessible
16:44joncosure, looking
16:46joncough
16:46joncosfink: what exactly is a holder in this context?
16:47sfinkI assume it's an object that you're reading a slot from
16:47joncoo
16:48joncook
16:49sfinkare cross-compartment accesses really a problem (other than for security), if it's same-zone?
16:49sfinkwell, not accesses
16:49sfinkpointers, for tracing and sweeping
16:49nbpjorendorff: Wasn't CPG good for getting rid of iframes' JS?
16:50sfinkiframes have their own globals
16:50nbpsfink: exactly.
16:50sfink...so doesn't that mean they need their own code for everything?
16:50jorendorffI can't tell if you're joking
16:51sfinkI'm probably completely misunderstanding the question
16:51sfinkignore me
16:51nbpI am not joking. I thought that people were using iframe as a way to be able to reclaim memory.
16:51bzWhy would CPG affect that?
16:52nbphaving multiple globals in the same compartment will put us back in the case where we would have to trash the whole page, right?
16:52bzYou mean we'd have to GC the whole page to GC stuff in the iframe?
16:53jorendorffThe work I'm doing will only affect stuff that's same-zone already.
16:53* bz isn't sure whether the unit of G Cis zones or compartments
16:53sfinkzones
16:53sfinkbut it was once compartments
16:53bzjorendorff: Not quite
16:53bzsfink: Ah, hence my confusion
16:53jorendorffRight, GC was the reason for introducing zones.
16:53sfinkif we had that GC granularity gain at some point, it's gone already now
16:53nbpoh, sorry, just me not thinking through. So you would have to GC a single compartment with multiple globals, but if the global is not held by anything else it would be trashed as before.
16:53bzjorendorff: we'll need to change our zone boundaries a bit for your work
16:53jorendorff bz: Why?
16:53bznbp: Right
16:54nbpjust the set of things to iterate over might be larger.
16:54bzjorendorff: Consider the case of a page doing a window.open to a same-origin thing
16:54jorendorffSure
16:54bzjorendorff: Right now those are two separate zones
16:54bzjorendorff: becuase they're separate tabs
16:54bzjorendorff: We'll want them to be same-compartment, though, right?
16:54jorendorffbz: I was not planning to introduce an iron-clad rule that same-origin same-tab-group realms are necessarily same-compartment
16:55bzAh
16:55bzSo we'd still have transparent CCWs in some cases?
16:55nbpjorendorff: shouldn't the opposite rule be enforced?
16:55jorendorffnbp: what rule?
16:55nbpjorendorff: that different-origin get different compartments.
16:56jorendorffbz: We definitely have CCWs in some cases anyway, for document.domain and (I think) some sandboxes (chrome-to-chrome)
16:56jorendorffI don't know the cases very well :-|
16:56jorendorffIf nothing else, JS shell debugger tests want transparent CCWs :-)
16:56bzok
16:56bzI mean, yes, we will still have them for sure; the question is whether they will be web-exposed
16:57jorendorffYes
16:57bzI guess if we can preserve the flexibility of deciding where our zone boundaries are that's good
16:57bzbecause we may want to tune that for GC purposes....
16:57bzHere's my concern
16:57bzSay we do the window.open thing
16:58bzSame-origin
16:58bzAnd then one of the pages sets document.domain
16:58bzNow access to that window we opened should be denied, right?
16:58jorendorffnbp: Definitely.
16:58bz(per spec)
16:59jorendorffbz: by the Window and Location objects, yes.
16:59bzok
16:59bzhow do we implement that denial?
16:59bzConcretely, say the page does openedWindow.foo
16:59bzWe land in the [[Get]] hook of "openedWindow"
16:59bzWhat's the current global and why?
17:00jorendorffbz: Are we talking about the case where the opened window is same-compartment or different?
17:01bzdifferent
17:01bzif it's same-compartment it's all clear
17:02jorendorffbz: Well, then this is very similar to the case where the opened window starts out as different-origin, but both pages set document.domain to be same-origin, and then the opened window sets document.domain once again, right?
17:02jorendorffThat's a case that we'll have anyway, where we have access to a Window object across compartments and need to restrict access.
17:03joncosfink: what happens if the target compartment gets nuked? direct pointers from other compartments will not be affected (this may not be a big problem; I don't know whether we ever nuke a single compartment in a zone)
17:03bzjorendorff: right; my point is that we need a CCW that does NOT enter the target's global here
17:03bzjorendorff: Or something
17:04jorendorffbz: So... I had a dumb idea about this, which I tried to mention two or three times on Vidyo, but never got to
17:04bzjorendorff: I guess the point is that this is just what we will use for a non-Xray CCW to Location and Window
17:04sfinkjonco: I hate you
17:05bzjorendoff: regardless of origins. So it works, great. ;)
17:05sfinkjonco: but I think it's ok in this case?
17:05jonco:)
17:05sfinkoh wait
17:05jorendorffjonco: same thing will happen as now, right? doesn't nuking just cut edges?
17:06jorendorffand only edges from chrome to content it doesn't necessarily make the target compartment completely unreachable even todayright?
17:06sfinkyeah, but we use those edges for stuff, and if we hang onto a pointer into the target compartment, we may mess up sweeping
17:06sfinksorry, that was cryptic
17:06jorendorffbz: which is: can we just make Window and Location always be proxies and never CCWs? It seems like we very much need non-entering behavior
17:06jorendorffbz: and dynamic checks
17:06jorendorffbz: they don't seem much like CCWs, then, honestly
17:07jorendorffbz: (I mean that even cross-compartment access might be implemented with a regular proxy, not a CCW)
17:07sfinkjorendorff: this is a "special" cross-compartment edge, one that is not registered in the wrapper map. And the wrapper map is used to generate a sweep ordering.
17:07jorendorffohhh
17:08jorendorffsfink: I didn't realize you were talking about that bug, ignore me
17:08bzjorendorff: hmm
17:08jorendorffbz: Ideally, it could be exactly the same proxy as "home-compartment" access, if the same behavior is needed.
17:08joncosfink: I think it's probably fine, still looking at the code...
17:08sfinkjonco: don't we get the right sweep ordering in this case because we know we have the global wrapper, and when we nuke, we... er... wait, hold on
17:08joncosfink: if they're same zone then it doesn't matter
17:09sfinkoh, right, duh. I just said that, didn't I?
17:09bzjorendorff: I guess as long as we exempted this special proxy from compartment asserts or something...
17:10Manishearthsfink: I have a function that's being called when an "is gecko" parameter is true. It's non thread safe. We pass false in the stylo bindings. How do I convince the analysis to ignore this call?
17:10bzjorendorff: I mean, the main reason transparent CCWs exist is to make compartment asserts happy, right?
17:10ManishearthAdd a whitelisted edge?
17:10Manishearthor is there a way to use an assert to convince it?
17:10sfinkactually, I don't think it would matter even if it were different zone in this case. We know we have the global wrapper in the same jitcode, and the findDeadProxyZoneEdges stuff I added would fix the sweep ordering.
17:10sfinknot that it matters
17:10jorendorffbz: no, not exempt. It's like a wrapper in that WrapperFactory can return it, and it's cached in the wrapper weakmaps
17:11jorendorffbz: Only the implementation (the ProxyHandler) has more in common with an ordinary Window than a CCW
17:11sfinkManishearth: sorry, I don't follow. What's the relationship between "is gecko" and the thread safety?
17:11bzOh, I see
17:11bzwith a WindowProxy, not Window
17:12bzBut in the case of Location it would actually be more like a Location, sure
17:12Manishearthsfink: in "is stylo" mode we call a thread safe function. in "is gecko" mode, we call a function that does heap writes
17:12sfinkoh, I see
17:12Manishearthsfink: also, this is still complaining about memchr
17:12Manishearthhttps://treeherder.mozilla.org/#/jobs?repo=try&revision=ae1b331a6dbfe9cfa2ea8951691918b051fbbf4e
17:12Manishearth(this is a different one)
17:13bzjorendorff: Apart from being "less like what we do now", I'm not seeing obvious problems with it yet
17:13ManishearthError: External function
17:13ManishearthLocation: *memchr$void* memchr(void*, int32, uint64)
17:13ManishearthStack Trace:
17:13jorendorffbz: it's just a matter of what is simpler to implement ... bholley did say that the dynamic checks in the spec are basically just what we do now, right? which would argue for keeping CCWs
17:14jorendorffbz: GC and nuking might also require a real CCW there
17:15bzjorendorff: right; I haven't had a chance to think through all the edge cases...
17:15* bz can put in the time to do that if desired; would need a clearer idea of the proposed setup.
17:15sfinkManishearth: can you guard the heap write function is if (!IsInServoTraversal())?
17:15jorendorffbz: another thing we haven't talked about is dividing the work. I'm going to focus on filing blockers today and thinking about what can be done in parallel
17:15sfinks/is/with/
17:15jorendorffbz: but I'm off on vacation all next week
17:16bzjorendorff: OK
17:16Manishearthsfink: can I MOZ_ASSERT instead?
17:17sfinkI don't think so
17:17sfinkum
17:17Manishearthaw
17:18sfinkManishearth: I'll get back to that, but with respect to memchr -- did we add that to whitelist, or simpleWrites?
17:18sfinkI think I may have told you to put it in simpleWrites
17:18sfinkit should be in whitelist, since it's just a read
17:18sfinksame for strlen, for that matter
17:18sfinkoops
17:19Manishearthsfink: I tried both, neither worked iirc
17:19Manishearththis one is the whitelist
17:20joncosfink: I don't understand cache IR well enough to know what's going on here
17:20sfinklooks like it's C++ mangled. I suspect you'll need to do /memchr/
17:20sfinkinstead of "memchr"
17:21joncosfink: does this optimisation only apply to a single object? if so I don't see why we store the compartment rather than just comparing the object pointer directly
17:21Manishearthsfink: oh, thanks
17:22sfinkuh, hm. There's the holder, and the thing read out. One is dynamic. Let me look.
17:23sfinkok, now I'm feeling like I don't understand this well enough
17:23sfinkit takes in an object and (dynamically) unwraps it
17:23nbpOne day I should investigate why SpiderMonkey 17.0 is a dependency of the toolchain needed to build Firefox.
17:24sfinkoh, wait. Maybe EmitReadSlot{Result,Return} don't store unwrapped...
17:24joncosfink: yeah I wasn't sure. it's conditional on several things
17:26sfinkit can definitely store unwrapped->group_
17:27sfinkjonco: so I"m just making stuff up here, but it looks to me like the optimization is meant to apply to multiple objects. Those with the same ObjectGroup, for example, in some cases.
17:28joncosfink: that was my impression too
17:28joncosfink: so I don't know why we store a specific object - but we obviously do because this bug happened
17:28sfinkthe bug is from holder, not unwrapped
17:29joncowait, what is holder again?
17:29* sfink tries to figure out what holder is again
17:29sfinkyeah
17:29nbpfirefox-build-env > libgnome > gconf > polkit > spidermonkey-17.0.0
17:29sfinknbp: ptomato-M is working on upgrading libgnome, I think?
17:30sfink(or updating its JSAPI version, I mean)
17:30ptomato-Msfink: nbp: not sure what the context is, but unfortunately polkit is a separate embedding from what I'm upgrading
17:30nbpsfink: as long as they don't use bleeding-edge versions of SpiderMonkey, the Linux community should be fine and we should still have a way to bootstrap a distro.
17:31sfinkbut we've managed to mangle the source package. Probably my fault. I need to fix it anyway.
17:31sfinkptomato-M: oh! Good to know, thanks.
17:31ptomato-MI understood they were upgrading too, though
17:31ptomato-Mwhat's the issue?
17:33sfinkptomato-M: I think nbp was just curious why the firefox build depends on spidermonkey-17 :)
17:33nbpptomato-M: no, it is just unexpected that we depend on spidermonkey for generating a build environment for firefox.
17:33sfinkjonco: so I *think* holder is the object that a property is actually found on, possibly something from the actual object's prototype chain
17:33ptomato-Myes, that does seem odd :-)
17:33ptomato-MI don't know who's maintaining polkit but if they need help upgrading you can tell them to reach out to me
17:34joncosfink: oh I see
17:35sfinkjonco: and the comment says we're guarding on the holder's *shape*. But then why oh why are we writing in the holder??!
17:35joncosfink: I think we need to discuss this with evilpie
17:35sfinkoh. That's only for unboxed objects. But the test case... ok, let me step through this some more
17:36sfinkjonco: specifics aside, it makes sense for me to be guarding on "stuff" in the target compartment
17:37joncosfink: sure
17:37sfinkoh, bleh. My source has changed, which is why the specific write is happening in a weird place. Lemme recompile.
17:38sfinkeven without that, I see it, though
17:39sfinkthe "Guard on the holder's shape." comment is immediately followed by writer.loadObject(holder), which calls addStubField with uintptr_t(obj)
17:39sfinkevilpie: ping
17:39jandemwhat's the question?
17:40sfinkjandem: we have a crash in evilpie's cross-compartment CacheIR IC thing
17:40sfinkbecause EmitReadSlotResult(writer, unwrapped, holder, shape, unwrappedId) writes holder into the jitcode
17:41sfinkand holder is in the unwrapped target compartment
17:41sfinksorry, I should say s/crash/assertion failure/
17:41evilpieI am in a movie
17:41sfinkso when we scan the heap for cross-compartment edges, we find it
17:41sfinkevilpie: as an actor? :-) It's ok, we can talk later. Or jandem will fix.
17:42sfinkjandem: so one option might be to figure out how to weaken this cross-compartment assert (to a cross-zone assert)
17:42jandemso yeah, holder is the object that has the property we're interested in
17:42sfinkthough I'm a little curious about why guarding on holder's shape results in holder itself being written into the jitcode
17:42jonco"evilpie: the movie"
17:43sfink"you'll never look at a pie in the same way again"
17:43jandemsfink: is this Ion? baseline writes holder in the stub, Ion bakes it in the code
17:43nbpsfink: Good, they are open for changes: https://cgit.freedesktop.org/polkit/tree/docs/man/polkit.xml#n500
17:44sfinkjandem: yes, I think this must be ion. It repros with --ion-eager
17:44jandemsfink: we load holder into a register, then we check its shape
17:44jandemsfink: and holder is a constant in Ion
17:44jandemsfink: so we use ImmGCPtr
17:44sfinkjandem: er, it repros with --no-ion --baseline-eager too
17:46jandemsfink: ok. But yeah the stub/code will contain a JSObject* holder that's in the target compartment
17:48sfinkoh, I think I see. The guard is for "any object with this shape", but in this case, the object in question is a constant
17:48sfinkbut I suppose you still have to make sure its shape is the same
17:48tcampbelljandem: have 15 minutes to chat some time today?
17:52joncocouldn't we just store the shape?
17:52jandemsfink: when a property is found on the prototype, we bake in the specific prototype object's shape (because we know, from other guards, it must be that object)
17:53jandemmaybe we could change the CCW case to load the proto dynamically so we don't have to bake in the JSObject*
17:53jandembut not sure how easy that is
17:53jandem(in terms of code changes)
17:54jandemshould discuss with Tom
17:54jandemtcampbell: i can irc a bit now but it's getting late here
17:56sfinkjandem: what would you think about adding a StubField::Type for this? So StubField::Type::SameZoneObject or something, that behaves identically to StubField::Type::JSObject, except somehow it magically weakens the cross-compartment assertion to a same-zone assertion?
17:56sfink(I'm not actually sure how to communicate which assertion to use)
17:57jandemsfink: yes that makes sense to me, but there's also the ImmGCPtr case in Ion JIT code
17:58sfinkoh, ok. I suspect I've only been stepping through a baseline case so far. I should probably figure out how to distinguish them.
18:26Manishearthsfink: quick r? https://reviewboard.mozilla.org/r/130118/diff/8#index_header
18:26Manishearthit finally passes!
18:26Manishearth:D
18:26Manishearth(one down, 9 to go)
18:27djvjnbp: ping
18:27nbpdjvj: pong
18:27djvjnbp: hey, how is the bytecode cache work going? I heard it was close to landing, mostly waitin for reviews.
18:28nbpdjvj: I got the review, if try is green tomorrow, I will land it.
18:28nbpdjvj: but disabled.
18:28djvjnice. I'm assuming disabled because perf improvements are mixed?
18:28nbpdjvj: I have to investigate XDR issues, and tune it to get better perf on benchmarks.
18:28djvjnbp: which benchmarks are you using?
18:30sfinkManishearth: I don't see why the analysis change is needed? It looks like you now have a nice MOZ_ASSERT(NS_IsMainThread()) in InitiSystemMetrics before any heap writes, so shouldn't it just work without the annotation?
18:30nbpdjvj: for the moment tp5n, but I discussed with someone else (can't recall the name, somewhere in my emails) about other benchmarks, and possibilities for getting results as we get closer to having something shippable.
18:31nbpdjvj: and various things to explore. (expected queing latency), page load testing benchmark from necko,
18:31sfinkManishearth: oh. You know what I said that I didn't think MOZ_ASSERT would work? I'm pretty sure I lied. Looking again, I think that should be fine (for either NS_IsMainThread or !IsInServoTraversal)
18:31sfinks/what/when/
18:31djvjnbp: testing against our facebook static page dumps would be a _really_ good idea.
18:31nbpdjvj: and I have to add telemtry probes as well.
18:32nbp djvj: tp5n already includes facebook, but I cannot run all the tests from it yet, I have to comment a few lines because of XDR failures.
18:32djvjnbp: like, testing real-world page-load against static dumps from our major target top-5 sites is basically 90% of the value we're going for.
18:33Manishearthsfink: hm, will try
18:33djvjnbp: do you have a bugno for this work?
18:33nbpdjvj: tp5n is the top 50, I think.
18:33nbpdjvj: Bug 900784
18:33firebothttps://bugzil.la/900784 ASSIGNED, nicolas.b.pierron@mozilla.com [meta] Add start-up cache for any JavaScript code.
18:33Manishearthsfink: i may have done a try push where the assert didn't work, not sure
18:33* nbp sending it to try, and going back home.
18:34sfinkManishearth: by the way, I tried compiling sixgill on osx. It fails horribly, but it turns out to be easy to compile just the part you need. I'm going to see if I can get the analysis to run using it, now. I'll update the instructions if I get it to work.
18:35Manishearththanks!
18:36* nbp wait on mercurial commands to wake up
18:43nbpdjvj: Tomorrow I shall have the answer if I can land it or not ;)
18:46djvjnbp: do you know what version of the facebook site tp5 contains?
18:46djvjnbp: cause it might be _old_.
18:46djvjlike, decade old.
19:01shudjvj: nbp: we have the workload that vladan sent us; you know about that, right?
19:28anbashu: Maybe this is a dumb question, but do we only abort syntax parsing for binding patterns because we first parse the pattern as an array/object literal? So if we add new methods to parse binding patterns instead of going through the general array/object literal parsing code, we'd be able to syntax parse binding patterns?
19:31shuanba: that is correct, nobody has bothered to rewrite destructuring yet
19:32anbashu: well, i just did.
19:32shuanba: oh sweet
19:32shuanba: you're no longer rewalking the literal to do binding?
19:32shuanba: i am excited to review
19:33anbashu: yes, but only for binding patterns. Destructuring assignment still aborts syntax parsing
19:33shuanba: ah okay
19:34anbashu: Destructuring assignment is a bit more complicated, I hope we can use the existing PossibleError class to make it syntax-parseable
19:40Manishearthsfink: /memchr/ doesn't work either
19:40Manishearthhttps://treeherder.mozilla.org/#/jobs?repo=try&revision=32dfcc0d5367e05b14e273f04c3127987cca9ffd
19:44sfinkManishearth: uh, that shouldn't be in parentheses
19:44sfinkquotes
19:44sfinkI meant quotes
19:44Manisheartheh
19:44sfinkI didn't say parentheses
19:45sfinkthat wouldn't make any sense
19:45Manishearthheh
19:45Manishearththanks
19:46sfinksomeday, this osx machine will finish cloning mozilla-unified and I'll be able to try things locally
19:46Manishearthlol
19:46sfinkon the 3rd attempt now
19:46Manishearthmozilla-unified still exists?
19:47Manishearthoh this is different
19:47ManishearthI think I confused it with comm-central
19:47sfinkoh, yeah
20:12shuanba: what's the idea of using PossibleError?
20:12anbashu: in general or for destructuring assingment?
20:12shuanba: for destructuring assignment
20:14tcampbellwhen people refer to "speedometer v2", what is the link for that?
20:14djvjshu: yeah, that's the one that I was referring to. Pinged harald on the bug and asked him about it.
20:15anbashu: we'd need to mark any invalid assignment destructuring syntax as a possible error (we already do this, but possible not for all cases), and then we can avoid calling the checkDestructuringXXX methods.
20:15shuanba: ah okay
20:24shuYoric: quick review ping
20:24Yoricshu: incoming
20:27Yoricshu: Sorry for the delay. Too many things on my mind + splinter hates me.
20:28shuYoric: no worries, wanted to land it because it's a fuzzblocker
20:28Yoric(it keeps eating my reviews, for some reason)
20:28shuYoric: thanks
20:47Waldonobody look now, but when there are syntax errors in the contents of a regular expression, our error message/context information is pretty bad
21:02Manishearthsfink: progress! /memchr/ worked, but it doesn't like strlen, so i moved it
21:02Manishearthlets see what this run gets us
21:02sfinkugh. Sorry for the awful turnaround time. I managed to clone, but it's still working on getting a current clang.
21:03sfinkthis is an ancient 10.6 box
21:03Manishearthno problem
21:03sfinkand ./mach bootstrap fails. Can't get watchman, can't get rust.
21:05Manishearthheh
21:15Manishearthsfink: for running it on ubuntu, do I need to do the xdbs step, or is it possible to generate that locally?
21:18sfinkthat's the hard one. It would be easier, at least to start, to download those from try.
21:19sfink(that won't allow you to make c++ changes)
21:24Manishearthcool
21:28Manishearthsfink:
21:28Manishearth[26.85s] #92 Analyzing Gecko_SetNullImageValue ...
21:28ManishearthError: AddRef/Release on nsISupports
21:28ManishearthLocation: _ZN12nsStyleImage7SetNullEv$void nsStyleImage::SetNull() @ layout/style/nsStyleStruct.cpp#2222 ### SafeArguments: <this>
21:28crowbotIssue #92: FontCache should cache font table blobs for harfbuzz - https://github.com/servo/servo/issues/92
21:29Manishearththis is a current error
21:29Manishearthsfink: this is a release on nsIAtom
21:29sfinkManishearth: btw, you shouldn&#39;t have to run any steps except for heapwrites
21:29Manishearthwe&#39;ve already whitelisted that, so I&#39;m not sure what&#39;s happening here
21:30sfinkManishearth: whitelisted aImage, you mean?
21:31Manishearthsfink: whitelisted nsIAtom in hasThreadsafeReferenceCounts
21:31sfinkoh, ok
21:33ManishearthNS_RELEASE(atom) should work
21:33sfinkyeah, I&#39;m trying to understand the refcount handling
21:35sfinkhasThreadsafeReferenceCounts only seems to apply for the patterns listed in ccheckOverridableVirtualCall (eg nsCOMPtr<T>::assign_with_AddRef...)
21:35Manishearthyes
21:35ManishearthI want NS_RELEASE on an nsIAtom to always be ignored
21:35Manishearthit&#39;s safe
21:36Manishearthwell, ->Release
21:37Manishearthoh I think I understand the issue
21:37Manishearthwe&#39;re calling nsIAtom->Release() there
21:37Manishearththat&#39;s just nsISupports->Release() from the POV of the analysis
21:37Manishearthand the type is erased
21:40sfinkmy if I&#39;m reading the json nesting correctly, it *looks* like it knows it&#39;s an nsIAtom
21:41Manishearthhm
21:41Manishearthwhich json?
21:41sfinkoutput of: $SG/xdbfind -json src_body.xdb &#39;_ZN12nsStyleImage7SetNullEv$void nsStyleImage::SetNull()&#39;
21:41sfinkwith $SG set to the sixgill bin dir
21:41sfinkbut it&#39;s really hard to read
21:42Manishearthah
21:52sfinkManishearth: I have to run for a bit, but I *think* this is fixable. The information is there, it&#39;s just getting discarded. I wish I understood the data model a little better, but I think I can figure it out.
21:52Manishearthsfink: thanks
21:52Manishearthsfink: is there any way to speed up the try runs? get them to run only that analysis? or is the pre-analysis step the slow one?
21:53sfinkthe pre-analysis step is the vast majority of the time
21:53sfinkthe heap write analysis is about 30 seconds, the rooting analysis that you don&#39;t need is about 6 minutes or so, and the build is the rest.
21:54sfink(the instrumented build, that is)
21:54Manishearthah
21:57jimbbillm: This comment is out of date, right? There are no more downcasts from JSRuntime to JSContext.
21:57jimbhttp://searchfox.org/mozilla-central/rev/7aa21f3b531ddee90a353215bd86e97d6974e25b/js/src/vm/Runtime.h#912-913
21:58billmjimb: yes
22:27evilpieshu: bug 1357483
22:27firebothttps://bugzil.la/1357483 NEW, nobody@mozilla.org Function#toString on a class prints more than it should
22:27evilpiesfink: lol ....
22:28shuevilpie: thanks
22:35Waldowow, cloning inbound is obscenely fast with the bundling change these days
22:35WaldoI should clone inbound more often
22:59jdmthat is definitely the takeaway here
22:59jdmdon&#39;t pull changes, just rm the old one and clone a new one
23:01shufor that fresh repo smell
23:26jimbshu: git gc?
23:27shujimb: Waldo was saying he should clone inbound more often because it&#39;s fast now
23:27jimbI found a bunch of moldy changesets in my fridge last night.
23:28Waldowith my strategy of having one inbound clone locally that I never modify, then a bunch of local clones of that that I work in, I haven&#39;t cloned inbound in a long time
23:28Waldobut when you get repo corruption back to cset 0, it&#39;s time
23:28jimb... I see
23:28Waldocouple too many force-kills or something
23:28* jimb is apparently very lucky
23:29jimbMy computers generally work. I generally get prompt notifications from my phone. My repos generally don&#39;t get damaged. etc.
23:29jimbFortune smiles upon me.
23:29jimbEven my video drivers work.
23:30shuWaldo: why do we change the end position of a parenthesized expr pn?
23:30shuWaldo: error reporting?
23:31Waldoum
23:31Waldoshu: maybe
23:31Waldoshu: note that I think I have a change to how that works locally that may smooth out some of the oddness to that
23:31* Waldo needs to finish this rebase and land stuff
23:31shudo we need to change it?
23:32shui&#39;m recording offsets for class exprs on the ClassNode itself for toString, and because parens change it, you get stuff like &quot;class {})&quot;
23:32shulol
23:33WaldoI remember stumbling across that recently
23:34WaldoI think we might not need to, but I don&#39;t remember what my patching locally actually does in enough detail to say
23:35shui&#39;m just going to delete that line and see what happens
23:36Waldothis toStringStart change is making for fun rebasing
23:36Waldojust &#39;cause I&#39;m touching so much in Parser.cpp overall
23:37shuwhy are you even getting conflicts with that rename
23:38Waldobecause I added a template parameter to Parser, so every single Parser function definition pretty much changes?
23:38Waldoincluding the ones with toStringStart in them
23:40shubummer
23:40shuthere weren&#39;t that many i thought
23:40shunot in Parser anyways, there were a bunch in JSScript and LazyScript
23:42WaldoI haven&#39;t needed to touch either of those yet
23:53Waldofeels like I probably never should, but who knows
21 Apr 2017
No messages
   
Last message: 8 days and 2 hours ago