mozilla :: #fx-team

11 Jul 2017
00:36gandalfso, a single patch with some changes to dom/base causes 60min recompilation. That's nuts.
01:28evanxdping jaws
03:42jmspeexAnyone can suggest how to diagnose this problem I've been having for the past 2-3 weeks with FF where suddenly all open tabs become unresponsive? New tabs work fine, but tabs already opened when the bugs trigger completely stop working (blank)
07:40igoldanhi
07:41igoldanwhat's the command for running eslint tests for all *.js in Firefox codebase?
07:41igoldanis it | mach lint --linter eslint --quiet |?
07:41igoldanor something else?
07:52Standard8igoldan|afk: yeah, thats good enough (itll run for html/jsm/xml as well)
11:23Gijsmikedeboer: lol at the empty ifdef :|
11:23mikedeboerGijs: ah, you're watching along ;)
11:24* Gijs needs to head out, will be back in ~1.5 hours
11:24Gijsmikedeboer: my other patch is driving me to despair, so... hey, at least the r+s are cheering me up!
11:24Gijsanyway
11:24* Gijs out
11:24mikedeboergood, gives me time to test drive your patches :)
11:24Gijs_away:D
13:28mikedeboerHow do I unset MOZ_PHOTON_THEME?
13:30johannhmikedeboer: either in toolkit/moz.configure or in your mozconfig: export MOZ_PHOTON_THEME=
13:31mikedeboerjohannh: ty!
13:31johannhmikedeboer: np
14:37mikedeboerjohannh: did you know that the Downloads icon is gone in non-photon mode?
14:37johannhmikedeboer: we need screenshot tests on cedar :(((
14:47nhnt11\o/
14:48nhnt119.5 hours after I began my efforts, a fresh hg clone has successfully completed on my windows laptop
14:53pastnhnt11: was that the reference hardware?
14:55nhnt11past: nah, it's the Razer Blade (it has an i7 I think)
14:55nhnt11I had to restart it a couple of times
14:55nhnt11(stream interrupted)
14:55nhnt11and then it takes forever to unpack and update
14:56pastoh, right
14:56* nhnt11 complains more but directs it to /dev/null
14:56pastI hope you started from a bundle
14:56RyanVMthat's the default these days
14:56Gijsdoesn't hg do that automatically?
14:56nhnt11yeah, it defaults to that
14:56nhnt11yep
14:56Gijsnhnt11: can Mozilla pay for your internet to suck less?
14:56RyanVMWindows I/O still sucks though :P
14:57nhnt11Gijs: I have 100mbps broadband, it's pretty great
14:57Gijsnhnt11: so what took 9.5 hours? :s
14:57GijsWindows IO is bad but not *that* bad
14:57RyanVMlol
14:58nhnt11yeah
14:58nhnt11that's what I thought too
14:58nhnt11It kept getting stuck
14:58* nhnt11 shrugs
14:59nhnt11At some point I disabled bitlocker
14:59nhnt11I think that helpd
14:59nhnt11helped(
14:59nhnt11*
16:02florianwhat would be the right place to call LightweightThemeManager.addBuiltInTheme for the compact themes so that they are only added either if we are using them, or when the customize mode is entered? Currently they are added before we create the first window at http://searchfox.org/mozilla-central/source/browser/components/nsBrowserGlue.js#633
16:04Gijsflorian: they would also need to be created if they get synced in
16:04Gijsflorian: or if the user enters about:addons
16:04Gijsflorian: not sure that "not on startup" is feasible
16:04Gijsand/or worth the effort
16:06florianGijs: does LightweightThemeManager.jsm really need to be in toolkit?
16:06florianif not, this could just go in a getter for http://searchfox.org/mozilla-central/source/toolkit/mozapps/extensions/LightweightThemeManager.jsm#117
16:09florianGijs: so, if we can't move it off of startup, is there an easy way to know if one of these themes is going to be used for first paint? If they are used, it makes sense to add them early, otherwise we could do it from an idle callback once we are done with startup
16:13florianlooks like I can just read the lightweightThemes.selectedThemeID pref
16:15Gijsflorian: pretty sure other toolkit apps support lwt-manager, yeah
16:15pdolGijs: Is it easier to change what automigration imports from the default browser? (between passwords, history, etc.)
16:16pdolContext: The funnelcake tests we ran with full automigration dropped retention relative to control. The exact same version without automigration increased retention.
16:16pdolSo, we likely have a perf issue still or it's possible that's it's turning some users off.
16:16pdolIt might be worth testing a few variants to try to isolate the issue.
16:17pdoleasy*
16:19Gijsflorian: what does that actually gain us, though? Feels like the complexity has costs and adding 2 JS objects to a JS Map() should be effectively 0-cost.
16:20Gijsflorian: if it isn't 0-cost, maybe we should instead investigate why it isn't.
16:20florianloading LightweightThemeManager too early, and accessing a string bundle and localized strings too early
16:21florianGijs: it doesn't add much complexity: https://pastebin.mozilla.org/9026856
16:21Gijspdol: relatively easy, you'd need to change https://dxr.mozilla.org/mozilla-central/source/browser/components/migration/AutoMigrate.jsm#57-60
16:22Gijspdol: and/or make it rely on a pref (they're just int flags)
16:22aswanflorian: do we actually reference the string bundle when the jsm is loaded? it is meant to be loaded lazily
16:23pdolGijs: thanks. Looks like we could likely try a few funnelcakes with different things being imported and see if the retention drops only for one data type (like history)
16:24florianaswan: http://searchfox.org/mozilla-central/source/browser/components/nsBrowserGlue.js#638 accesses it synchronously
16:24Gijsflorian: lots of stuff in nsBrowserGlue references gBrowser/gBrandBundle , and browser.xul will reference those, too...
16:24Gijsflorian: if our string bundle implementation isn't crap then that shouldn't be important in perf terms either
16:24Gijsit's not like it's referencing super-special-different-budnle
16:25florianGijs: any I/O matters for cold startup
16:25Gijsit's not IO if the bundle is already loaded
16:25Gijssurely we don't re-read the entire file for every string we look up? :s
16:25florianit's the first thing that loads it right now
16:25aswanflorian: ah, i had assumed you were talking about http://searchfox.org/mozilla-central/source/toolkit/mozapps/extensions/LightweightThemeManager.jsm#24
16:25Gijsflorian: but there'll be another thing 3ms later that instantiates it.
16:26Gijsflorian: what's the point?
16:26florianGijs: cleaning up enough the stuff running off of final-ui-startup, that it's possible to covert it with tests and prevent more stuff from being introduced there
16:26florian*cover
16:27Gijsflorian: the stringbundles aren't important though, as I've already said, other stuff will be loading those same bundles...
16:27Gijsflorian: what instantiates LWTManager if not that code?
16:28florianat that point, no individual thing is important (those that were very visible in profiles have been fixed already), is just lots of details that add up and make it too easy to add initialization for random new stuff at a random point of startup
16:28florianGijs: I think it'll be a script loaded by browser.xul, but I'm not sure yet
16:31Gijsso then that'll still be during startup, right?
16:31GijsI still don't see a point in delaying this from one point during startup to a slightly later point that's still during startup.
16:32mikedeboerGijs: switching between photon and non-photon requires a configure & build, which with a non-artifact debug build takes a little while
16:32mikedeboerGijs: so that's why the r+ is held up just a little bit ;)
16:33Gijsflorian: anyway, if there's a serious perf issue here that I'm somehow not understanding, then make the lwthememanager send an observer notification when it wants a particular theme that it doesn't know about, and make nsBrowserGlue add the relevant data structure in response to that observer topic.
16:34Gijsflorian: that way we still guarantee that the theme is there if needed, we don't have to introduce manual reads of the same prefs risking things getting out of sync if we change the storage format or something, and we won't load the jsm any earlier than before the builtin themes got added.
16:36Gijsmikedeboer: no worries, I expect there's still automated test issues
16:42pdolGijs: if a certain part of automigration is causing some perf issue, what's your guess on what it would be? History?
16:42Gijspdol: telemetry should know, I think - have you checked?
16:43Gijspdol: because it's opt-in I think you'd need a python thing to look at the data, but the data folks would know better than me
16:48pdolGijs: There are definitely some buckets or higher jank for bookmarks/history. Where in the experience would that jank manifest for the automigration case?
16:48pdolGijs: I'll see if the data folks can see if there is a correlation with retention drops.
16:49Gijspdol: the short answer is I'm not sure.
16:49pdolGijs: okay, thanks
16:50Gijspdol: the longer answer is that there are different things that could cause the jank, and in any case the import effectively coincides with startup, so it really would depend on how quickly startup is happening and/or how long the migration takes, if that makes sense
16:50Gijspdol: maybe startup is fast and the migration takes a long time (even if relatively jank-free) and the jank shows up later while Firefox is running
16:50gandalfaswan: do you have any better idea on what timeframe wrt. bug 1365709 we may be expecting?
16:50firebothttps://bugzil.la/1365709 ASSIGNED, aswan@mozilla.com Consume new webextension-based langpacks
16:50Gijspdol: maybe startup is slow and the migration is relatively short, and then jank will seem like it's part of startup
16:51Gijspdol: but it's hard to say without spending a lot of quality time with the data.
16:51Gijs(time which unfortunately right now I don't have)
16:51pdolYeah which would give a very poor impression and may lead to lower retention for either of those cases.
16:51pdolNo worries, I'll have someone look into it.
16:51aswangandalf: i actually started on it yesterday and hope to make a bunch of progress today. in a meeting right now but will you be around later today?
16:52MattNMossop: I was advising and attending their meetings but I'm not involved anymore
16:52gandalfaswan: awesome! yeah :)
16:52gandalfthanks so much! :)
16:53Gijspdol: cool. The retention correlation thing would be the most useful. That and/or just general correlations for various metrics on retention seems like it'd be super useful
16:53Gijspdol: have we tried doing a generic version of that on data we have more broadly?
16:56florianGijs: that seems like the Right Way to do it, but I'm not sure the added complexity is worth it just to avoid "risking things getting out of sync if we change the storage format"; is that likely to ever happen? I'm more annoyed by hardcoding the theme names than by the pref read, but they are already hardcoded in at least one other place: http://searchfox.org/mozilla-central/source/browser/base/content/browser-compacttheme.js#20
16:57pdolGijs: we're looking at that now. We've learned that of the users who churn after a day, the median time spent in FF is 10 min.
17:00Gijsflorian: well, there's that and the potential for races from the idledispatchtomainthread, plus the fact that if the themes aren't used they'll never be added, right?
17:04Gijsflorian: also, I'm a bit confused when my initial argument to not even fix this was "it adds complexity" and now that I've suggested something you're also saying "but that's complex" :P
17:04florianGijs: my suggested fix is simple though
17:04Gijsflorian: but the idle dispatch won't allow testing for the module's presence (or otherwise) either, presumably?
17:04Gijslike, won't that race just as hard?
17:05Gijsand then intermittently fail whatever "shouldn't load this module" test?
17:05Gijsplus, it either delays the init materially, in which case it risks actual races that cause user-observable bugs
17:05Gijsor it doesn't, in which case it doesn't seem like it actually buys us anything in terms of startup perf
17:06Gijs(in that the same perf-sensitive things will still happen at roughly the same time)
17:06florianGijs: I got tired of idle callback races in my startup tests, and filed bug 1380011 to fix it
17:06firebothttps://bugzil.la/1380011 NEW, florian@queze.net Make startupRecorder.js' 'before handling user event' phase more reliable
17:07florianGijs: the patch I suggested won't win anything if the user has selected one of the compact themes, and will add the themes once the main thread is idle otherwise. I can't think of a user observable bug that would cause.
17:07Gijsflorian: I can't tell whether you're claiming the delay will be significant or not
17:08florianI don't understand what you mean.
17:08Gijsflorian: "once the main thread is idle" does not mean anything sensible to me
17:08florianthe main change is that the code will be running after first paint when we don't need it before first paint.
17:09Gijsflorian: is there some guarantee that the code will run before first paint?
17:09Gijssounds like you're saying "no"
17:09florianbah
17:09Gijsso, what happens, for instance, if I session restore a session with about:addons or customize mode?
17:09Gijswill the items be in there or not?
17:09florianthe goal is to not run it before first paint, unless the theme is currently in use, in which case the patch I had is strictly equivalent to the current behavior
17:09Gijsand what guarantees that?
17:10GijsI don't think that idledispatch is the panacea we want here.
17:11florianhmm, can about:addons be restored directly to the 'Appearance' section?
17:12Gijsyes
17:12Gijsflorian: the end goal is to run this code as late as possible, but no later than necessary
17:12Gijsflorian: I'm arguing that idledispatch gives us neither of those.
17:13Gijsflorian: in some cases, it will run too late (the ones I just described)
17:13florianthat case will break then :(
17:13Gijsflorian: in others, it will run after first paint but the user will never need the theme that run
17:13Gijsflorian: so I don't think it's useful here.
17:24florianAddonManager.jsm loads LightweightThemeManager.jsm unconditionally before we create the first window, so there's indeed probably not much to win here
17:25Gijs:(
17:25Gijsglob: was there a bmo push in the last few hours?
17:26Gijsah, yes, there was
17:26globGijs: yes
17:26* Gijs moves this over a channel or some
17:26globGijs: ping dylan in #bteam
17:27Gijsglob: sorry, got into #bmo before I got that, I assume that also works?
17:27globGijs: yup
17:30globGijs: bug 1380019
17:30firebothttps://bugzil.la/1380019 NEW, dylan@mozilla.com Background color of security bugs is wrong
17:32Gijsthanks! I commented
17:34Gijsmikedeboer: you still around?
17:58mikedeboerGijs: somewhat, yes
17:59Gijsmikedeboer: no worries, I will just file a separate bug for something I noticed, was considering slipping it into the other overflow menu patches, but really it's better as a separate bug anyway
17:59mikedeboerokidoke
18:09jawsperry: ping?
18:27perryjaws: Hi
18:29jawshey perry, i was gonna ask you about why you canceled the reviews your bug but i think you're fixing a bug with it?
18:30perryOh, I caught one of my new tests timing out on linux. I'm fixing it right now and it'll probably be resubmitted shortly
19:18Gijsfelipe: I doubt it, but jwatt would know for sure
19:20johannhfelipe: with getComputedStyle?
19:20johannhmaybe I don't understand the question :)
19:20* Gijs assumes felipe wants a DOM reference to the <svg>&#39;s DOM
19:20Gijs(not just the URI)
19:21johannhoh I assumed just the URI
19:27felipeyeah
19:27felipeI gave up on that path anyways
19:30Standard8mconley: quick q, if we call loadFrameScript on a message manager, thats likely to be async with an indeterminate completion time, right?
19:31mconleyStandard8: the execution of the script in the content process?
19:31Standard8mconley: yeah - Ive got a test that is sending something to the frame script almost straight after calling the load
19:32mconleyI&#39;m pretty sure in the in-process case, it runs on the next tick of the event loop or so. For the out-of-process case, yeah, the content process needs to pop the message off the queue and run the script
19:32mconleyand the content process might be out to lunch
19:32mconleyhowever
19:32mconleymessages always execute in order
19:32mconley(with minor CPOW caveats)
19:33mconleyso if you do loadFrameScript, and then, say, do ContentTask.spawn on the same browser, you can know for sure that your ContentTask script stuff will run _after_ your frame script runs.
19:33mconleyStandard8: does that help?
19:34Standard8mconley: yeah thanks, it at least confirms that if I try what Im thinking then theres a chance of it helping
19:34mconley:)
19:34mconleycool
19:42Osmose./mach lint appears to be having trouble finding a package.json file for eslint-plugin-mozilla, anyone else seen this?
19:43Standard8Osmose: can you pastebin the full output?
19:43Standard8Osmose: alternately try nuking node_modules
19:43OsmoseStandard8: https://gist.github.com/Osmose/bee9de6fc290d1e19e30e51fb9da5183
19:44Standard8Osmose: which npm / node version are you on?
19:44OsmoseNuking node_modules did not fix it, sadly. :(
19:44OsmoseNode: 7.7.2, NPM: 4.6.1
19:44Standard8hmmm, Id have thought that would be ok
19:44Standard8the path looks a little strange
19:44OsmoseYeah that : is weird
19:45Standard8apparently that support has been there since 2.0.0
19:46Standard8Osmose: I guess you could try updating npm?
19:46Standard8Osmose: and presumable that file does exists?
19:47OsmoseI don&#39;t think it does, I don&#39;t have a file:tools directory at all
19:48OsmoseI have a tools directory, though
19:48Standard8Osmose: drop the `file:`
19:48Standard8thats just the protocol...
19:48OsmoseSo I&#39;m not passing a path at all, this is just what happens when I call ./mach lint
19:49OsmoseIt auto-generates the path &quot;/Users/mkelly/Projects/mozilla-unified/file:tools/lint/eslint/eslint-plugin-mozilla/package.json&quot;
19:49Standard8yeah I know, that comes from the top-level package.json
19:49OsmoseShould I edit that then?
19:50Standard8shouldnt be necessary
19:50Standard8it should just work ;-)
19:53Osmosemozilla-unified/tools/lint/eslint/eslint-plugin-mozilla/package.json exists
19:56OsmoseUpgrading to npm 5 doesn&#39;t work either
20:07aswangandalf: ping
20:10Standard8Osmose: hmm, unfortunately that leaves me at a loss as well
20:13aswanah, unping gandalf
20:14gandalfaswan: pong
20:14gandalftoo late!
20:15aswanheh, i was going to say i couldn&#39;t find the L10nRegistry anywhere then i found bug 1333980
20:15firebothttps://bugzil.la/1333980 ASSIGNED, gandalf@aviary.pl Land L10nRegistry
20:15gandalfaswan: yeah, it&#39;s in review. For now if you can make space for where the call to L10nRegistry will be fired, that&#39;s enough I believe :)
20:17aswanokay great, i can spit out something half-baked today and we can iterate from there...
20:19gandalfyeah, awesome!
20:19gandalfI&#39;ll test it against the whole puzzle I have locally and will let you know if we need anything else :)
20:27Gijsmiketaylr: ping?
20:27Gijsmiketaylr: so... I&#39;m looking at https://reviewboard.mozilla.org/r/153002/diff/3#index_header
20:28Gijsmiketaylr: and pre-patch, we&#39;re just Cu.importing TabListener.jsm
20:28Gijsand your patch changed it to use XPCOMUtils.defineLazyGetter
20:28Gijsso we won&#39;t import it until we need it
20:28Gijsthis makes sense
20:28miketaylrheya
20:28Gijsbut then your latest changes to make it pass test force-import it in init() in more or less the same place as before, if it&#39;s not loaded.
20:29Gijsmiketaylr: isn&#39;t that basically the same in that it always imports it when init() runs? :)
20:29johannhmak: late ping
20:30miketaylrhmm yeah, thinking
20:31Gijsmiketaylr: so my understanding is that the difference we&#39;re interested in here is that normally, on startup there won&#39;t be windows
20:31Gijsand so we shouldn&#39;t need to import this immediately, because the loop over CustomizableUI.windows will just return an empty array and nothing will happen
20:32Gijsmiketaylr: we&#39;ll only need it when the first onWindowOpened call happens, which is when the window actually gets loaded and stuff, which is outside of the &quot;initial startup&quot; phase that ehsan and co are interested in.
20:32miketaylryeah, in part 2 (https://reviewboard.mozilla.org/r/153004/diff/8#index_header) we&#39;re only calling init after delayedStartup, when the main thread is idle
20:33Gijsok, but then we probably don&#39;t need part 1?
20:33miketaylryeah... i think you&#39;re right.
20:34Gijsmiketaylr: as an alternative, define the lazy getter in init() itself.
20:34Gijsthat way, if init re-runs, the getter will be defined again.
20:35miketaylroh, cool. lemme try that.
20:35Gijs(be careful with the |this| reference if you do this)
20:35OsmoseStandard8: I was able to get things working by installing eslint-plugin-mozilla directly from npm
20:36OsmoseI suspect something is up with those file: paths, and potentially my local setup
20:36Standard8Osmose: weird, npm should be symlinking it
20:36Standard8locally
20:37Gijsmiketaylr: cool, summarized on the bug. I gotta go pack, but I&#39;ll try to be quick in followup rounds tomorrow if need be :)
20:37miketaylrcool, appreciate the help Gijs
20:37Gijsnp!
21:14johannhmak: unping
21:15Mossopgandalf: http://projectfluent.io/fluent/guide/text.html says that leading and trailing whitespace is ignored. Does that include the newlines in a multi-line message or are they preserved?
21:18gandalfMossop: ignored
21:19gandalfMossop: http://projectfluent.io/play/?gist=884222cac5b0f7c6d46388db53e8afcf
21:20Mossopgandalf: If all whitespace is ignored doesn&#39;t that string become &quot;Loki is a simple micro-bloggingapp written entirely in <i>HTML5</i>.It uses FTL to implement localization.&quot; ?
21:22gandalfMossop: testing. I may have misremembered
21:22MossopLooks like maybe the whitespace is coalesced as in HTML?
21:22gandalfyes
21:23MossopOk
21:23gandalfMossop: https://pastebin.mozilla.org/9026881
21:43Mossopgandalf: Is there a particular reason you went with manually parsing characters at a time rather than using regular expressions?
21:43gandalfMossop: performance
21:44Mossopgandalf: You measured it to be faster?
21:44gandalfyeah
21:44MossopHuh. That&#39;s quite surprising
21:44gandalfwe did a lot of cmd line benchmarking using xpcshell and d8
21:44gandalfand regexps were slower
21:44gandalfit may have changed lately tho
12 Jul 2017
No messages
   
Last message: 72 days and 11 hours ago