mozilla :: #servo

18 Mar 2017
00:09noxSimonSapin: We should make it take a LayoutThreadInit or something.
00:09noxSome other similar functions do that.
00:09SimonSapinI gave up
00:10SimonSapinitll be per-process lock for now
01:27SimonSapinnox: 13 commits, 72 files +1416/-850 lines, 2:22 am. Its one of those PR
02:48Caspy7I got some disagreement on my question when I asked in #rust, so figured there some more browser folk in here...
02:48Caspy7I'm wondering if safe rust would have avoided this exploit https://www.mozilla.org/en-US/security/advisories/mfsa2017-08/
02:48Caspy7and integer overflow
02:49ManishearthCaspy7: integer overflow in Rust cannot cause security issues
02:49Manishearthshort of very construed cases
02:49Manishearth(namely, mem::forgetting a lot of refcounted objects on 32 bit)
02:49Caspy7Manishearth: cool, thanks for the answer
02:49ManishearthCaspy7: er, safety, not security
02:49Manishearthin general most security issues will be safety related
02:50ManishearthI do not know exact details about this exploit
02:50Manishearthbut it seems like it's safety related
02:50Caspy7it was found in Pwn2Own
02:50Caspy7which is security related
02:51Manishearthright
02:51Manishearththat doesn't tell us if it was a memory safety issue or not
02:51Manishearthusually these things are
02:51Manishearthand from the smell of that API it probably is
02:51Manishearthinteger overflows in rust can cause correctness bugs
03:06Caspy7Manishearth: thanks for your input/thoughts
03:24Manishearthstandups: font-size refactorings (#16016) for bug 1341775
03:24crowbotPR #16016: Add separate specified value for keyword font sizes - https://github.com/servo/servo/pull/16016
03:24standupsOk, submitted #43891 for https://www.standu.ps/user/Manishearth/
03:24firebothttps://bugzil.la/1341775 NEW, manishearth@gmail.com stylo: need to do the right thing with "font-family: monospace" in terms of font size
04:00* bz wonders what timezone canaltinova is in
04:04bzHmmm
04:04bz"*|a:not(*)"
04:04bzDoes stylo not parse that?
04:04* bz wonders why not
04:26Manishearthbz: Istanbul
04:29bzok
04:29* bz sort of assumed, but he was still around at like 6pm Eastern
04:29bzI guess that's not _that_ late in the grand scheme of things
04:30bzMan
04:30bzso much serialization breakage for :not()
04:50Manishearthbz: don't we support :not()?
04:50Manishearthoh
04:50Manishearthserialization
04:50bzWe support :not()
04:51bzif you put something between the parens!
04:51Manishearthbz: I mean, he is a student. When I was a student I never slept at the right time either
04:51bzBut ":not()" with nothing in parens is invalid
04:51bzAnd servo serializes all sorts of things to ":not()" with nothing in the parens
04:51Manishearthbz: should I close the dir issue since https://bugzilla.mozilla.org/show_bug.cgi?id=1341086 is already fixing it?
04:51firebotBug 1341086 NEW, manishearth@gmail.com stylo: need to support the :-moz-system-metric pseudo-class
04:51Manishearthor wait for that to land
04:51Manishearthbz: lol
04:52ManishearthI think I know why
04:52* bz poked briefly, but hasn't sorted it out yet
04:52bzAnyway, you should dup 1348479 to 1341086
04:52bzOr mark dependent, however you prefer
04:52* bz is adjusting the parse_selectors.html expectations for canaltinova
04:53bzTrying to reproduce some of these problems...
04:54Manishearthbz: got an example of a serialization bug?
04:54Manishearthwith the not?
04:54bzUm... I put it in the issue, no?
04:55Manishearthbz: I didn't see the issue :)
04:55Manishearthi only saw the irc comment
04:55Manishearthlooking
04:55bzhttps://github.com/servo/servo/issues/16017
04:55crowbotIssue #16017: Incorrect serialization of :not(*) - https://github.com/servo/servo/issues/16017
04:55Manishearththe ghservo notices render differently for me in irssi and I tend to gloss over them
04:55bzhum
04:56bzso cloning, wtf
04:56bzVery much wtf
05:06Manishearthbz: why exactly is not(*) not allowed to parse?
05:06Manishearthhttps://drafts.csswg.org/selectors-4/#negation
05:06Manishearthspec explicitly says not(*|*) should parse
05:07Manishearthoh nvm
05:07Manishearthno
05:07bz:not(*) is allowed to parse.
05:07bz:not() is not allowed to parse.
05:07Manishearthbz: oh, I see the issue :)
05:08bzThe issue is not the parsing. The issue is the serialization. ;)
05:08Manishearthyep
05:09Manishearthbz: we don't represent the universal selector differently
05:09Manishearthit's stored as a
05:10ManishearthSimpleSelector::LocalName(None, None), basically
05:10Manishearthor something like that
05:10Manishearthbz: should we be preserving *s on parse?
05:11Manishearthe.g. *.class.class should be preserved?
05:11Manishearthor is this just this one issue?
05:12bzSo...
05:12Manishearththe main issue of course is returning bad css on serialization
05:12Manishearthbut I wonder if it's something we should be doing anyway
05:12bzGecko represents * much the same way
05:13bzAs a selector with nothing in it
05:13bzSort of.
05:13Manishearthhm
05:13Manishearthso that seems like something we should special case I guess
05:14bzHere's the thing
05:14bzConsider these three selectors:
05:14bz"*", "*|*", "|*"
05:14bzThose all select different sets of nodes, iirc
05:14* bz is double-checking the spec
05:14Manishearthi think the first is the same as the second?
05:15bzIt's not
05:15bzWhen there's a default namespace
05:15bzBecause then "*" means "any element in the default namespace"
05:15Manishearthoh
05:15bzBut "*|*" means "any element in any namespace"
05:16bz"|*" means "any element in null namespace", of course.
05:16Manishearthoh and we can't just pick one on serialization
05:16Manishearthis see
05:16bzSo for a start, you need to represent those differently.
05:16* bz looks up what Gecko does
05:17bzSo yeah
05:17bzWhen we parse "*" the way we represent it depends on whether there is a default namespace
05:17bzIf there is, we represent it as (that namespace, any tag)
05:18bzIf not, it's represented identically to "*|*"
05:19bzSo backing up a sec....
05:20bzWhen parsing ".foo", I'm pretty sure it's parsed as "*|*.foo"
05:20bzHmm, no
05:20bzit's parsed as "*.foo", so affected by default namespace
05:21bzin Gecko; can't speak for servo
05:21bzAnyway, when serializing....
05:21bzWe call ToString() on the selector
05:21bzThis takes a stylesheet as argument
05:22bzSo we know what the default namespace is
05:22bzAnd can serialize accordingly
05:22bzThen it's just a matter of serializing the shortest form that is semantically equivalent
05:22bzSo if we parse "*.foo" we will serialize to ".foo"
05:23bzAnd if we parse "*|*.foo" and there is no default namespace, we will serialize to ".foo"
05:23bzBut if there _is_ a default namespace, we will serialize to "*|*.foo"
05:23Manishearthlol the debug impls forward to to_css how helpful
05:23bzhmm?
05:24bzbtw, what's servo's representation of selectors?
05:24bzAs in, array, linked list, something else?
05:24Manishearthenums?
05:24Manishearthlots and lots of nested enums
05:24bzno, I mean for the list of selectors.
05:24Manishearthyou rarely see linked lists in rust
05:24Manishearthvecs
05:24bzok
05:25bzAnd when serializing, we just iterate the vec?
05:25Manishearthyes
05:25Manishearthbz: we unwrap the first element and write that, and then serialize (element[i], ",")
05:25bzThe reason I ask is that gecko uses a linked list but we explicitly convert to an array for serialization so we don't end up doing recursion down the list
05:25Manishearther, comma first
05:25bzSorry, I meant this part:
05:26Manishearthi don't see why a linked list at all makes sense in the first place here :)
05:26bza > a > a > a > a > a { }
05:26Manishearthoh
05:26Manishearththat might be an LL
05:26bz(I mean, they're both linked list in Gecko)
05:26Manishearthlet me check
05:27Manishearthbz: so, a a a a will be a LL
05:27Manishearthhttps://doc.servo.org/selectors/parser/struct.ComplexSelector.html
05:27Manishearthas will a > a > a > a
05:27Manishearthhttps://doc.servo.org/selectors/parser/enum.Combinator.html
05:27Manishearthall of those
05:28bzok
05:28bzlinked in which direction?
05:28Manishearthparent links to child
05:28* bz assumes right to left
05:28bzSo if I have "div > span"
05:28Manishearththat's a div -> span
05:28bzit links div -> span
05:28bzUm
05:28Manishearthyeah
05:28bzBut matching goes right to left
05:29Manishearthoh I see the issue
05:29Manishearthhm
05:30bz:-moz-locale-dir
05:30bzDo we have a bug tracking that already?
05:30bz:-moz-lwtheme
05:30bz::-moz-tree-row
05:30Manishearthlocale-dir, yes
05:30Manishearththe other ones no
05:31Manishearthlocale-dir is the same as the system-metric one
05:32Manishearthall the stringy ones get fixed there
05:32bzok
05:32* bz wishes we had dxr or searchfox for servo
05:33Manishearthdoc.servo.org
05:33bzesp. given this penchant for naming all sorts of things the same
05:33Manishearthactually dxr can work with servo
05:33Manishearthhttps://dxr.mozilla.org/servo/
05:33bzaha
05:33bzno
05:33Manishearthxrefs are not too great iirc
05:34bzno search for "this impl of to_css"
05:34Manishearthrust does have support for spitting out analysis that works with dxr
05:34bzI mean, text search I can do with grep too
05:34Manishearthbut it may not work for support
05:34Manishearther, servo
05:34bzBut finding where this to_css thing is defined....
05:34Manishearthnrc was working on this before
05:34Manishearthbz: docs, click src link on impl
05:34Manishearthhttps://doc.servo.org/selectors/parser/struct.ComplexSelector.html
05:34Manishearthscroll down to ToCss, click that
05:35bzhttps://doc.servo.org/cssparser/serializer/trait.ToCss.html#tymethod.to_css
05:35bzoh, on the right, I see
05:35bzok
05:35bzgreat
05:35* bz bookmarks
05:35bzyes
05:35bzthis has the same bug Gecko used to
05:36bz if let Some((ref next, ref combinator)) = self.next {
05:36bz next.to_css(dest)?;
05:36bzThat's recursion down the list
05:36Manishearthyep
05:36* bz files issue
05:36bzIt's a nice clean way to do it
05:36bzAnd leads to stack overflow crashes on the web.
05:39Manishearthbz: couldn't we just store them in array form after parsing?
05:39Manishearthor parse them in array form in the first place
05:39bzhmm
05:40bzthe moz-any stuff landed?
05:40* bz wonders why it's failing for him
05:42bzgah
05:42bzThe thing that landed for :moz-any is wrong!
05:46* bz sighs
05:47bzwell, at least now we will have test coverage...
05:59bzServo's serialization is broken outside :not() too
06:00bzIt's serializing "*|foo" as "foo"
06:00bzwhen there is a default namespace
06:08bzok
06:08bzI think that's all for the failures canaltinova found by fixing test_selectors.html to actually get to the end
06:08* bz sighs
06:08bzSo much failure. ;)
07:24Manishearthheycam: *facepalm*
07:24Manishearththanks
07:24heycam:)
10:11noxWhy do I receive Bugzilla stuff when I'm not in Cc,
10:11noxand how do I make it stop?
10:12noxOh actually I am.
10:57* emilio starts feeling like heycam, with a zillion reviews in queue :(
10:57* emilio just cleared it though
10:58heycamemilio: I'm actually doing pretty well lately with reviews... only 10 at the moment!
10:58heycam(plenty of needinfos though)
10:59emilioheycam: woah! I just woke up with four and cleared them all, but it starts getting tiresome of thinking through all the implications of the changes after the first hour :/
10:59heycamemilio: I'm in #2 position so far this year :) https://pastebin.mozilla.org/8982453
11:00emilioheycam: I can start landing test-pass expectations as r=heycam if that helps you beat smaug ;)
11:00heycamhaha
11:00emilioheycam: I'm actually surprised to be there, I'm not even full-time :D
11:01* emilio can't wait to see his review queue when he is :(
11:02* heycam goes to get some dinner, later
11:02* stshine has none
11:03* emilio throws patches at stshine
11:04emilioheh
11:04emiliostshine: actually there are a few layout cleanups I should do, I can throw them at your queue if you wish ;)
11:05stshineemilio: I'm trying to going through style code, but I'd like to help if I can
11:06stshineemilio: have you enabled incremental restyle now?
11:06emiliostshine: sure, it should be working quite decently, for different amounts of decentness
11:07emiliostshine: we're aware of a few bugs, but a lot of them will conflict so badly with #15890, and it's been sitting there for a while :(
11:07crowbotPR #15890: selectors: Check the bloom filter at most once per complex selector. - https://github.com/servo/servo/pull/15890
11:08stshineemilio: I was reading your bloom filter code today :)
11:09emiliostshine: if you feel like reviewing that one, that'd be super-nice :)
11:09* stshine doesn't think he can judge that for now ...
11:10stshineemilio: What are the layout reviews you were talking about?
11:10emiliostshine: heh, fair enough... will ping Simon a bit harder then ;)
11:11emiliostshine: they're a few cleanups about the layout_thread code, last time I looked we were doing some unneeded stuff here and there, but I don't remember the details r/n
11:11emilioSimonSapin: I know you're busy, but #15890 should be relatively straight-forward, and should unblock other stuff that others can review. If you could review it soon it'd be really nice :)
11:11crowbotPR #15890: selectors: Check the bloom filter at most once per complex selector. - https://github.com/servo/servo/pull/15890
11:11stshineemilio: Maybe I am capable of handing that
11:14SimonSapinemilio: sorry, I dont have bloom filtering "paged in" right now and fixing #16014 is already spilling over PTO
11:14crowbotPR #16014: Per-process lock for CSSOM objects - https://github.com/servo/servo/pull/16014
11:15emilioSimonSapin: ok, will ping heycam then, thanks for answering :)
11:16emiliostshine: sure! I need to run in a few minutes, but if I do them I can tag you for review
11:16* emilio also needs to rebase #13864, and apologies to ferjm for that :(
11:16crowbotPR #13864: script: Try to conform a bit more to "The event loop processing model" - https://github.com/servo/servo/pull/13864
11:17stshineemilio: ciao
11:21stshineIt is strange that we try to handle calc() in Number::parse() .
11:22SimonSapinanything using <number> in specs can be calc()
11:27stshineSimonSapin: But <length> can also be calc() right, so how should calc() be classified?
11:30SimonSapinhttp://doc.servo.org/style/values/specified/length/enum.Length.html also includes a calc variant
11:32stshineOh. Looks like calc() as Length should have an unit but not as a Number
11:33SimonSapinhum, yes?
11:33SimonSapin<number> is unitless
11:43stshineThe resolved type of the expression is determined by the types of the values it contains. <number-token>s are of type <number> or <integer>. A <dimension-token>s type is given by its unit (cm is <length>, deg is <angle>, etc.).
11:43stshineSimonSapin: I guess that is the case ^
11:46SimonSapinstshine: the spec and implementation are the other way around
11:46SimonSapinstshine: the spec is described as &quot;when you find a calc() expression, here is how you determine its type based on whats inside&quot;
11:47SimonSapinstshine: but the implementation is like &quot;ok, here we expect maybe a <number>, so if we find a calc() function well only accept unitless numbers inside&quot;
11:48stshineyep
11:50stshineSo we need a new structure other than f32 to represent a <number> ...
11:51SimonSapinwell, depends
11:52SimonSapincan we always know the result of the calculation at parse time? (unlike <length> where we dont know yet how many px is 1em)
11:52SimonSapinand do we need to preserve the original calc() expression for serialization?
11:52SimonSapinif the answers are &quot;yes&quot; and &quot;no&quot;, then f32 is enough
11:53SimonSapinotherwise, well need an enum with a Calc variant, for the specified value
11:55stshineSimonSapin: It looks like that #15960
11:55crowbotIssue #15960: Number::parse should not simplify calc() to number when parsing - https://github.com/servo/servo/issues/15960
11:56SimonSapinenum it is, then
11:57stshineOr maybe we can handle it as a unitless variant of <length> if this is too much troublesome?
11:58SimonSapinits not a question of whether its troublesome, but rather that theyre different types and Servo uses different Rust types for each CSS value type
11:59SimonSapin(compared to nsCSSValue which does dynamic typing)
12:02stshineHmm
12:02* stshine goes back to digging style crate
15:24emiliofun: thread &#39;main&#39; panicked at &#39;Uknown type after type references: Type(basic_istream<_CharT, _Traits> &(basic_istream<_CharT, _Traits> &, basic_string<_CharT, _Traits, _Alloc> &), kind: FunctionProto, cconv: 1, decl: Cursor( kind: NoDeclFound, loc: builtin definitions, usr: None), canon: Cursor( kind: NoDeclFound, loc: builtin definitions, usr: None)),
15:24emilioCursor(operator>> kind: FunctionTemplate, loc: /usr/bin/../lib64/gcc/x86_64-pc-linux-gnu/6.3.1/../../../../include/c++/6.3.1/bits/basic_string.h:5322:5, usr: Some(&quot;c:@N@std@FT@>3#T#T#Toperator>>#&>@N@std@ST>2#T#T@basic_istream2t0.0t0.1#&>@N@std@N@__cxx11@ST>3#T#T#T@basic_string3S2_S3_t0.2#S0_#&quot;))&#39;, /home/emilio/projects/moz/rust-bindgen/src/ir/context.rs:431
15:25* emilio rages at C++
15:25SimonSapincrowbot: is javascript-url-query-fragment-components.html intermittent
15:25crowbotNo intermittent issues filed with &#39;javascript-url-query-fragment-components.html&#39; in the title
15:48SimonSapinlooks like someone disconnected the macbook on my desk
15:48noxSimonSapin: OH
15:48noxOH
15:48SimonSapinor its ip address changed
16:03emiliofitzgen: ping?
16:04emiliofitzgen: so I&#39;m trying to update bindgen in stylo and failing very hard at doing so
16:05emiliofitzgen: first of all, I&#39;ve encountred template params whose declaration and location is builtin, and your code doesn&#39;t account for them.
16:06emiliofitzgen: with that worked around (returning an opaque type), I clash at &quot;Should have found the template definition one way or another&quot;
16:08emiliofitzgen: (that&#39;s in <chrono>)
16:10emiliofitzgen: with that further worked around, I clash again at the same place, but with: DEBUG - bindgen::ir::ty - from_clang_ty: ItemId(199855), ty: Type(std::ratio<1, 1000000000>, kind: Unexposed, cconv: 100, decl: Cursor( kind: NoDeclFound, loc: builtin definitions, usr: None), canon: Cursor( kind: NoDeclFound, loc: builtin definitions, usr: None)), loc:
16:10emilioCursor(period kind: TypedefDecl, loc: /usr/bin/../lib64/gcc/x86_64-pc-linux-gnu/6.3.1/../../../../include/c++/6.3.1/chrono:720:37, usr: Some(&quot;c:@N@std@N@chrono@N@_V2@S@system_clock@T@period&quot;)) @ /home/emilio/projects/moz/rust-bindgen/src/ir/ty.rs:967
16:15emiliofitzgen: furthermore, trying to make a bit more sense of it all, I arrive to https://github.com/emilio/rust-bindgen/commits/stylo-hax, which gets stuck in the monotone framework (I kinda expected that actually). Previous attempt is at https://github.com/emilio/rust-bindgen/commits/stylo-workarounds.
16:18emiliofitzgen: This is not particularly urgent, but I believe these should be fixed before releasing a new version, and I&#39;m failing to see how to fit the AST dumps I&#39;m seeing in the current model
16:25emiliofitzgen: (current 0.22.1 works fine with clang trunk AFAICT)
16:32emilioSimonSapin: \o/
16:32emilioSimonSapin: should I try to review it this afternoon?
16:55zimiois there a way to print debugging info while running integration tests?
16:55zimioor to run the debugger?
16:56zimioI&#39;m using ./mach test-wpt
17:01SimonSapinemilio: if you feel like it :)
17:03SimonSapinemilio: it affects Cargo.lock, though... do you have push access to autoland?
17:03SimonSapinI hear we have autorevendor now, but it has failed at least once
17:05SimonSapinzimio: ./mach test-wpt --debugger path/to/test.html
17:05zimiothanks
17:08emilioSimonSapin: I do, and otherwise usually sheriffs are pretty helpful at landing autoland patches (or we could open a new bug).
17:08emilioSimonSapin: anyway, I need to run now, but I may be back later and review it :)
17:14SimonSapincool, thanks
17:36jdmzimio: what sort of debugging info are you referring to?
18:39emilioSimonSapin: just had the energy to do a first pass, but looks good over-all! :)
18:43SimonSapinemilio: &quot;I believe the layout thread can access the doc at a given point.&quot; how?
18:43SimonSapinor did you mean can not?
18:43emilioSimonSapin: https://github.com/servo/servo/blob/master/components/layout_thread/lib.rs#L938
18:44emilioSimonSapin: (and we _always_ get one of those before styling anything)
18:44SimonSapinemilio: ScriptReflow is only available in Msg::Reflow
18:45emilioSimonSapin: yeah, but we need to reflow a document to style anything right? I guess the problems are web fonts
18:45SimonSapinAdvanceClockMs, TickAnimations, and ReflowWithNewlyLoadedWebFont also need access to style objects
18:45SimonSapinalthough nothing ever sends ReflowWithNewlyLoadedWebFont as far as I can tell, so that might be dead code
18:46emilioSimonSapin: look at how we store the quirks mode, we could do the same with the lock, couldn&#39;t we?
18:46emilioSimonSapin: i.e., store it at the first reflow (which needs to come earlier than all those, maybe except the web font one), and then assert we have it.
18:48SimonSapinif were sure that these messages only come after the first reflow, that could work
18:48SimonSapinemilio: although, do we only ever have one Document per LayoutThread?
18:49emilioSimonSapin: yep, that&#39;s right as of right now (there&#39;s an issue open to make it per-origin)
18:49SimonSapindoes navigation create a new layout thread?
18:50emilioSimonSapin: huh, good question
18:57emilioSimonSapin: I think only if it&#39;s to a different origin, if I&#39;m reading hte constellation code correctly
18:58SimonSapinemilio: I think Id rather figure this out in a follow-up :)
18:58emilioSimonSapin: sounds fine :)
18:58emilioSimonSapin: but please do file an issue
18:58SimonSapinI will
18:58SimonSapinwriting responses to your comments
19:08SimonSapinemilio: how does StylesheetGuards sound? Instead of ReadGuards
19:10emilioSimonSapin: sgtm
19:22Manishearthemilio: there?
19:23emilioOn my phone r/n
19:23Manishearthemilio: regarding your comment about only keeping GetDocumentState public -- we can&#39;t
19:23Manishearthit has to be this two-way flaky thing
19:23Manishearthbecause Servo can&#39;t update the bits off main thread
19:23emilioManishearth: well, de also need the update thing, sure
19:24Manishearthso what&#39;s flaky about it?
19:24Manishearthoh well we can assert that it&#39;s set
19:24emilioManishearth: but we shouldn&#39;t need the getPossiblyOutdatedXXX
19:24Manishearthim not sure
19:24emilioManishearth: right
19:25Manishearthemilio: but there are no bits that always get set in the lazy resolution
19:26Manishearthemilio: maybe I&#39;m misunderstanding what you want me to do here
19:27emilioManishearth: isn&#39;t there an mGotDocumentState?
19:27Manishearthemilio: oh i see
19:28Manishearthemilio: so what should I do here? I can make getpossiblystale assert
19:28Manishearthbut I can&#39;t get rid of it entirely
19:28Manishearthwe need a const method
19:29Manishearthand we shouldn&#39;t overload here
19:33Manishearthemilio: anyway in that case we don&#39;t need the NS_IsMainThread
19:37emilioManishearth: well, you could just keep getdocumentstate actually
19:37Manishearthemilio: no?
19:37Manishearthemilio: it has other callers
19:37emilioAnd just assert at the time of computing it
19:37Manishearthwhich do want to mutate it
19:38emilioManishearth: I mean something like: if (!mGotDocumentState & FOO) { AssertIsMainThread(); computeFoo(); }
19:39Manishearthemilio: I&#39;d prefer to keep the asserty one separate
19:39Manishearthyou&#39;re turning a compile time error into a runtime one
19:39emilioManishearth: then return mDocumentState
19:39emilioManishearth: which compile error]
19:39Manishearth?
19:39emilio*?
19:39Manishearthemilio: the compile error of calling a non-const function
19:39Manishearththat&#39;s how we caught this in the first place
19:40Manishearthwithout it you will just call GetDocumentState, and have asserts happening
19:40Manishearthwith it, you try calling it, it forces you to use the const version.
19:40emilioManishearth: oh, I guess then
19:40Manishearthand, I mean, technically we shouldn&#39;t be mutating consts anyway, it&#39;s not just a thread thing
19:41emilioManishearth: well, there&#39;s logical constness too, caching stuff in a const method is normal AFAICT
19:42emilioManishearth: (in a single-threaded program ofc)
19:42Manishearthah
19:42Manishearthfair
19:42Manishearthstill, compile time errors are great
19:55SimonSapinemilio: pushed fixup commits are responded to your comments
20:00SimonSapinManishearth: do we have autorevendor now?
20:02bholleyManishearth: I _ think_ glob implemented that
20:03ManishearthSimonSapin: yes
20:03bholleyer, SimonSapin
20:03bholleySimonSapin: question on the lock stuff - should we use an AtomicRefCell instead of an RwLock for stylo?
20:03Manishearthidk if deployed
20:03SimonSapinbholley: maybe?
20:03bholleySimonSapin: IIUC we&#39;ll never have contention. The advantages are (1) incorrect behavior manifests itself as a panic rather than a hang, which is _way_ easier to diagnose on CI, and (2) the borrows are cheaper
20:04bholley(half the cost, IIRC)
20:05bholleySimonSapin: given that your stuff is all wrapped in a nice API, maybe conditionally replacing the RwLock with an AtomicRefCell would be an easy change?
20:05SimonSapinbholley: Ive spent some time today debugging deadlocks, so that would have helped there
20:05bholleySimonSapin: I&#39;m assuming servo still needs the RwLock behavior
20:05SimonSapinyeah its an easy change
20:05SimonSapinoh, conditionally compiled?
20:06SimonSapinbholley: is AtomitRefCell the same as RwLock::try_read and try_write?
20:06bholleySimonSapin: well, try_read().unwrap()
20:07bholleySimonSapin: i.e. it panics
20:07SimonSapinright
20:07bholleySimonSapin: a la RefCell
20:07bholleySimonSapin: if Servo doesn&#39;t actually need the blocking behavior, then we can use AtomicRefCell everywhere
20:07bholleySimonSapin: which sounds a lot nicer to me! Deadlocks suck
20:08SimonSapinit does sound nicer, but Im not sure servo doesnt need it
20:08SimonSapinthe TickAnimation event is not synchronized with script
20:10SimonSapinbholley: is every C++ -> Rust FFI call in stylo made on the main thread?
20:10bholleySimonSapin: mmm
20:10bholleySimonSapin: probably, though it&#39;s possible that a style worker thread might do an FFI call into C++ and that might call back into Rust
20:11bholleySimonSapin: but I don&#39;t think we have any of that
20:11bholleySimonSapin: in general Gecko only interacts with stylo on the main thread
20:11SimonSapinbut those wouldnt do writes, and are happening while script is paused
20:11bholleySimonSapin: yeah sure
20:11bholleySimonSapin: I think the answer to your question is &quot;it&#39;s fine&quot;
20:11bholley:-)
20:11SimonSapinso yeah, I think atomic refcell is ok for stylo at least
20:12bholleySimonSapin: \o/ - panics are _so_ much nicer than deadlocks :-)
20:12SimonSapinindeed
20:12SimonSapinI spent some time today staring at gdb track traces today and not finding the deadlocked thread
20:12SimonSapins/track/stack/
20:13SimonSapin(I ended up finding the bug by bise
20:13bholleySimonSapin: and that&#39;s when they&#39;re locally reproducible
20:13SimonSapinbisecting the JS code instead)
20:13bholleySimonSapin: When it only reproduces on CI, then you get stuff like https://bugzilla.mozilla.org/show_bug.cgi?id=1346232
20:14firebotBug 1346232 NEW, dmajor@mozilla.com stylo: Reftests application timed out after 330 seconds with no output
20:14SimonSapinyeah, both deadlocks I fixed today were reliable to reproduce: same thread acquiring the lock for both read and write
20:14bholley \_()_/
20:15bholleySimonSapin: (we have this same issue with ElementData - occasionally we hit the borrow panics when we introduce a bug, but they&#39;re easy to spot and we get a stack)
20:15Manishearthemilio: re: mbrubeck&#39;s IsSignificantChild patches: why can&#39;t that funciton just be marked const?
20:15ManishearthI don&#39;t see a difference there
20:15SimonSapinso, yes to atomic refcell for shared lock (and maybe rename some things), but in a follow up because this PR is already big :)
20:15Manishearthalso wait what how did this compile even
20:17Manishearthoh of course
20:17ManishearthI see
20:19bholleySimonSapin: sounds great! Thanks for pushing this over the line
20:19* bholley lunches
20:19SimonSapinbholley: AtomicRefCell would need new API
20:19bholleySimonSapin: what API?
20:19SimonSapineither something equivalent to RwLock::raw_unlock_read and raw_unlock_write
20:20SimonSapinused after mem::forget(AtomicRef)
20:20SimonSapinor some way to get *const AtomicRefCell from &AtomicRef or &AtomicRefMut
20:21SimonSapinor some other pointer to compare to make sure its the same refcell
20:21bholleySimonSapin: ok - can you file a bug and NI me? I can probably add the API later today when I get back from lunch
20:21SimonSapinwithout either, guards would need to store two pointers instead of one
20:22SimonSapinbholley: on https://github.com/bholley/atomic_refcell ? bugzilla?
20:32bholleySimonSapin: bugzilla is probably best
20:34canaltinovaManishearth: Updated the test expectations in bug 1341642. Is it still r+? Shall I open a servo pr for it?
20:34firebothttps://bugzil.la/1341642 NEW, canaltinova@gmail.com stylo: Need support for the various -moz stuff used for alt text
20:36Manishearthcanaltinova: yes
20:36Manishearthcanaltinova: gecko r+ carries over by default
20:37canaltinovaManishearth: ah, okay. Will open a pr now
20:39Manishearthfeel free to self-r
20:39Manishearthwell, r=whoeverreviewedit
20:40bholleySimonSapin: (alternatively, if you know exactly what API you need, you can just PR it to me)
20:40bholleySimonSapin: the AtomicRefCell impl is very small
20:44canaltinovaManishearth: oh, saltfs highstated? thanks :)
20:44SimonSapinbholley: Im already 1 day late on PTO :)
21:26canaltinovaManishearth: ^ it&#39;s merged, I need a push on the m-c side :)
22:05Manishearthcanaltinova: on it
22:08canaltinovaManishearth: thanks
22:09Manishearthcanaltinova: oh, did you already roll bz&#39;s patch into yours?
22:09canaltinovaManishearth: yes, why shouldn&#39;t I?
22:09Manishearthcanaltinova: no, that&#39;s okay
22:10Manishearthcanaltinova: I just didn&#39;t expect that and `patch` was throwing errors :)
22:10Manishearthjust wanted to check that nothing was left behind
22:10Manishearthpushed
22:11canaltinovaManishearth: ok, thanks
22:16Manishearthcanaltinova: if you still have time, any idea what you&#39;re looking into next?
22:20canaltinovaManishearth: I really should finish this will-change property(#15813). I should implement sugar for nsCOMArray. But I can&#39;t say that I know how to implement it :/
22:20crowbotPR #15813: [WIP] Implement parsing/serialization and glue for will-change property - https://github.com/servo/servo/pull/15813
22:21Manishearthif nscomarray is used exactly once it&#39;s pretty ok to not do any sugar and directly write unsafe stuff
22:21canaltinovawell, I couldn&#39;t find a usage for it
22:24canaltinovaManishearth: So I&#39;ll need some bindings for it. Probably it will look like nsTArray&#39;s functions
22:25KiChjangcan we remove oOIgnitionOo from the list of people who&#39;s looking for something to work on?
22:25KiChjangand jdhorwitz as well
22:25KiChjangboth of them were pinged multiple times on issues and didn&#39;t respond at all
22:27Manishearthcanaltinova: yeah it&#39;s probably similar
19 Mar 2017
No messages
   
Last message: 153 days and 15 hours ago