1335987 - stylo: Weird timing issue with @import rule

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, defect)

Description

[Xidorn Quan [:xidorn] UTC+10]

Attachment: testcase

This is a failure pattern found in test_parse_rule.html that, in certain condition, @import rule would not be applied when doing cascading.

See the attached file, stylo shows "rgb(0, 0, 0)", while other browsers would show "rgb(0, 128, 0)".

This is a pretty weird timing issue that, with any of the following modification, it would show the correct result:
* remove the outer setTimeout, so make the inner script run synchronously, or
* add some text to the <div>

I have no idea why this happens.

Comment 1

[Emilio Cobos Álvarez (:emilio)]

I believe this is just a consequence of our buggy handling of stylesheet additions? As far as I know, when we append a child stylesheet, we flush the active stylesheet list, but don't enqueue the document for a restyle, so the style will only be applied if the element gets re-restyled.

Comment 2

[Emilio Cobos Álvarez (:emilio)]

Actually I guess it should be in a mutation batch anyway... I'd rather take a more detailed look.

Comment 3

[Emilio Cobos Álvarez (:emilio)]

I believe this is a traversal/incremental restyle issue where we don't update the style contexts.

Comment 4

[Emilio Cobos Álvarez (:emilio)]

We're skipping the document restyle completely as far as I know, I've got to see why yet.

Comment 5

[Emilio Cobos Álvarez (:emilio)]

So what it's happening is that those settimeouts make servo restyle the element for the first time once the import has loaded.

Since we don't have restyle data, we assume that the element really needs no changes at all, but the element already has a frame for some reason so we don't reconstruct the style context.

Still digging to see what's going on.

Comment 6

[Emilio Cobos Álvarez (:emilio)]

Gah, forget that, I know what's going on.

Bobby, the PeekStyleXXX optimization is biting us in the foot. Mainly, without the setTimeout call, the restyle is the initial restyle so there's no problem, and the style context gets recreated during frame construction.

In the setTimeout case, we need to actually do a restyle when the import loads. We restyle from color: <default> to color: green.

When we compare to get the change hint, it returns nsChangeHint(0), because nobody has accessed StyleColor(), so PeekStyleColor() returns false. When there's a text node inside, of course, the StyleColor() gets accessed to get the proper inherited color, so the change hint returns non-zero and we properly recreate the style context.

So the cool part is that it's not a timing issue after all.

We have to options here I guess: Disabling that optimization (which is definitely the easiest fix), or teach nsDOMComputedStyle that the most up-to-date style may not be on the frame but on the element, which is something I guess we could/should do, albeit it may be a bit more complex.

Bobby, what do you think?

Comment 7

[Bobby Holley (:bholley)]

Oh yeah. I guess given the PeekStyle optimization, we can't rely on the change hint for either culling work in RecreateStyleContexts, or culling property cascading like your patch previously did in [1].

For the first issue (the one in this bug), we basically want to pointer-compare the ComputedValues and see if they changed. Ideally we'd avoid the refcount traffic in the case that they didn't, so some sort of Servo_Element_GetComputedValuesIfChanged(ServoComputedValues* aOldStyle). Or we could roll it all into one omnibus function with Servo_TakeChangeHint if we wanted to coalesce the FFI calls, but I think it's not hot enough to justify doing that.

For the second, I think we should do the correct optimization and check the additional outparams returned by CalcStyleDifference.

[1] https://github.com/servo/servo/pull/15317

Comment 8

[Bobby Holley (:bholley)]

Also, I think this means we should get rid of the logic introduced in bug 1330874.

Comment 9

[Emilio Cobos Álvarez (:emilio)]

Attachment: patch

Let me know if you think we should take care of the refcount traffic right now. I think we also need to optimize all the pseudo-element business, so that work can happen at the same time.

Comment 10

[Emilio Cobos Álvarez (:emilio)]

I removed the second changeHint & ~nsChangeHint_NeutralChange (in AppendFrame) locally, which is unneeded.

Comment 11

[Bobby Holley (:bholley)]

Comment on attachment 8833450 [details] [diff] [review]
patch

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

Thanks for fixing this!

::: layout/base/ServoRestyleManager.cpp
@@ +161,4 @@
>      return;
>    }
>  
> +  // TODO(emilio): We could avoid some refcount traffic here.

I'd be more specific (adding an FFI function to only return the ComputedValues if they're different from the old ones), since otherwise this comment looks like it's talking about the oldStyleContext stuff, which has a non-atomic (and therefore cheap) refcount.

@@ +165,5 @@
> +  //
> +  // Hold the old style context alive, because it could become a dangling
> +  // pointer during the replacement. In practice it's not a huge deal (on
> +  // GetNextContinuationWithSameStyle the pointer is not dereferenced, only
> +  // compared), but better not playing with dangling pointers if not needed.

You don't need to justify avoiding dangling pointers, so you can make this comment shorter. :-)

::: layout/style/nsStyleContext.cpp
@@ +1267,2 @@
>    // Given that, we can't strip the neutral change hint here, since it may
>    // provoke errors like bug 1330874.

This comment should go, right?

Comment 12

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/60dade66b054579bcbe5f37c7263a756f6cd108b

Comment 13

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/60dade66b054