Closed Bug 1474959 Opened 2 years ago Closed 2 years ago

thread '<unnamed>' panicked at 'assertion failed: self.writing_mode.map_or(true, |wm| wm == writing_mode)', servo/components/style/rule_cache.rs:40:9

Categories

(Core :: CSS Parsing and Computation, defect, P2)

defect

Tracking

()

RESOLVED FIXED
mozilla63
Tracking Status
firefox-esr52 --- unaffected
firefox-esr60 --- unaffected
firefox61 --- unaffected
firefox62 --- unaffected
firefox63 --- fixed

People

(Reporter: tsmith, Assigned: emilio)

References

(Blocks 1 open bug)

Details

(Keywords: assertion, regression, testcase, Whiteboard: [fuzzblocker])

Attachments

(8 files)

Attached file testcase.html
Reduced with m-c:
BuildID=20180711094246
SourceStamp=aff060ad3204234adae2d59b3776207c6687ebfc

The fuzzers have been hitting this issue frequently since yesterday afternoon.

thread '<unnamed>' panicked at 'assertion failed: self.writing_mode.map_or(true, |wm| wm == writing_mode)', servo/components/style/rule_cache.rs:40:9

0|0|firefox|mozalloc_abort|hg:hg.mozilla.org/mozilla-central:memory/mozalloc/mozalloc_abort.cpp:aff060ad3204234adae2d59b3776207c6687ebfc|34|0x5
0|1|firefox|abort|hg:hg.mozilla.org/mozilla-central:memory/mozalloc/mozalloc_abort.cpp:aff060ad3204234adae2d59b3776207c6687ebfc|81|0x5
0|2|libxul.so|_ZN11panic_abort18__rust_start_panic5abort17hfb98714efe360e0fE|git:github.com/rust-lang/rust:src/libpanic_abort/lib.rs:915ac660268eeff1cee8f6e59670f19b501a143e|61|0x6
0|3|libxul.so|__rust_start_panic|git:github.com/rust-lang/rust:src/libpanic_abort/lib.rs:915ac660268eeff1cee8f6e59670f19b501a143e|56|0x6
0|4|libxul.so|rust_panic|git:github.com/rust-lang/rust:src/libstd/panicking.rs:915ac660268eeff1cee8f6e59670f19b501a143e|559|0x5
0|5|libxul.so|_ZN3std9panicking20rust_panic_with_hook17h608586f043d70222E|git:github.com/rust-lang/rust:src/libstd/panicking.rs:915ac660268eeff1cee8f6e59670f19b501a143e|531|0xb
0|6|libxul.so|_ZN3std9panicking11begin_panic17hd4c46bd0d5679291E|git:github.com/rust-lang/rust:src/libstd/panicking.rs:915ac660268eeff1cee8f6e59670f19b501a143e|445|0x20
0|7|libxul.so|_ZN5style10properties9longhands25border_inline_start_width16cascade_property17h4f63a2f0df9efb44E|hg:hg.mozilla.org/mozilla-central:servo/components/style/rule_cache.rs:aff060ad3204234adae2d59b3776207c6687ebfc|40|0x18
0|8|libxul.so|_ZN5style10properties7cascade17h4e09fd1ac4d72296E|s3:gecko-generated-sources:8d1b5b5f5fefe1eef0302fcdbb5cf0f017338c0a0486ab37aae77633b33f6f923f5ba176c3f5d79090ca356d3d10861290d1f5f4b74dae52a16bb723ad53b6ce/toolkit/library/x86_64-unknown-linux-gnu/debug/build/style-4661bea949fb2a68/out/properties.rs:|82143|0x12
0|9|libxul.so|_ZN5style7stylist7Stylist25cascade_style_and_visited17h555512092bbfc5c8E|hg:hg.mozilla.org/mozilla-central:servo/components/style/stylist.rs:aff060ad3204234adae2d59b3776207c6687ebfc|904|0x49
0|10|libxul.so|_ZN109_$LT$style..style_resolver..StyleResolverForElement$LT$$u27$a$C$$u20$$u27$ctx$C$$u20$$u27$le$C$$u20$E$GT$$GT$25cascade_style_and_visited17h202bbdaacd0e3f63E|hg:hg.mozilla.org/mozilla-central:servo/components/style/style_resolver.rs:aff060ad3204234adae2d59b3776207c6687ebfc|306|0x28
0|11|libxul.so|_ZN109_$LT$style..style_resolver..StyleResolverForElement$LT$$u27$a$C$$u20$$u27$ctx$C$$u20$$u27$le$C$$u20$E$GT$$GT$21cascade_primary_style17hd572f170bb70d550E|hg:hg.mozilla.org/mozilla-central:servo/components/style/style_resolver.rs:aff060ad3204234adae2d59b3776207c6687ebfc|216|0x17
0|12|libxul.so|_ZN109_$LT$style..style_resolver..StyleResolverForElement$LT$$u27$a$C$$u20$$u27$ctx$C$$u20$$u27$le$C$$u20$E$GT$$GT$35cascade_styles_with_default_parents28_$u7b$$u7b$closure$u7d$$u7d$17hf6ff545a8166e9cfE|hg:hg.mozilla.org/mozilla-central:servo/components/style/style_resolver.rs:aff060ad3204234adae2d59b3776207c6687ebfc|336|0x8
0|13|libxul.so|_ZN109_$LT$style..style_resolver..StyleResolverForElement$LT$$u27$a$C$$u20$$u27$ctx$C$$u20$$u27$le$C$$u20$E$GT$$GT$35cascade_styles_with_default_parents17h684fe6583d24215dE|hg:hg.mozilla.org/mozilla-central:servo/components/style/style_resolver.rs:aff060ad3204234adae2d59b3776207c6687ebfc|102|0xb
0|14|libxul.so|_ZN5style9traversal13compute_style17hccde0c3c8a613b66E|hg:hg.mozilla.org/mozilla-central:servo/components/style/traversal.rs:aff060ad3204234adae2d59b3776207c6687ebfc|663|0x5
0|15|libxul.so|_ZN168_$LT$style..gecko..traversal..RecalcStyleOnly$LT$$u27$recalc$GT$$u20$as$u20$style..traversal..DomTraversal$LT$style..gecko..wrapper..GeckoElement$LT$$u27$le$GT$$GT$$GT$16process_preorder17h652c998a879b70d7E|hg:hg.mozilla.org/mozilla-central:servo/components/style/traversal.rs:aff060ad3204234adae2d59b3776207c6687ebfc|434|0x15
0|16|libxul.so|_ZN5style6driver12traverse_dom17h00cd87b45169009eE|hg:hg.mozilla.org/mozilla-central:servo/components/style/driver.rs:aff060ad3204234adae2d59b3776207c6687ebfc|111|0x1b
0|17|libxul.so|_ZN10geckoservo4glue16traverse_subtree17h6effab5464b8672dE|hg:hg.mozilla.org/mozilla-central:servo/ports/geckolib/glue.rs:aff060ad3204234adae2d59b3776207c6687ebfc|293|0xe
0|18|libxul.so|Servo_TraverseSubtree|hg:hg.mozilla.org/mozilla-central:servo/ports/geckolib/glue.rs:aff060ad3204234adae2d59b3776207c6687ebfc|351|0x11
0|19|libxul.so|mozilla::ServoStyleSet::StyleDocument(mozilla::ServoTraversalFlags)|hg:hg.mozilla.org/mozilla-central:layout/style/ServoStyleSet.cpp:aff060ad3204234adae2d59b3776207c6687ebfc|996|0x1d
0|20|libxul.so|mozilla::RestyleManager::DoProcessPendingRestyles(mozilla::ServoTraversalFlags)|hg:hg.mozilla.org/mozilla-central:layout/base/RestyleManager.cpp:aff060ad3204234adae2d59b3776207c6687ebfc|2965|0x12
0|21|libxul.so|mozilla::PresShell::DoFlushPendingNotifications(mozilla::ChangesToFlush)|hg:hg.mozilla.org/mozilla-central:layout/base/PresShell.cpp:aff060ad3204234adae2d59b3776207c6687ebfc|4289|0x19
0|22|libxul.so|nsRefreshDriver::Tick(long, mozilla::TimeStamp)|hg:hg.mozilla.org/mozilla-central:layout/base/nsRefreshDriver.cpp:aff060ad3204234adae2d59b3776207c6687ebfc|1934|0x5
0|23|libxul.so|mozilla::RefreshDriverTimer::TickRefreshDrivers(long, mozilla::TimeStamp, nsTArray<RefPtr<nsRefreshDriver> >&)|hg:hg.mozilla.org/mozilla-central:layout/base/nsRefreshDriver.cpp:aff060ad3204234adae2d59b3776207c6687ebfc|303|0xb
0|24|libxul.so|mozilla::RefreshDriverTimer::Tick(long, mozilla::TimeStamp)|hg:hg.mozilla.org/mozilla-central:layout/base/nsRefreshDriver.cpp:aff060ad3204234adae2d59b3776207c6687ebfc|322|0xf
0|25|libxul.so|mozilla::VsyncRefreshDriverTimer::RefreshDriverVsyncObserver::TickRefreshDriver(mozilla::TimeStamp)|hg:hg.mozilla.org/mozilla-central:layout/base/nsRefreshDriver.cpp:aff060ad3204234adae2d59b3776207c6687ebfc|767|0x5
0|26|libxul.so|mozilla::VsyncRefreshDriverTimer::RefreshDriverVsyncObserver::NotifyVsync(mozilla::TimeStamp)|hg:hg.mozilla.org/mozilla-central:layout/base/nsRefreshDriver.cpp:aff060ad3204234adae2d59b3776207c6687ebfc|579|0xc
0|27|libxul.so|mozilla::layout::VsyncChild::RecvNotify(mozilla::TimeStamp const&)|hg:hg.mozilla.org/mozilla-central:layout/ipc/VsyncChild.cpp:aff060ad3204234adae2d59b3776207c6687ebfc|68|0x9
0|28|libxul.so|mozilla::layout::PVsyncChild::OnMessageReceived(IPC::Message const&)|s3:gecko-generated-sources:0c7cf777c2ff93c34ff1546f677320cb1229427e6947e87c6fa76720f9b9c5b6a4a4d036521ed9a643f4fa5e10a57d8748e2532d47fce8282aa653340c0c00ff/ipc/ipdl/PVsyncChild.cpp:|167|0xc
0|29|libxul.so|mozilla::ipc::MessageChannel::DispatchAsyncMessage(IPC::Message const&)|hg:hg.mozilla.org/mozilla-central:ipc/glue/MessageChannel.cpp:aff060ad3204234adae2d59b3776207c6687ebfc|2134|0x6
0|30|libxul.so|mozilla::ipc::MessageChannel::DispatchMessage(IPC::Message&&)|hg:hg.mozilla.org/mozilla-central:ipc/glue/MessageChannel.cpp:aff060ad3204234adae2d59b3776207c6687ebfc|2064|0xb
0|31|libxul.so|mozilla::ipc::MessageChannel::RunMessage(mozilla::ipc::MessageChannel::MessageTask&)|hg:hg.mozilla.org/mozilla-central:ipc/glue/MessageChannel.cpp:aff060ad3204234adae2d59b3776207c6687ebfc|1910|0xb
0|32|libxul.so|mozilla::ipc::MessageChannel::MessageTask::Run()|hg:hg.mozilla.org/mozilla-central:ipc/glue/MessageChannel.cpp:aff060ad3204234adae2d59b3776207c6687ebfc|1943|0xc
0|33|libxul.so|nsThread::ProcessNextEvent(bool, bool*)|hg:hg.mozilla.org/mozilla-central:xpcom/threads/nsThread.cpp:aff060ad3204234adae2d59b3776207c6687ebfc|1051|0x15
0|34|libxul.so|NS_ProcessNextEvent(nsIThread*, bool)|hg:hg.mozilla.org/mozilla-central:xpcom/threads/nsThreadUtils.cpp:aff060ad3204234adae2d59b3776207c6687ebfc|519|0x11
0|35|libxul.so|mozilla::ipc::MessagePump::Run(base::MessagePump::Delegate*)|hg:hg.mozilla.org/mozilla-central:ipc/glue/MessagePump.cpp:aff060ad3204234adae2d59b3776207c6687ebfc|97|0xa
0|36|libxul.so|MessageLoop::RunInternal()|hg:hg.mozilla.org/mozilla-central:ipc/chromium/src/base/message_loop.cc:aff060ad3204234adae2d59b3776207c6687ebfc|325|0x17
0|37|libxul.so|MessageLoop::Run()|hg:hg.mozilla.org/mozilla-central:ipc/chromium/src/base/message_loop.cc:aff060ad3204234adae2d59b3776207c6687ebfc|318|0x8
0|38|libxul.so|nsBaseAppShell::Run()|hg:hg.mozilla.org/mozilla-central:widget/nsBaseAppShell.cpp:aff060ad3204234adae2d59b3776207c6687ebfc|158|0xd
0|39|libxul.so|XRE_RunAppShell()|hg:hg.mozilla.org/mozilla-central:toolkit/xre/nsEmbedFunctions.cpp:aff060ad3204234adae2d59b3776207c6687ebfc|920|0x11
0|40|libxul.so|mozilla::ipc::MessagePumpForChildProcess::Run(base::MessagePump::Delegate*)|hg:hg.mozilla.org/mozilla-central:ipc/glue/MessagePump.cpp:aff060ad3204234adae2d59b3776207c6687ebfc|269|0x5
0|41|libxul.so|MessageLoop::RunInternal()|hg:hg.mozilla.org/mozilla-central:ipc/chromium/src/base/message_loop.cc:aff060ad3204234adae2d59b3776207c6687ebfc|325|0x17
0|42|libxul.so|MessageLoop::Run()|hg:hg.mozilla.org/mozilla-central:ipc/chromium/src/base/message_loop.cc:aff060ad3204234adae2d59b3776207c6687ebfc|318|0x8
0|43|libxul.so|XRE_InitChildProcess(int, char**, XREChildData const*)|hg:hg.mozilla.org/mozilla-central:toolkit/xre/nsEmbedFunctions.cpp:aff060ad3204234adae2d59b3776207c6687ebfc|746|0x8
0|44|firefox|content_process_main(mozilla::Bootstrap*, int, char**)|hg:hg.mozilla.org/mozilla-central:ipc/contentproc/plugin-container.cpp:aff060ad3204234adae2d59b3776207c6687ebfc|50|0x14
0|45|firefox|main|hg:hg.mozilla.org/mozilla-central:browser/app/nsBrowserApp.cpp:aff060ad3204234adae2d59b3776207c6687ebfc|287|0x11
0|46|libc-2.23.so||||0x20830
0|47|firefox|MOZ_ReportAssertionFailure|hg:hg.mozilla.org/mozilla-central:mfbt/Assertions.h:aff060ad3204234adae2d59b3776207c6687ebfc|164|0x5
Flags: in-testsuite?
Priority: -- → P3
A fuzzer blocker is probably worth a higher priority.
Priority: P3 → P2
This seems to a regression from bug 1472567 according to mozregression, but applying that patch locally doesn't seem to trigger this bug, so I reran the bisect, and but got the exactly same result. So probably there are some patches between my local tree and that bug broke some invariant that the patch relies on.
Blocks: 1472567
Keywords: regression
Oh wait, actually I can reproduce it locally...
Flags: needinfo?(emilio)
Ah, I see what's going on. We don't cascade writing-mode / etc for :visited, which means that the visited style ends up with a different WritingMode.

I can fix this as soon as I find some time, if you don't get to it before, Xidorn. I still have a flight remaining though...
Oh, I see, good catch! I can fix it and you will get a patch to review when your flight finishes :)
Flags: needinfo?(emilio)
Assignee: nobody → xidorn+moz
It's actually a bit trickier than I initially thought.

The problem here is that, we need the writing mode of the element to resolve the logical properties. However, we don't know the writing mode of the element for visited style because we compute visited style before the element style.

We also cannot cascade writing mode properties for visited style because that would cause problems (both behavior difference on visual result and this assertion) when :visited has a different writing mode than the element style.

I guess that means we would actually need to cascade the element style first then visited style.
(In reply to Xidorn Quan [:xidorn] UTC+10 from comment #6)
> We also cannot cascade writing mode properties for visited style because
> that would cause problems (both behavior difference on visual result and
> this assertion) when :visited has a different writing mode than the element
> style.

I'm not convinced this is actually a problem:  if the :visited style has a different writing-mode, it might swap which border-color is on which side... but that's not detectable in computed style, so why would it be a problem?
(In reply to David Baron :dbaron: ⌚UTC-7 from comment #7)
> I'm not convinced this is actually a problem:  if the :visited style has a
> different writing-mode, it might swap which border-color is on which side...
> but that's not detectable in computed style, so why would it be a problem?

This is not a problem in terms of privacy because of the reason you said.

But it is a problem that it doesn't match what the spec would want (because writing-mode etc. shouldn't apply via :visited), and internally it doesn't fix the assertion in this bug for all cases (because we would end up having a wrong rule condition when we start cascading the element style).
Where does the spec require not doing that?
The spec may not require not doing that, but it would be weird to apply color with a different writing mode than other stuff. And we need the correct behavior to appease the assertion anyway.
Comment on attachment 8991595 [details]
Bug 1474959 part 1 - Have StyleBuilder return a UniqueArc.

https://reviewboard.mozilla.org/r/256520/#review263376

::: servo/components/style/properties/gecko.mako.rs:246
(Diff revision 1)
>          pseudo_ty: structs::CSSPseudoElementType,
>          pseudo_tag: *mut structs::nsAtom
> -    ) -> Arc<ComputedValues> {
> +    ) -> UniqueArc<ComputedValues> {
>          let arc = {
> -            let arc: Arc<ComputedValues> = Arc::new(uninitialized());
> +            let arc: UniqueArc<ComputedValues> = UniqueArc::new(uninitialized());
>              bindings::Gecko_ComputedStyle_Init(&arc.0 as *const _ as *mut _,

nit: While you're here maybe fix the indentation?
Attachment #8991595 - Flags: review?(emilio) → review+
Comment on attachment 8991595 [details]
Bug 1474959 part 1 - Have StyleBuilder return a UniqueArc.

https://reviewboard.mozilla.org/r/256520/#review263376

> nit: While you're here maybe fix the indentation?

This will be fixed in bug 1475229.
Comment on attachment 8991596 [details]
Bug 1474959 part 2 - Set visited_style and visited bit after ComputedValues is built.

https://reviewboard.mozilla.org/r/256522/#review263654

Behavior-wise looks fine, but it seems to me that it's a bit unfortunate that we need to care about visited explicitly in more cases... I'm leaving the r? flag for now because I want to take a closer look at this during the weekend or next monday.

::: servo/ports/geckolib/glue.rs:3088
(Diff revision 1)
> -            doc_data.stylist.device(),
> +        let mut style = StyleBuilder::for_inheritance(
> +            device,
>              Some(styles.primary()),
>              Some(pseudo),
> -        ).build().shareable()
> +        ).build();
> +        style.inherit_visited(device, styles.primary());

It looks a bit unfortunate that for_inheritance no longer can do visited correctly and needs this...

I want to take a closer look at this on monday to see if I manage to figure out a way to avoid this.

It's also somewhat unfortunate that mBits is mutated while it's a `const` in C++...

I think we may want to try to move the visited cascading to `cascade` / `apply_declarations`, and make a single `StyleBuilder` handle both visited and  unvisited styles internally.

That'd be cleaner I think.
(In reply to Emilio Cobos Álvarez (:emilio) from comment #18)
> I think we may want to try to move the visited cascading to `cascade` /
> `apply_declarations`, and make a single `StyleBuilder` handle both visited
> and  unvisited styles internally.

`cascade` may work, while `apply_declarations` is already way long and complicated, and I don't think it's a good idea to put any extra level of logic into that...
Sorry I couldn't get to this today... It's on my list for tomorrow :(
Comment on attachment 8991599 [details]
Bug 1474959 part 5 - Add a visited reftest for different logical border color with different writing modes.

https://reviewboard.mozilla.org/r/256528/#review264376

This looks great, thanks!
Attachment #8991599 - Flags: review?(emilio) → review+
I think this would be a bit easier to understand and maintain, wdyt Xidorn?

The extra cascade_rules declaration is to avoid multiple monomorphizations of the apply_declarations function, but let me know if you don't think it's worth it.

We can remove a style bit as well with this patch which I'll submit separately in a second.
Attachment #8992611 - Flags: review?(xidorn+moz)
It was only used by the rule cache, and we handled it badly at some places (though it didn't cause correctness issues).

This patch takes advantage of the previous patch handling the visited in `apply_declarations` to avoid using the rule cache for visited style computation.
Attachment #8992612 - Flags: review?(xidorn+moz)
Comment on attachment 8992611 [details] [diff] [review]
Push visited style computation a bit further down.

Review of attachment 8992611 [details] [diff] [review]:
-----------------------------------------------------------------

OK, I guess it's the better way to fix this. I don't think it's really easier to understand but it's a smaller change anyway.

r=me with the issues below addressed.

::: servo/components/style/properties/properties.mako.rs
@@ +3702,5 @@
>  where
>      E: TElement,
>  {
>      debug_assert_eq!(parent_style.is_some(), parent_style_ignoring_first_line.is_some());
>      let empty = SmallBitVec::new();

It's not clear to me why do you move `property_restriction` into `rule_declarations_iterator` but not this one. I have a reverse feeling that this should probably go local into the iterating function, and restriction should probably be passed in instead.

Actually it's not totally clear to me why you need to split this function at all. I don't see any other place using `rule_declarations_iterator`. Is there?

@@ +3776,2 @@
>          Some(parent_style) => {
> +            (parent_style, layout_parent_style.unwrap_or(parent_style))

I find you having both `layout_parent` and `layout_parent_style` around very confusing.

If the non-Option then layout_parent_style (now layout_parent) is only used when we call StyleAdjuster, can we move the handling of that around StyleAdjuster::adjust instead? That should leave just one `layout_parent_style` around.

::: servo/components/style/stylist.rs
@@ +848,3 @@
>          // We need to compute visited values if we have visited rules or if our
>          // parent has visited values.
> +        let visited_rules = match inputs.visited_rules.as_ref() {

This may be more concise:
```
let visited_rules = input.visited_rules.as_ref().or_else(|| {
    parent_style.and_then(|s| s.visited_style()).map(|_| {
        inputs.rules.as_ref().unwrap_or(self.rule_tree.root())
    })
});
```
Attachment #8992611 - Flags: review?(xidorn+moz) → review+
Attachment #8991596 - Flags: review?(emilio)
Attachment #8991597 - Flags: review?(emilio)
Attachment #8991598 - Flags: review?(emilio)
Comment on attachment 8992612 [details] [diff] [review]
Remove IS_STYLE_IF_VISITED.

Review of attachment 8992612 [details] [diff] [review]:
-----------------------------------------------------------------

::: servo/components/style/properties/computed_value_flags.rs
@@ -56,5 @@
>          /// Whether the child explicitly inherits any reset property.
>          const INHERITS_RESET_STYLE = 1 << 8;
>  
> -        /// A flag to mark a style which is a visited style.
> -        const IS_STYLE_IF_VISITED = 1 << 9;

Rather than removing it, consider replacing it with something like
> // const UNUSED_FLAG = 1 << 9;

so that it is easier for people to see this empty slot when they are adding new flags.
Attachment #8992612 - Flags: review?(xidorn+moz) → review+
(In reply to Xidorn Quan [:xidorn] UTC+10 from comment #25)
> Comment on attachment 8992611 [details] [diff] [review]
> Push visited style computation a bit further down.
> 
> Review of attachment 8992611 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> OK, I guess it's the better way to fix this. I don't think it's really
> easier to understand but it's a smaller change anyway.
> 
> r=me with the issues below addressed.
> 
> ::: servo/components/style/properties/properties.mako.rs
> @@ +3702,5 @@
> >  where
> >      E: TElement,
> >  {
> >      debug_assert_eq!(parent_style.is_some(), parent_style_ignoring_first_line.is_some());
> >      let empty = SmallBitVec::new();
> 
> It's not clear to me why do you move `property_restriction` into
> `rule_declarations_iterator` but not this one. I have a reverse feeling that
> this should probably go local into the iterating function, and restriction
> should probably be passed in instead.
> 
> Actually it's not totally clear to me why you need to split this function at
> all. I don't see any other place using `rule_declarations_iterator`. Is
> there?

I cannot because it doesn't outlive the function call otherwise. But you're right regarding that, I was going to use it in another place but then changed my mind.

> @@ +3776,2 @@
> >          Some(parent_style) => {
> > +            (parent_style, layout_parent_style.unwrap_or(parent_style))
> 
> I find you having both `layout_parent` and `layout_parent_style` around very
> confusing.
> 
> If the non-Option then layout_parent_style (now layout_parent) is only used
> when we call StyleAdjuster, can we move the handling of that around
> StyleAdjuster::adjust instead? That should leave just one
> `layout_parent_style` around.

Yeah, sounds fine.

> ::: servo/components/style/stylist.rs
> @@ +848,3 @@
> >          // We need to compute visited values if we have visited rules or if our
> >          // parent has visited values.
> > +        let visited_rules = match inputs.visited_rules.as_ref() {
> 
> This may be more concise:
> ```
> let visited_rules = input.visited_rules.as_ref().or_else(|| {
>     parent_style.and_then(|s| s.visited_style()).map(|_| {
>         inputs.rules.as_ref().unwrap_or(self.rule_tree.root())
>     })
> });
> ```

Hmm, not sure if I like it more or less... I'll give it a thought :)
(In reply to Xidorn Quan [:xidorn] UTC+10 from comment #26)
> Comment on attachment 8992612 [details] [diff] [review]
> Remove IS_STYLE_IF_VISITED.
> 
> Review of attachment 8992612 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: servo/components/style/properties/computed_value_flags.rs
> @@ -56,5 @@
> >          /// Whether the child explicitly inherits any reset property.
> >          const INHERITS_RESET_STYLE = 1 << 8;
> >  
> > -        /// A flag to mark a style which is a visited style.
> > -        const IS_STYLE_IF_VISITED = 1 << 9;
> 
> Rather than removing it, consider replacing it with something like
> > // const UNUSED_FLAG = 1 << 9;
> 
> so that it is easier for people to see this empty slot when they are adding
> new flags.

Instead I just renumbered the later servo flag.
Pushed by emilio@crisal.io:
https://hg.mozilla.org/integration/mozilla-inbound/rev/454249841df9
Push visited style computation a bit further down. r=xidorn
https://hg.mozilla.org/integration/mozilla-inbound/rev/229e7ad57321
Remove IS_STYLE_IF_VISITED. r=xidorn
https://hg.mozilla.org/integration/mozilla-inbound/rev/56043453ebe6
Add a visited reftest for different logical border color with different writing modes. r=emilio
Assignee: xidorn+moz → emilio
I'm not sure that ignoring writing-mode in :visited is compliant with the proposed spec (https://github.com/w3c/csswg-drafts/commit/4a4b81cf51524b5f74a877e8e7ce66881b5ac383)

+		2. Otherwise, compute |el|’s style,
+			treating only its <a>relevant link</a> as matching '':visited''
+			(and thus, not '':link'').
+			All other links must be treated as matching '':link''.
+
+		3. From the styling results,
+			record the values of the <dfn lt="allowed :visited property">allowed :visited properties</dfn>:
+
+			* 'color'
+			* 'background-color'
+			* the 'border-color' sub-properties
+			* 'outline-color'
+			* 'column-rule-color'
+			* 'fill-color'
+			* 'stroke-color'
+
+			These are |el|’s <dfn dfn lt=":visited style">:visited styles</dfn>.

According to that, the writing mode should affect the logical-to-physical mapping in :visited.

This is also what happens with CSS variables (https://github.com/w3c/csswg-drafts/issues/2263) like

  a {
    --foo: brown;
  }
  :visited {
    --foo: yellow;
    color: var(--foo);
  }

The link is yellow in Firefox. It's not observable that --foo is yellow, but the effect on color is not ignored. For consistency, I think the same should happen with writing-mode and logical properties.
(In reply to Oriol Brufau [:Oriol] from comment #31)
> The link is yellow in Firefox. It's not observable that --foo is yellow, but
> the effect on color is not ignored. For consistency, I think the same should
> happen with writing-mode and logical properties.

That means you'd need to track multiple different writing-modes, because the :link and :visited writing-modes could be different, if you allow writing-mode to affect :visited, or otherwise make the logical properties useless (by not honoring writing-mode and direction in the :visited rules), which is effectively what the spec says but I don't think it's the intended behavior.
(In reply to Emilio Cobos Álvarez (:emilio) from comment #32)
> That means you'd need to track multiple different writing-modes

Sure. I guess this is also what happens with custom properties. If I use getComputedStyle(a).getPropertyValue('--foo') I get " brown", but var(--foo) gets " yellow".

I posted about this in https://github.com/w3c/csswg-drafts/issues/2844, maybe the CSSWG should discuss it.
Based on the spec text, we probably shouldn't be supporting logical border colors at all, since they are not subproperties of border-color at all currently.
(In reply to Xidorn Quan [:xidorn] UTC+10 from comment #34)
> Based on the spec text, we probably shouldn't be supporting logical border
> colors at all, since they are not subproperties of border-color at all
> currently.

But physical and logical properties share computed values. So when you record the computed value of the physical border colors, this takes logical border colors into account.
You need to log in before you can comment on or make changes to this bug.