mozilla :: #servo

16 Jul 2017
00:24Manishearthemilio: birtles ping
01:04Manishearthhiro: ah
01:04Manishearthhiro: there?
01:05Manishearththis is exactly what I've been struggling with right now :)
01:05hiroManishearth: yes, you just read the comment?
01:05hiroManishearth: the code is bit complicated actually.
01:05Manishearthhiro: so there are two functions
01:06Manishearthhiro: so basically, it used to use nsStyleContext and ServoComputedValues
01:06Manishearthhiro: is it ok if I call that function with ServoStyleContext if in servo mode?
01:07Manishearthor should I not change the function to take GeckoStyleContext
01:07hiroManishearth: sure.
01:07Manishearthhiro: what's the difference between the two functions when StyleType changes?
01:07hiroManishearth: but the point is, we use nsStyleContext even if we are on stylo.
01:08hiroManishearth: when we call Element.animate(), we use nsStyleContext both on gecko and stylo.
01:08Manishearthhiro: yeah, but the problem is in the new world we have a clash between nsStyleContext and ServoStyleContext
01:08Manishearthhiro: ok
01:08Manishearthhiro: i guess my question is then
01:08hiroManishearth: and then, in DoUpdateProperties() we handle it separately.
01:08Manishearthhiro: when the one with nsStyleContext is called, in Servo mode, is it ok to call http://searchfox.org/mozilla-central/source/dom/animation/KeyframeEffectReadOnly.cpp#199-205 ?
01:09Manishearthor must http://searchfox.org/mozilla-central/source/dom/animation/KeyframeEffectReadOnly.cpp#192-197 be called?
01:09Manishearthand what should I do if the style context is null?
01:09hiroManishearth: you can call Servo version.
01:09Manishearthgreat
01:10ManishearthI was afraid that would be an issue
01:10hiroManishearth: we have null check for style context somewhere I don't remember.
01:10hiroManishearth: ah, the null check is in DoSetKeyframes().
01:11Manishearthhiro: ok
01:12Manishearthhiro: oh so it doesn't matter in the null case
01:12Manishearthwonderful thanks
01:12hiroManishearth: thank you!
01:17Manishearthhiro: https://hg.mozilla.org/try/rev/51710bd2441f31a7e526988bafef57040ec25f51 ?
01:19hiroManishearth: thanks, looks good for now. We need to clean it up sooner or later in bug 1349004. That was actually our animation team bug. :)
01:19firebothttps://bugzil.la/1349004 REOPENED, mantaroh@gmail.com stylo: Call UpdateProperties with ServoComputedValues (for target element and parent's one) instead
01:24Manishearthhiro: cool. thanks for looking through my patches :)
01:24Manishearthhiro: currently in my patches Part 9 broke transitions somehow, btw
01:24Manishearthbrb
01:25hiroManishearth: actually I haven't look all over the patches, I just skimmed animation code.:)
01:25Manishearthheh
01:26hiroManishearth: you mean part 9 in MozReview?
01:26Manishearthhiro: yeah
01:27hiroManishearth: oh, is that related to animation?
01:27Manishearthhiro: has_css_transitions() retrns false
01:27Manishearthhiro: it isn't :(
01:28Manishearthbut the bug isnt there in previous patches
01:28hiroManishearth: hmm, OK. interesting.
01:34hiroManishearth: I am reading the part 9, but I found no suspicious point. Honestly I am sill not familiar with reading rust code though.
01:34hiroManishearth: which test cases failed actually?
01:40allonhadayaWould any of you happen to be in the Portland area and be interested in getting together for a little hack session?
01:41Manishearthhiro: transitions don't work at all
01:41Manishearththey happen immediately
01:43hiroManishearth:hmm, relevant code is, I guess, changes in update_animations().
01:44Manishearthhiro: hmm but there are no changs there
01:45hiroI don't really understand what the difference between arc_as_borrowed() and as_borrowed().
01:47Manishearthhiro: they do different conversions
01:47Manishearthactually, hm
02:14Manishearthemilio: there?
02:19emilioManishearth: can try, though its pretty late here
02:20Manishearthemilio: https://dxr.mozilla.org/mozilla-central/source/layout/style/nsComputedDOMStyle.cpp#674-675
02:20Manishearthemilio: we call ResolveTransientStyle there
02:20Manishearthand RTS uses a null parent context ptr
02:21emilioManishearth: that's because its not used
02:21Manishearthwhich triggers some asserts
02:21Manishearthactually maybe I should just remove those asserts for servo
02:21Manishearthasserts are in nsStyleContext::SetStyleIfVisited
02:21emilioManishearth: Ok, wait, so... SetStyleIfVisited on what?
02:22Manishearthemilio: ResolveTransientServoStyle calls GetContext
02:22emilioManishearth: yeah, though it should just return a context with your changes
02:22Manishearther, ResolveTransientStyle
02:23Manishearthemilio: yes, but GetContext calls SetStyleIfVisited, which has asserts in it ensuring that the parent and the parent of the visited style are the same
02:23emilioManishearth: the point is that you should not be calling GetContext... How do you know where to inherit from?
02:24emilioManishearth: what that function does is basically resolving the style "manually" up the parent chain
02:24Manishearthemilio: ResolveTransientServoStyle should just not call GetContext? that works
02:25emilioManishearth: wait, what? Why would it? What if the element is unstyled?
02:25Manishearthemilio: wait no it used to
02:25Manishearthemilio: http://searchfox.org/mozilla-central/source/layout/style/ServoStyleSet.cpp#543-555
02:26emilioManishearth: Sure, that's as of right now... But aiui, with your patches the servo thingie should return a context directly
02:26Manishearthemilio: yeah, but a context that hasn't passed through GetContext()
02:26emilioManishearth: the servo thingie being ResolveStyleLazily, if I remember correctly
02:27Manishearthso e.g it may not have a gecko-side visited style
02:27emilioManishearth: what's that GetContext supposed to do?
02:27Manishearthemilio: currently GetContext only really sets up the visited style on the gecko side
02:27emilioManishearth: why do we need a gecko side visited style on the first place?
02:28Manishearthemilio: because I haven't refactored that yet
02:28Manishearthemilio: it's a followup, the visited style stuff is complex and we use both of them right now
02:28emilioManishearth: That makes no sense, there's no way you're going to get the correct style parent there, I guess
02:29Manishearthyeah
02:29Manishearthemilio: what?
02:29Manishearthemilio: anyway
02:29Manishearthemilio: I figured out the fix
02:29ManishearthI'm just skipping it if GetStyleIfVisited() is nonnull
02:29Manishearthbc it doesn't need to be set up again
02:29emilioManishearth: the servo side has the parent style, but that style may just be temporary
02:29Manishearthyeah
02:30emilioManishearth: so if you don't store it anywhere (maybe you do?) I don't know how can you end up there
02:30emilioManishearth: with the correct one, I mean
02:30Manishearthemilio: store what?
02:30ManishearthI don't understand you
02:30emilioManishearth: how would GetStyleIfVisited be non-null in the case I'm talking about?
02:31emilioManishearth: ok
02:31emilioManishearth: so the transient style thing does two things
02:31emilioManishearth: if the element has a style, you just return it, nothing special there, no extra setup should be needed
02:32emilioManishearth: but it also handles the case of unrendered subtrees, where the element doesn't have a style
02:32Manishearthemilio: when it computes that style is that style discarded immediately after?
02:33emilioManishearth: and in that case it resolves the parent chain, to get the right style to inherit from, and then returns the element style
02:33emilioManishearth: and the parent chain is just discarded
02:34emilioManishearth: so my point is that there's no way that you could get the correct parent style for the context you're returning as of right now
02:34Manishearthyeah
02:34ManishearthI get that
02:34emilioManishearth: so, what's the thing that was unclear?
02:35Manishearthemilio: wanted to be sure this style context is discarded
02:35Manishearthgetting an assert in mozilla::ServoRestyleManager::TextPostTraversalState::ComputeHintIfNeeded
02:35emilioManishearth: which style context?
02:35Manishearth"missed some frame in the hierarchy"
02:36Manishearthemilio: the one we're creating?
02:36emilioManishearth: which test?
02:36emilioManishearth: that's not necessarily discarded, that's used for gcs, for example
02:37Manishearthemilio: gcs?
02:37emilioManishearth: getComputedStyle
02:37Manishearthanyway i guess if stuff worked with having the wrong parent context before it can still work
02:38Manishearthoh
02:38Manishearthok thats fine
02:38emilioManishearth: why do we need the parent style again? Aren't we getting rid of it?
02:38Manishearthemilio: what
02:38Manishearthoh
02:38emilioManishearth: why do we need it?
02:38Manishearththere are asserts :)
02:38Manishearthbut yes, we don't need it. that clears some things up actually
02:39emilioManishearth: gecko needs it for getting inherited stuff, but we get that upfront
02:39Manishearthok
02:40emilioManishearth: I'd really really prefer to keep using the same struct for Servo and Gecko btw, and I'm unsure why that wouldn't be possible
02:40Manishearthemilio: what?
02:40emilioManishearth: instead of that to_outer thing
02:40Manishearthemilio: again no idea what you're talking about :)
02:40Manishearthemilio: where are we using different structs?
02:41emilioManishearth: why do we need to differentiate between StyleContext and ComputedValues, I mean
02:41emilioManishearth: well, one has a pointer to the pres context
02:41Manishearthemilio: so when I was writing this there were some visited style shenanigans that meant that there *had* to be a way to deal with a plain CV
02:41emilioManishearth: but on topbof that, and maybe the bits (but CV already has bits), why should they differ?
02:41Manishearthand some animation stuff
02:41Manishearthbut I removed that
02:41Manishearthor
02:41Manishearththat got removed later
02:42Manishearthemilio: the distinction exists primarily for my sanity
02:42Manishearthemilio: I plan to remove it later
02:42emilioManishearth: I think it's not sane to keep it
02:42Manishearthemilio: trust me. i tried to avoid it when writing these patches and they got worse
02:42Manishearthemilio: it's going to go away
02:42Manishearthjust not immediately
02:43emilioManishearth: and given in m-c there are 12 y/o comments that say "XXX Temporary hack", I'd prefer it to be as soon as possible...
02:43emilioManishearth: do we have bugs on file at least?
02:44Manishearthemilio: I have a list of followups to do and I intend to do them immediately
02:44Manishearthemilio: and ultimately it's not bad as is IMO
02:45emilioManishearth: well... There are a few things that are not that bad as is but make me really sad when I have to read the code again... :(
02:45emilioManishearth: and I spend a fair amount of time trying to keep complexity under control
02:46Manishearthemilio: yes, because this is a huge set of patches that keeps bitrotting, and it would be worse with those changes
02:46Manishearthalso, again, that distinction was necessary (or we were under the impression it was necessary) when I wrote those patches
02:46Manishearthbecause animations and visited style had some weird stuff going on
02:46emilioManishearth: I doubt a lot that's actually true
02:47emilioManishearth: I could see why before we had rules and visited styles in the ComputedValues
02:47Manishearthsure
02:47emilioManishearth: and even with that I don't see what that distinction makes easier
02:48emilioManishearth: like, at all
02:48Manishearthemilio: no IIRC there were some cases where you actually needed an Arc<ComputedValuesInner>
02:48Manishearthand anyway
02:48emilioManishearth: when?
02:48Manishearthwe discussed this all before I started these patches
02:48Manishearthand the plan was to split it and pass around &CV or &CVI depending
02:48emilioManishearth: and why couldn&#39;t that move to use a SC
02:48Manishearthnow that the change is made it seems like we can always use &CV
02:48emilioManishearth: that sounds ugly :(
02:49emilio(The CVI thing)
02:49Manishearthemilio: at the time we were under the impression it was necessary
02:49Manishearthemilio: and, besides, again, it makes the change much easier to carry out
02:49emilioManishearth: why is that? (Just curious)
02:49ManishearthI tried doing it the other way first, trust me
02:50Manishearthand there were some cases where we had a bare Arc<CV> that was shared but not off a style context. unsure where they were
02:50Manishearthemilio: lets us replace the use cases incrementally
02:50emilioManishearth: well, I&#39;d prefer an argument I guess... Can de maybe discuss it next week?
02:51emilioManishearth: I emphathise with that, but I&#39;d rather prefer not having to land hacks and then spend time removing them
02:51emilioManishearth: happy to work in the patch myself too if you want
02:52emilioManishearth: (and if you think it&#39;d be helpful)
02:52Manishearthemilio: again, I tried to do it the other way -- spent two days doing that, and it was horrible
02:52Manishearthemilio: I really don&#39;t want to relitigate decisions made a month ago
02:52Manishearthemilio: I don&#39;t mind fixing it in this bug itself
02:52Manishearthbut I want to fix the leak and animation issue first
02:53Manishearthemilio: actually I do mind, this keeps bitrotting and I&#39;m wasting half my time fixing up rebases (this rebase created a bunch of new asserts)
02:53Manishearthso I&#39;d prefer to land this now and I promise to work on the followups immediately after
02:54emilioManishearth: that&#39;s why I emphatise, but the &quot;tried to unsplit for two days and it was awful&quot; doesn&#39;t make me feel like they&#39;re going away soon then, and I&#39;d really want to understand why :)
02:54emilioManishearth: but agreed
02:54Manishearthemilio: it is an easy change to mane *now*
02:54Manishearth*make
02:54Manishearthemilio: you misunderstand
02:54emilioManishearth: not going to stop you from landing it, but I&#39;m happy to help you polish it before it lands if you prefer
02:54Manishearthemilio: it was hard to execute this series of patches without the CVI distinction
02:54Manishearthbut
02:55Manishearthnow that the patches are done, getting rid of the distinction isn&#39;t hard
02:55ManishearthI don&#39;t mind working on it
02:55emilioManishearth: not sure why, but again, ok
02:55Manishearthbut I&#39;d really prefer to focus on fixing these asserts and stuff
02:55Manishearthemilio:
02:55Manishearth MOZ_ASSERT(mOwner == ExpectedOwnerForChild(aFrame),
02:55Manishearth &quot;Missed some frame in the hierarchy?&quot;);
02:55Manishearthany idea what that&#39;s about?
02:56emilioManishearth: I do have an idea about what&#39;s it about, but I need to know the test-case
02:56emilioManishearth: what&#39;s mOwner there, and aFrame?
02:56Manishearthemilio: layout/reftests/css-display/display-contents-acid-dyn-1.html
02:56Manishearthemilio: it&#39;s in ServoRestyleManager
02:57emilioManishearth: yeah, I know where it is
02:57ManishearthaFrame is an nsIFrame
02:57ManishearthmOwner in this case is an nsTableCellFrame
02:57emilioManishearth: sure, I mean which kind of frame it is :)
02:58emilioManishearth: OK, I bet the expected owner in this case is a :-moz-table-cell pseudo
02:58emilioManishearth: is that right?
02:58emilioManishearth: if so, that&#39;s an assertion bug, probably
02:59Manishearthemilio: moz-text
02:59emilioManishearth: is aFrame.GetParent()->GetParent() == mOwner?
02:59emilioManishearth: the _expected_ owner (not aFrame)
02:59Manishearthemilio: yes
03:00emilioManishearth: what&#39;s aFrame.GetParent()?
03:00Manishearthemilio: nsBlockFrame
03:00emilioManishearth: the pseudo, I mean
03:00Manishearthemilio: moz-cell-content
03:01emilioManishearth: is that an anon box?
03:01Manishearthemilio: i think?
03:01emilioManishearth: anyway, don&#39;t worry about that, file a bug and ill fix the assert
03:02Manishearthemilio: the assert only gets hit in my rebased patch series
03:02Manishearthemilio: i can try to fix it, is the fix easy?
03:02Manishearthanyway I&#39;ll ignore it for now
03:02Manishearththanks
03:03emilioManishearth: yeah, you just need to hop through the anon boxes, in ExpectedOwnerForFrame, which that function is supposed to do anyway
03:03emilioManishearth: but I guess since we&#39;re calling it on a text frame due to display contents that may have taken an unexpected path
03:04emilioManishearth: anyway, 5am here, gtg
03:04Manishearthemilio: lol ok
03:04Manishearthemilio: the fn seems to handle anon boxes already so I&#39;ll let you have a look at it
03:07emilioManishearth: yeah, so the problem is that -moz-text is technically an anon box
03:07emilioManishearth: so we just go on the first path of the function
03:08Manishearthah
03:08emilioManishearth: so the first condition should be something like if (IsAnonBox(aFrame) && !aFrame.IsTextFrame())
03:08emilioManishearth: or something like that
03:08emilioManishearth: I&#39;ll file a bug to fix it
03:09emilioManishearth: I&#39;m pretty sure I can make it trigger w/o your patches
03:09Manishearththanks
03:09Manishearthbig help :)
03:09Manishearthnow to get back to this leak
03:09emilioManishearth: one less :)
03:09Manishearthyeah
03:10emilioManishearth: it&#39;d be interesting to know why it doesn&#39;t trigger right now, btw, but not sure you want to do that right now :-)
03:10* emilio sleeps
03:11Manishearthyeah
03:11emilioManishearth: please remind me of this assertion if I don&#39;t do so tomorrow
03:11emilioManishearth: or just file the bug and ni? me (on my phone r/n)
03:11Manishearthyeah
08:45ManishearthI knew it!
08:45Manishearthpointer equality
08:45Manishearth!##$
08:45Manishearth:| :| pointer equality matters in our animation code
08:45Manishearthand I messed that up
08:46Manishearthbecause of a .cloned()
08:46Manishearthwhich I gave an &T to instead of an &Arc<T>
08:46Manishearthbecause I typed ... an extra ... asterisk
08:56Manishearthor, no, that wasn&#39;t it :|
09:16ManishearthEq impl on RawOffsetArc. That&#39;s what it was
09:19cynicaldevilManishearth: rollercoaster of emotions!
09:19ManishearthWOOOO it works
09:25Manishearthkay now for that pesky leak
12:07cynicaldevilnox: ping
12:31ripiecesHello. Is servo thought to be compiled for 32 bit embedding too? Asking because I am running into several problems (first was rustc compiler for 686 not downloading (404), then after I fixed it manually a problem with source type being 64 bit while target type is 32 bit in rust-webvr-0.5.0\src\api\openvr\display.rs:138:35).
12:33ripiecesI meant on Windows for 32bit.
12:51Coder206ripieces: Hello! I hope you are doing well! If I recall correctly, Servo works on 64 bit Windows exclusively.
12:52noxcynicaldevil: Pong.
12:53cynicaldevilnox: I found the mistake!
12:53cynicaldevilIt was the &#39;end&#39; method causing problems all along
12:53noxcynicaldevil: Oh.
12:54cynicaldevilnox: For this particular test, the tokenizer ended up in some uncommon state after the feed method returned,
12:54cynicaldevilDue to this, the call to end calls the run method in turn,
12:55cynicaldevilwhich calls the TreeSink methods.
12:56ripiecesCoder206: I am doing okay, hope you too. That&#39;s sad, I was planning to use servo for making a GUI and scripting, but it would have to be in a 32 bit process. But thanks for being clear about it.
12:57noxcynicaldevil: Oh.
13:04Coder206ripieces: My apologies, I was looking over the issues and found this one: https://github.com/servo/servo/issues/17012 It seems to indicate a bug related to the 32 bit version of Windows.
13:04crowbotIssue #17012: Metabug for Rust-related issues - https://github.com/servo/servo/issues/17012
13:09Coder206ripieces: Despite this, I have very minimal experience with the 32 bit version of Servo. I can try to reproduce your issue if you would like :)
13:11ripiecesCoder206: Well I am compiling with Visual Studio 2017 the x64_x86 cross tools command prompt. I don&#39;t want to waste your time, if you think it&#39;s worth it, I could try to open a bug report in the relevant repository myself, but I am pretty new to Servo, just trying to compile it for first time to be honest :D
13:13Coder206ripieces: Are you running Windows 10 32 bit?
13:14Coder206ripieces: No worries. I have about 1 hour so I will see how far I can get :)
13:18ripiecesCoder206: No, I am running on 64 bit of course, but the target process I want to use it for has to be 32 bit, so I am compiling for 32 bit.
13:19ripiecesCoder206: If you can try that would be great, but there is not much I can give you in return for your precious time :(
13:20ripiecesCoder206: as said it first fails at downloading the rustc and lib with 404 (apart from randomly failing to move the extracted directory for some reason, but I can work around all that), the main issue is that type size issue.
13:20noxripieces: Can you file that dowloading bug for starters?
13:20ripiecesYes, sure will do in approx 30 minutes :-)
13:21ripiecesshould I file that on servo repro?
13:21noxAnd maybe file an issue about 32 bit support in general.
13:21noxripieces: Yep.
13:21ripiecesnox: okay
13:22ripiecesCoder206: I really don&#39;t waste your time, maybe you need it for something more important, maybe give s.th. else priority, but thanks for the offer :-)
13:25Coder206ripieces: Ok, my pleasure :) If there is anything else I can do to help out give me a shout :)
13:25* Coder206 waves to nox
13:25noxTalking about waves,
13:25noxI&#39;ll be going to the beach first week of August.
13:25noxI&#39;m hyped, haven&#39;t seen the beach in a decade.
13:25* nox waves back to Coder206.
13:26Coder206nox: Wow! That&#39;s awesome, is it in France? (like Marseille?)
13:26noxCoder206: Fake sea. https://en.wikipedia.org/wiki/Arcachon_Bay
13:26noxBut yeah in France.
13:28Coder206nox: Really cool! :) I am so happy for you! A well deserved break I am sure :D
13:29noxThanks. It&#39;s not like I never take PTO, but I rarely actually move from home when I do and I should do it more often.
13:29Coder206It can definitely do some good for you :)
14:12ripiecesnox: I opened as you said, not sure if I messed it up or not, I hope not.
14:14noxDon&#39;t worry, we can always do triage or whatever.
14:14noxThanks for filing it!
14:15ripiecesThanks for your IRC support :]
14:28emilionox: any chance you could review #17731? (tomorrow is fine if you want)
14:28crowbotPR #17731: style: Respect calc for percentages. - https://github.com/servo/servo/pull/17731
14:29noxemilio: Sure assign it to me I&#39;ll look at it tomorrow.
14:29emilionox: thanks!
14:33noxToo busy petting my cats today.
14:39noxemilio: Hah.
14:39emilionox: oh noes, I typoed in the branch name!
14:39* emilio will never learn how to type embarrassing properly
14:39noxemilio: https://www.youtube.com/watch?v=CfRiCYfEUI0
14:39emiliooh well
14:40emilionox: isn&#39;t embarassing also wrong?
14:40noxIt is, but I was linking the video, I don&#39;t care about its title. :P
14:41emilionox: I&#39;m glad I&#39;m not the only one which has trouble with that word at least :-D
14:41noxAh ah.
14:42emilionox: should we ditch the r? @foo and start using GH review requests?
14:42noxWe could do that.
14:42noxBut I would ask jdm.
14:43emiliocrowbot: tell jdm wdyt about ditching r? @foo and using GH review requests instead?
14:43crowbotthere&#39;s a phone right next to you, but okay
14:47noxemilio: You are a hacker,
14:47noxemilio: you work around bugs efficiently,
14:47noxemilio: just look up the thesaurus and learn a synonym you find adequate. :P
14:47noxemilio: http://www.thesaurus.com/browse/embarrassing
14:48emilionox: lol, fair enough
14:48noxI need to start using &quot;unpropitious&quot;, it looks cool.
14:50emiliohah
15:09Manishearthemilio: reminder ping for that assertion bug
15:10emilioManishearth: I&#39;ve cc&#39;d you in a bug with a patch 2 hours ago :)
15:10emilioManishearth: and I made it block bug 1367904 so you noticed it
15:10firebothttps://bugzil.la/1367904 NEW, manishearth@gmail.com stylo: fuse ServoComputedValues and nsStyleContext into a single allocation
15:11emilioManishearth: bug 1381323 btw
15:11firebothttps://bugzil.la/1381323 NEW, emilio+bugs@crisal.io The ExpectedOwnerForFrame assertion doesn&#39;t account for text frames.
15:11Manishearthemilio: oh
15:11Manishearthemilio: hadn&#39;t checked mail yet
15:11Manishearthi checked try
15:11Manishearthanimations issue fixed!
15:12emilioneat
15:14Manishearthemilio: is there a way to get the tests run individually so i can know which test is leaking?
15:14emilioManishearth: ./mach mochitest foo.html or ./mach reftest foo.html or...
15:15Manishearthemilio: no i mean
15:15Manishearthemilio: right now I know that the entire crashtest suite leaks, I don&#39;t know which of the tests leak
15:16emiliols layout/base/crashtests | xargs ./mach run {} ?
15:16emilioManishearth: IDK how to isolate that apart from running the tests individually
15:16Manishearthemilio: hmmm
15:16Manishearthemilio: was hoping for there to be a way to do that easily on try
15:16emilioManishearth: note that some XBL tests require some prefs, and so on
15:16Manishearthoh well
15:17emilioManishearth: dom.allow_XUL_XBL_for_file = true
15:26Manishearthlooks like theres an XPC leak in 1001237.html
15:26noxI read that as for_life and I was like &quot;hell no&quot;.
18:01Manishearthemilio: the leak is in layout/style/crashtests/1200568-1.html
18:03emilioManishearth: so, you&#39;re leaking style contexts related to detached nodes it seems?
18:04Manishearthemilio: i think
18:04Manishearthi wonder why
18:05Manishearthi wonder if this needs animations to happen
18:05Manishearthi bet it does
18:06Manishearthyep
18:06Manishearthyay, not an animations issue
18:07emilioManishearth: yeah, we don&#39;t do any animation stuff outside of the document
18:07Manishearthemilio: huh
18:08Manishearthemilio: so one of the things that was leaked was an nsStyleQuoteValues
18:08Manishearthbut nsStyleQuoteValues has nothing in its destructor? how does it know?
18:08emilioManishearth: not at my computer, so no idea
18:08Manishearthemilio: not asking you, thinking aloud :)
18:09Manishearththere is no MOZ_COUNT_DTOR()nsStyleQuoteValues
18:36LCPolan&quot;:lktyszxc vb,/
21:32rwaweberNot sure if I&#39;m off my rocker, but would we be able to put most of these configurations in the minion&#39;s config file(.travis/minion)? https://github.com/servo/saltfs/blob/master/Vagrantfile#L76-L80. Just looking primarily to remove the 1.8.3 vagrant restriction
21:33rwaweberTheres probably a handful of other things I&#39;m not considering, so I apologize in advance :P
21:34cpearceI am looking into ways to bring Gecko&#39;s media (C++) code into Servo. I&#39;m wondering whether Servo has the concept of a single &quot;main&quot; thread to which Runnables can be dispatched? At least on a per-tab/JS context basis?
21:37emiliocpearce: yeah, that sounds a lot like the script thread
21:37emiliocpearce: it&#39;s per origin
21:37cpearceemilio: Ah, excellent.
21:38emiliocpearce: https://github.com/servo/servo/blob/master/components/script/script_thread.rs
21:39cpearceemilio: Thanks, that looks like what we need. :)
21:40Manishearthemilio: so, thinking about the leak more, we only hit it if we flush styles via GetComputedStyle
21:40Manishearthfor a non-bound element, GetComputedStyle will call ResolveTransientStyle, yeah?
21:40Manishearthwe&#39;re probably messing that up somehow
21:41emilioManishearth: so... yeah, you&#39;re probably leaking that, somehow
22:00cpearceDoes Servo have something like a queue of timestamped frames for WebGL that the compositor takes care of displaying at the correct time? That might be useful for video playback.
22:03emiliocpearce: not right now... The general canvas architecture is kind of a mess and needs to be reworked (I think mortimer was working on that last time I checked)
22:04cpearceemilio: hmm... OK. so we&#39;ll need to write something in Servo&#39;s compositor to handle a queue of timestamped frames for Video playback.
22:10emiliocpearce: yeah, I think it&#39;s worth checking with mortimer to see what&#39;s going to be the general Canvas/VR architecture, in order to avoid duplicating work
22:11cpearceemilio: OK. thanks. What timezone is mortimer in?
22:11emiliocpearce: He lives in Spain, so CET, I suppose
22:11cpearceOK, thanks.
22:11cpearcemortimer: ping! :)
22:12cpearceI&#39;m in New Zealand, so not a lot of overlap. :(
22:13emiliocpearce: well, it&#39;s kinda late here, so probably he&#39;s asleep already :P
22:13* cpearce nods
22:16cpearceDoes Servo have some kind of &quot;shutdown service&quot; that notifies listeners of an impending shutdown?
22:20cpearceUh.. has anyone tried initializing MSCOM inside Servo? Gecko&#39;s media stack on Windows uses that...
22:20cpearce(at least for H.264 decoding...)
22:30emiliocpearce: So, for shutdown notifications I guess the closer thing we have is Compositor::start_shutting_down and related methods
22:30emiliocpearce: re. MSCOM, I know vlad did a while ago a bunch of COM related stuff for fonts on windows, but not sure how related it is (probably not too much)
22:31cpearceemilio: I suppose if you&#39;re using Direct3D for graphics you&#39;ll be using MSCOM, so it should work somehow.
22:35emiliocpearce: I think we use OpenGL for graphics in all platforms
22:35cpearcehmm....
22:36cpearceMight make it tricky to get hardware accelerated video decoding on Windows then... I don&#39;t know enough about graphics to be sure however.
22:36* emilio neither
22:40emiliocpearce: I think the Gecko people should be making accelerated video decoding work as part of the WR stuff, so probably worth checking with them... I think they&#39;re using D3D + Angle instead of raw OpenGL, so servo should be able to do that without too much trouble
22:40gwcpearce: emilio: the MSCOM stuff was (probably) related to WR using DirectWrite for rasterizing font glyphs. We plan to use ANGLE on Windows (which is what Gecko + WR uses), which will give us access to the HW accel video capabilities.
22:41gwcpearce: emilio: long term we may have a native D3D backend, but ANGLE is certainly the plan for now
22:41emiliogw: sounds good, thanks for checking! That was my suspicion :)
22:42cpearcegw: is WR used by Servo as well?
22:42cpearceAnd is it out of process, or in process?
22:42emiliocpearce: yeah
22:42emiliocpearce: and, it&#39;s supposed to work out of process (in the UI process, not in a separate GPU process), but I don&#39;t think we test that very often..
22:43emiliocpearce: all our testing infra is on single-process afaict, except some very basic smoketests we run on multiprocess
22:43cpearceIt&#39;s a very good idea to do DXVA video decoding OOP, as the drivers are crashy.
22:43gwcpearce: WR is Servo&#39;s only renderer, so yes :) It&#39;s currently in-process, but the API uses the ipc-channel crate, so all messages to/from WR are serialized over an IPC boundary. So at least in theory, it should be relatively simple to get it working OOP again.
22:44gwwow, many acronyms
22:44emilioheh
22:46* emilio goes to sleep for today
22:46cpearcegw: so... does WR have stuff to do async display of a queue of timestamped video frames? I assume it needs something like that for its use in Gecko? Or is that functionality in some other part of the gfx stack?
22:46cpearceemilio: thanks for your help!
22:47emiliocpearce: np, and good luck!
22:51gwcpearce: it doesn&#39;t. WR provides the concept of an &quot;external image&quot;, which is what Gecko is intending to use for video. When you have an external image in the display list, WR runs a callback in the rendering thread when it needs that image data - which can be a CPU-side buffer, a native texture, or (planned) a HW video surface etc. I believe the idea is for Gecko to handle the timestamping etc externally, but probably worth discussing with the people working
22:51gw gecko-side on WR + video.
22:52Manishearthemilio: hmmmm
22:52cpearcegw: OK. thanks.
22:52cpearceI&#39;d guess the timestamp syncing would then run in said callback.
22:52Manishearthemilio: we use an ArenaRefPtr<nsStyleContext> in nsComputedDOMStyle
22:53Manishearthemilio: I suspect that&#39;s where our trouble is coming from
22:53gwcpearce: I think so, yep
22:54gwcpearce: that&#39;s where it all gets a bit hand-wavy for me - I think WR exposes everything the video people have asked for so far though. you can also provide textures in (various) yuv formats which are then decoded by WR shaders during rendering, if that&#39;s useful.
22:56cpearcegw: we&#39;ll be needing to do that yeah. I&#39;ll sync up with the Gecko guys working in the overlap between gfx and media (I&#39;m on the media side, not the gfx side).
22:56gwcpearce: sounds good
22:57mitzikatohey everyone, i&#39;m milica (outreachy intern working on demos). i&#39;m trying to do a visualization of parallel style computation and i need a way to retrieve the node traversal order info from servo (+ computed values for each node). been looking at style/traversal.rs for the past couple of days and learning rust along the way, but i&#39;m kinda stuck. i&#39;d really appreciate if anyone would be up for brainstorming this with me, and maybe explain the
22:57mitzikatoscoping (and ownership? mutability?) rules for ElementData and ElementStyles in style/data.rs. i know it&#39;s sunday, so anytime tomorrow is fine, just (someone pleaaase) ping me :)
23:00Manishearthemilio: there?
23:00Manishearthgot some hardcore C++ SFINAE TMP bonanza design decisions to make
23:04emiliomitzikato: ping me tomorrow and I can try to help :)
23:05emilioManishearth: I&#39;m pretty sure ArenaRefPtr isn&#39;t the problem
23:05emilioManishearth: for non-arena allocated objects it&#39;s just a normal RefPtr
23:05Manishearthemilio: I replaced it with RefPtr and we stopped leaking ServoStyleContexts
23:05Manishearthwe did start leaking GeckoStyleContexts though but that was to be expected
23:06emilioManishearth: that sounds pretty weird...
23:06emilioManishearth: so ArenaRefPtr is basically a RefPtr that gets nulled out when the arena goes away
23:06Manishearthyep
23:07emilioManishearth: so there&#39;s no reason we should use RefPtr there, apart from perf, and there should be no leak
23:08emilioManishearth: it just gets dropped on GC instead of on the presshell going away
23:08mitzikatoemilio: ty :)
23:10Manishearthemilio: that doesn&#39;t sound right
23:11emilioManishearth: in what sense?
23:11Manishearthemilio: oh, sorry, it didn&#39;t leak the GeckoStyleContext when I made it a refptr, it just hit assertions that try to ensure that the style ctx is destroyed in the right time
23:12Manishearthhowever, the ServoStyleContext leaks did go away
23:12Manishearthtrying my ArenaRefPtrTraits solution
23:12Manishearthlet&#39;s see
23:12emilioManishearth: I don&#39;t understand why it leaks, care to explain?
23:12emilioManishearth: ArenaRefPtr(nonArenaAllocatedThing) should work as is
23:13emilioManishearth: why would it leak with your patches?
23:15emiliomitzikato: well, I guess I&#39;ll stay around for a bit after all, so I can PM you and go through it, if this is a good time for you
23:15* emilio waves fist at Manishearth and the timezone differences :)
23:20Manishearthemilio: i think the thing gets registered in the arena
23:20Manishearthgoing to try with some changes and see what happens
23:20emilioManishearth: why doesn&#39;t it leak now then?
23:20Manishearthfor servo style contexts? it does leak no?
23:21emilioManishearth: it doesn&#39;t
23:21emilioManishearth: otherwise we should see orange in our tests right now
23:21emilioManishearth: we definitely create ServoStyleContexts and stash them in that ptr
23:24Manishearthemilio oh right. hm
23:34Manishearthanyway ill wait for this build, which will take a while
23:35* Manishearth goes back to reading about SIDH
23:36emilioManishearth: SIDH? You doing crypto stuff now?
23:36* emilio goes reading
23:50njnI was just about to get back on Stylo memory reporting. I just had to do my weekly triage of Nightly builds... and oh dear it&#39;s a clusterf**k
17 Jul 2017
No messages
   
Last message: 7 days and 17 hours ago