mozilla :: #jsapi

8 Sep 2017
00:36jorendorfftcampbell: so about the word "global" for this
00:36jorendorfftcampbell: I don't suppose you're still around
00:36tcampbellstill here
00:37jorendorfftcampbell: Do you think "global" is the best word? the whole point of the new apis is, there are lots of them with in a single global. right?
00:37tcampbelljorendorff: if you have a word to describe union of NSVO and GlobalObject, I'd be happy to change
00:37tcampbellwell, that isn't quite right.. hmm
00:38jorendorffit's global-like in that it has the "lexical tier"
00:39jorendorffthe problem is we already have so many different kinds of environment :-P
00:39tcampbellin my head, I was inventing "NonSyntacticGlobal" to represent a thing that pretended to be the Realm.[[GlobalEnv]]
00:39jorendorff...hmm
00:39tcampbellbut shared some of its bindings with other
00:40jorendorfftcampbell: ok, that's freaking me out a little, it's not how I thought of it :D
00:41jorendorffbut it makes sense
00:41tcampbelljorendorff: clearly needs more descriptions
00:42jorendorffEither that, or the problem is that we're creating special-case APIs
00:42jorendorffwhen there is some very simple JS::NewEnvironment(scads of options) API that we haven't figured out
00:43jorendorffbut I don't want to ask you to take more time than you already have, nor am I confident that exposing the full cartesian product of all possible options is wise
00:43jorendorff(in a sense, that's how we got here)
00:43tcampbellthis was very much a special-case API. It is not for other consumers yet
00:43tcampbellbut you are right, it may be good to clean up in near term
00:44jorendorffOK, since it is a special-case API, how about a special-case name
00:44tcampbell.. how sensible
00:47jorendorfftcampbell: r=me with "js::NewJSMEnvironment" or words to that effect
00:47jorendorffstamped
00:47jorendorffgood night! :)
00:47tcampbellwoo! thanks :)
07:04mrgigglesthe mozilla-inbound tree is now closed (bustages and test failures)
07:29mrgigglesthe mozilla-inbound tree is now open
10:07* jandem just had to test FF 55, weird how outdated it feels already
10:07jandemvery happy with the default photon theme
10:07jandemon mac at least
11:48smaugon linux about:preferences look really weird
12:04smaugemilio: NoteDescendantsNeedFramesForServo is 3.4% of innerHTML set time
12:04smaugLooks like NoteDirtyElement part of it
12:05smaugoh, even more
12:05smaugNoteDirtyElement is 4.6%
12:11emiliosmaug: hmmm...
12:12emiliosmaug: I wonder if it's triggering a pretty bad case of it. Is it calling it from bottom to top of the HTML?
12:12smaugoh, wrong channel :)
12:13smaugemilio: ContentAppended->CSSFC::MaybeConstructLazily->NodeDescendantsNeedFramesForServo
12:13smaugthat is that the profiler says
12:13emiliosmaug: hmm... Yeah, that makes sense, but I wonder about the order of those ContentAppended calls
12:14emiliosmaug: I can try to take a look, there were some innerHTML test-cases floating around
12:14smaugemilio: see the bug, and the bug it blocks
12:14smaugCC you to the bug
12:14smaugCCed
12:15emiliosmaug: k, thanks
12:23nbpsewardj_: I have a crash under DoLULBacktrace, recorded in rr.
12:23sewardj_nbp: oh yes?
12:24nbpsewardj_: to be precise on the memcpy.
13:40RyanVMjonco: wow, nice catch
13:40RyanVMguess that was a footgun waiting to burn us :)
13:41joncoyeah :) thanks
13:42RyanVMi'll be running it through Try shortly
13:50tcampbellhmm.. anyone know why this extra |with({}){}| causes the code after it to behave different? https://pastebin.mozilla.org/9031856
13:50tcampbellI feel I'm missing some corner of the spec
13:55anbatcampbell: Add a semi-colon after |hello()| ;-)
13:55jandemtcampbell: automatic semicolon insertion
13:56jandembah too late
13:57joncowhat does that evaluate as if the semicolon is missing?
13:57mrgigglesthe mozilla-inbound tree is now closed (leaks on Windows from bug 1365970)
13:57firebothttps://bugzil.la/1365970 ASSIGNED, wiwang@mozilla.com Move sessionrestore data collector timer in the content process to idle dispatch
13:59jandemjonco: without the with, it's like hello()(function() {})() or something
14:01joncojandem: right, but what does it result in "hello() is not a function"? if I do one expression followed by another I get "missing ; before statement"
14:05jandemjonco: I think it tries to call the result of hello(), which is undefined
14:05jandemthen we try to decompile where that value came from
14:05jandemand that's the hello() call
14:05jandems/result/return value/
14:06mrgigglesthe mozilla-inbound tree is now open
14:06zombiesubtle difference between "hello is not a function" and "hello() is not a function" has bitten people before
14:08joncoaha
14:09joncocheers
14:09zombiethat message could probably be clarified "result of hello() is not a function"
14:18tcampbellweird.. that makes sense I guess
15:00tcampbellthe gecko subscript loader has such a hacky environment =\
15:23tcampbellstandups: Cleaned up GetThisValue/ComputeImplicitThis code (Bug 1397385)
15:23standupsOk, submitted #50687 for https://www.standu.ps/user/tcampbell/
15:23firebothttps://bugzil.la/1397385 ASSIGNED, tcampbell@mozilla.com ComputeImplicitThis leaks VarEnvironmentObjects to script
15:42djvjstandups: wrote probatory fixes for bug 1375669 and bug 1376717
15:42firebothttps://bugzil.la/1375669 NEW, kvijayan Crash in js::jit::JitcodeGlobalEntry::callStackAtAddr const
15:42firebothttps://bugzil.la/1376717 NEW, kvijayan Crash in js::jit::JitcodeGlobalEntry::callStackAtAddr
15:44djvjstandups doesn't like me :(
15:44tcampbelldid you register with the bot?
15:44tcampbellotherwise check with #standups
15:44djvjI signed in and it has my profile..
15:44djvjmaybe because I wasn't registered with NICKSERV?
15:45tcampbellMy account needed manual database fixes at one point
15:45djvjstandups: wrote probatory fixes for bug 1375669 and bug 1376717
15:46standupsOk, submitted #50689 for https://www.standu.ps/user/djvj/
15:46standupsOk, submitted #50690 for https://www.standu.ps/user/djvj/
15:47tcampbelloh, just slow
15:47djvjwow
15:48nbpI was expecting to see "doesn't like me" as one of the submitted messages.
15:49tcampbellhaha
15:51jorendorffbz: ping re multiple-realms-per-compartment
16:08evilpiejonco++
16:09joncoevilpie: thanks
16:12evilpiejonco: did you run the six-speed benchmarks?
16:14joncoevilpie: no I couldn't get it to run locally
16:15jonco(I start the local server but just get 404s connecting to it)
16:15evilpiesomething like this https://pastebin.mozilla.org/9031869
16:22joncoevilpie: 8.91 s before, 7.86 s after
16:22evilpie\o/
16:23evilpiebut we need to optimize Map/Set operations more
16:23joncowe should be able to skip postbarriers as well for nursery-allocated maps/sets but it's kind of complicated in the interpreter
16:23joncoI'm not sure it's worth it if we're going to JIT those operations
16:25joncoactually that may not affect this benchmark too much though, it's mainly using integer keys
16:39jorendorffYoric: Another thing I probably didn't mention
16:40Yoricjorendorff: Yes?
16:40jorendorffI don't know how far along you are currently, but it might be good to use FullParseHandler to create parse nodes
16:40jorendorffinstead of calling into ParseNode.h stuff directly
16:41YoricMmmh... ok, that's probably feasible.
16:41jorendorffIt would share more code with the source Parser.
16:41jorendorffTo the extent that the FullParseHandler actually does anything
16:41jorendorffYoric: Maybe more importantly: when people change the Parser they will be forced to update BinAST
16:41jorendorffat least, sometimes :)
16:43jorendorffAnyway ParseHandler is supposed to be an interface around AST generation, that's its job, might as well use it
16:45Yoricjorendorff: I have finished updating the tokenizer and tests, I now need to propagate to the parser. Might take me a few hours :)
16:48jorendorffmrbkap: In this patch you reviewed, I wrote:
16:48jorendorff RealmPrivate* realmPriv = new RealmPrivate(realm);
16:48jorendorff realmPriv->scope = this;
16:48jorendorffThis is OK, right? we crash on oom in js/xpconnect?
16:48jorendorffof course we do
16:48jorendorffyeah, of cousre
16:48jorendorffunping
16:57nbpstandups: Bug 1396822: Experiments with branch removals within IonBuilder; Bug 1398137: Found rr issue when turning on the Gecko Profiler; Bug 1395233: Back to figuring out what is going on with the JSBC memory.
16:57standupsOk, submitted #50693 for https://www.standu.ps/user/nbp/
16:57firebothttps://bugzil.la/1396822 NEW, nicolas.b.pierron Remove dead branches from IonBuilder.
16:57firebothttps://bugzil.la/1398137 INVALID Crash under memcpy | DoLULBacktrace
16:57firebothttps://bugzil.la/1395233 ASSIGNED, nicolas.b.pierron JSBC: 3% Heap Unclassified summary windows10-64
16:58tcampbellnbp: that is a known rr issue
16:58tcampbelllet me find bug
16:58tcampbellnbp: https://github.com/mozilla/rr/issues/1930
16:58crowbotIssue #1930: Recording Firefox with Gecko Profiler causes crash - https://github.com/mozilla/rr/issues/1930
16:59nbpstandups: Bug 1398105: crash-stat found it before the fuzzers?! Time to debug now
16:59standupsOk, submitted #50694 for https://www.standu.ps/user/nbp/
16:59firebothttps://bugzil.la/1398105 NEW, nobody@mozilla.org Crash in js::jit::LRecoverInfo::OperandIter::settle
16:59nbptcampbell: Thanks, I will comment in the bug.
17:00tcampbellit got me when I was trying to deal with netflix IonCompile
17:01tcampbellnbp: the other problem I hit was that using rr will change the Ion heuristics
17:01tcampbellsince ncpu is set to 1
17:02nbptcampbell: if you give it the -h option, it will no longer change the ncpu.
17:03nbptcampbell: rr record -h, will give it an emulated number of CPUs.
17:03tcampbellooh.. good to know
17:05tcampbelloh right, you mentioned that in review when I last asked about this
17:06nbp-h stands for cHaos
17:06tcampbelllol
17:41mrgigglesthe mozilla-inbound tree is now closed (toolchain bustage, but jobs also start with bug 1324892)
17:41firebothttps://bugzil.la/1324892 NEW, spohl.mozilla.bugs@gmail.com Switch Mac builds to 10.11 SDK
17:51mrgigglesthe mozilla-inbound tree is now open
18:15mccr8John-Galt: Could you go through the patches for 1186409 in mozreview and r+ them? MozReview got confused at some point and it won't let me land until those are set... Thanks.
18:17John-Galtmccr8: Yeah. Sec
18:17* John-Galt frowns at mozreview
18:17mccr8I'm not sure if I messed up the commit IDs at some point or what.
18:20mccr8and away we go.
18:20tcampbellhttp://i.imgur.com/7drHiqr.gif
18:20John-GaltI don't think so. That probably would have created new bugzilla attachments rather than resetting the flags on the existing ones
18:20tcampbellmccr8: John-Galt: what are next steps?
18:21John-Galttcampbell: Is that Ron Paul?
18:21mccr8it is
18:21tcampbellI think so?
18:21John-GaltHeh
18:21tcampbellthe internet is a weird place
18:22tcampbellI'm working on the IndirectEval thing today, but unclear if it will be too hacky
18:22mccr8It only took me about 15 messages to the bug to land it.
18:22mccr8tcampbell: well, it sounds like whatever stuff you mentioned in comment 140 needs to be addressed.
18:22John-GaltI don't particularly care about the indirect eval thing. I think we should just fix the code that depends on it, and then ban eval in system globals after the merge.
18:23John-GaltI think the next step is probably to pref on for non-release builds, wait a week or so, and then decide what we want to do for beta
18:23tcampbellJohn-Galt: that works too. I'm likely going to end up with an optional API that one can call on the NSVO to shadow the eval/Function
18:24mccr8I was thinking I might shuffle around the dependencies for remaining stuff to be about the pref flip but maybe I've spammed bugzilla enough already...
18:24tcampbellJohn-Galt: do you have a sense of what code breaks when you ban eval?
18:25mccr8If I get a green try run, I'll post to dev-platform about the pref once it has hit Nightly and people can try it out.
18:25John-Galttcampbell: The only things that broke in my push were a couple of third-party libraries that used indirect eval to get the global (which are one-line fixes) and some test code that used eval, mostly for stupid reasons.
18:25John-Galtmccr8: Yeah, that sounds like a good idea
18:26tcampbellJohn-Galt: are there things that live out of tree that we might miss? I don't know how the ecosystem is distributed and what code survives killing add-ons
18:26John-GaltTry run: https://treeherder.mozilla.org/#/jobs?repo=try&revision=d6140b978d26318a885289e0adfcb3fcd3953563
18:27John-Galttcampbell: The only out-of-tree things that might break at this point are test pilot add-ons
18:27mccr8Yeah I'm a little concerned about breakage there given that they are out of tree. I guess JSM merging won't affect them because they are addons still...
18:28tcampbellwell, now that it is a pref we can make sure they test it
18:29John-GaltYeah. Strict mode broke one of them. JSM merging won't break their eval usage because, yeah, they don't get global sharing. Banning eval might, in which case someone will suffer my severe displeasure...
18:29mccr8John-Galt: so do you think I should shuffle the remaining blocked and blocking bugs onto the turn-on-the-pref bug?
18:29John-Galtmccr8: Yeah, that probably makes sense
18:30John-GaltShouldn't generate that much bugspam, and anyone who gets spammed asked for it, anyway.
18:30tcampbellagree
18:30mccr8Cool.
18:31mccr8tcampbell: I'll get a final go ahead from you before I land any pref flip.
18:33tcampbellgreat. I've been going cleaning up little related things in js, but not hitting new nsvo-specific issues
18:33tcampbellthe subscript loader makes me sad, but that is another story
18:38John-Galttcampbell: Which parts of it?
18:39* John-Galt really wants to get rid of the shared object lexical scopes...
18:39tcampbellJohn-Galt: the environment provides the subscript is bizare
18:39John-Galtshu added them as a hack to deal with code (especially add-ons) that depended on `let` and `const` showing up across multiple scripts. But that behavior really kind of sucks
18:40tcampbellJSOP_FUNCTIONTHIS(null) -> global object
18:40tcampbellJSOP_GLOBALTHIS -> loader target
18:40tcampbellJSOP_IMPLICITTHIS -> loader target
18:41tcampbellit is a weird hybrid of rules..
18:41John-Galttcampbell: So, the two changes I really want to make to the subscript loader now that legacy add-ons are gone is: 1) no more shared lexical scope for everything loaded into the same object, and 2) no more different capturing behavior for qualified vs. unqualified assignments.
18:41John-GaltThe former leads to weird, unexpected conflicts between scripts loaded into the same object, and the latter leads to weird, random variable leaks to the global when people aren't careful enough.
18:41tcampbellJohn-Galt: https://docs.google.com/a/mozilla.com/spreadsheets/d/1BNQt3o0lOU7DTY5_wM8kro5UN5LrCnTwChx7n0-xclQ/edit?usp=sharing
18:42John-GaltBut that requires that we do something about the non-syntactic with scopes we use for event listener attributes, since they can't capture unqualified variable assignments
18:44tcampbellThe event listener case is a little funny that it even has a lexical scope between the attributes and the event function. It can't be accessed by anyone
18:44John-GaltI think that's basically just a side-effect of it reusing the same scope chain code we use elsewhere.
18:44tcampbellyep
18:44tcampbellfor sure
18:46tcampbellFrameScripts using |this| to be the global also seems less ideal than having an explicit binding when you want to access the MM
18:51John-GaltYeah, I really want to fix that too. Ideally, Cu.import, loadSubScript, and frameloader scripts should behave as similarly as possible.
18:52John-GaltBut one thing at a time, I guess... Better to wait til the next cycle for those.
18:53tcampbelloh yeah, they are down the road for sure. I've just been mapping out the current state in detail so I don't break and notice these oddities
19:03John-Galttcampbell: We should break them, though. :) Just... without breaking code that depends on them
19:05John-Galttcampbell: There are, as of the past couple weeks, like 5 people who have any concept of how these things behave in all the odd corner cases. I'm one of them, and even *I* get a headache trying to verify that your spreadsheet and your tests are correct. For ordinary users of those APIs... there is just no hope
19:11tcampbellmy spreadsheet was from brute force experiments. Looking at the code confused me so much
19:14John-GaltYeah. Your spreadsheet is by far the best documentation of the way those things work that I've seen yet. And I still can't look at it without getting a headache. If a contract is that hard to document in an understandable way, it's a good sign it's not a good contract.
19:48tjrWould anyone be willing to teach me how to induce JS to be jit-ted, and then find and debug the jitted assembly? (Ideally on Win64?)
19:49tjrMaybe late on a friday is not a great time, but we could schedule something for Monday or Tuesday?
19:51tcampbelltjr: A few starting points for your search.. Use the jsshell with "--ion-eager" flag, then you should be able to break on js::jit::IonCannon / EnterIon
19:52tcampbelltjr: I'd suggest doing debug builds, since the JIT code is still optimized by breakpoints are easier
19:53tcampbell*but
22:53John-Galtmccr8: Hrm. :( It looks like my follow-up patch causes us to access the shareGlobal pref too early in the parent process, so it always winds up false. What do you think about just always creating the shared loader global to compile module scripts in, even if we don't have global sharing enabled?
22:54mccr8John-Galt: I guess that sounds reasonable. I suppose the drawback is that you have some slight additional overhead when sharing is disabled, for the extra compartment. But it shouldn't be too bad...
22:56John-Galtmccr8: Yeah, I don't think there should be much overhead if we don't actually run any scripts in it, and hopefully global sharing will be able to ride the trains default enabled anyway.
22:58mccr8hmm enabling JSM sharing breaks one of the tests that tcampbell just added. That seems bad. ;) test_ComponentEnvironment.js
23:03John-GaltHeh
23:07John-Galtmccr8: Hm. Turns out, I'm wrong. It wasn't my follow-up. The preference winds up false for me when ReallyInit is called to load the main process singleton...
23:08John-GaltEven if I set it to true in all.js
23:08* John-Galt wonders if he's doing something stupid
23:08mccr8Hmm. Weird. I tested it locally and it seemed fine but maybe there's some difference in how the processes are starting? Or the OS? I'm on Linux.
23:09mccr8did you do a ./mach build or just a build binaries? I think sometimes you need a build step to get it stored properly.
23:10John-GaltYes, semi-stupid. I didn't do a full build so greprefs.js didn't get updated.
23:10John-GaltSo it just has the default value at that point, not the user value
23:11mccr8phew
23:12tcampbellmccr8: that test should fail
23:12tcampbellIt tests for indirecteval
23:13mccr8tcampbell: yeah sorry I should have investigated a little more. I'm going to file a bug for making the failures more obvious. ;)
23:13tcampbellHaha, no worries
23:14mccr8right now when it fails, the output is: FAIL | js/xpconnect/tests/unit/test_ComponentEnvironment.js
23:14mccr8xpcshell return code: 0
23:15tcampbellOh, yeah, I don't know how to xpcshell, feel free to make that suite more informative
23:16mccr8Kind of annoying how every test suite has its own way to do things...
23:16mccr8Fortunately I was just looking at some xpcshell tests.
9 Sep 2017
No messages
   
Last message: 11 days and 3 hours ago