1404167 - stylo: "::first-letter" not restyled properly on owned anon boxes.

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, defect)

Description

[Daniel Holbert [:dholbert]]

STR:
 1. Apply attached patch and rebuild (in a debug build)

 2. Start your build and ensure that about:config pref layout.css.servo.enabled is set to true.

 3. View dom/svg/crashtests/1402798.html (direct link: https://hg.mozilla.org/mozilla-central/raw-file/76a26ef7c493311c170ae83eb0c1d6592a21396d/dom/svg/crashtests/1402798.html )

ACTUAL RESULTS:
Fatal assertion failure, causing your content process to crash. (The assertion in the patch fails.)

EXPECTED RESULTS:
The assertion should not fail.  And indeed, it does not fail if I disable the stylo pref.

So it seems stylo is changing behavior here, in a way that is sometimes problematic.  (This behavior-change was responsible for causing bug 1402798.)

Comment 1

[Daniel Holbert [:dholbert]]

Attachment: patch needed to trigger bug (not a fix, just part of STR)

Comment 2

[Daniel Holbert [:dholbert]]

From my debugging, it looked like the FirstLetterFrame is always getting a StyleSVG()->mFill with type=color and value=transparent black (0xff000000).  This happens even if I manually adjust the testcase to provide a different color (e.g. if I populate the empty ::first-letter{} rule with "fill:green" or something like that).  So it looks like the ::first-letter is just getting some default style struct, or at least the default value for 'fill' in its style struct.

Comment 3

[Emilio Cobos Álvarez (:emilio)]

Seems like we're "just" missing to restyle the first letter continuation frame.

Comment 4

[Emilio Cobos Álvarez (:emilio)]

And the reason for that is that we can't find the :-moz-svg-text anon box.

And the reason for that is because it's not reported either in the owned anon boxes or in the wrapper ones. I believe marking it as a wrapper anon box should just fix it.

Comment 5

[Emilio Cobos Álvarez (:emilio)]

I'm lying, that's supposed to be found via UpdateStyleOfOwnedAnonBoxes, so I think we're just failing to restyle the first-line in that anon box.

Comment 6

[Emilio Cobos Álvarez (:emilio)]

Attachment: Properly update the styles of first-letter childs of anon boxes.

Comment 7

[Emilio Cobos Álvarez (:emilio)]

The reftest text should probably say something different, like "the first-letter size should be 45px" instead of what it says, I'll tweak it locally.

Comment 8

[Emilio Cobos Álvarez (:emilio)]

Attachment: Properly update the styles of first-letter childs of anon boxes.

With the logic a bit tweaked to avoid action at a distance and for it to be (marginally, I guess) faster.

Comment 9

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8913509 [details] [diff] [review]
Properly update the styles of first-letter childs of anon boxes.

r=me

Could you also add reftests with something like fieldset and <td>?  Here's a test that's failing for me locally:

  <style>
  td::first-letter, fieldset::first-letter {
    border: inherit;
  }
  </style>
  <fieldset style="border: 10px solid red" onclick="this.style.border = '10px solid green'">
    This text should have a green border on the first letter after being clicked.
  </fieldset>
  <table>
    <tr>
      <td style="border: 10px solid red" onclick="this.style.border = '10px solid green'">
        This text should have a green border on the first letter after being clicked.
      </td>
    </tr>
  </table>

Comment 10

[Emilio Cobos Álvarez (:emilio)]

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/710b592482eea53c22ea092f5ec038524a539e9f

Comment 11

[Emilio Cobos Álvarez (:emilio)]

Was backed out for asserting the expected owner for frame in a single inspector test:

 * https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=710b592482eea53c22ea092f5ec038524a539e9f&selectedJob=134004656

Looking into it now.

Comment 12

[Emilio Cobos Álvarez (:emilio)]

Fixed the assertion for when a first-letter is wrapped in a first-line, added a reftest, and:

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/ac3ccd00426152daa9f7afbba00cc6abd59a1039

Comment 13

[Daniel Holbert [:dholbert]]

Thanks for the quick work!  Can you request beta approval, to uplift this to 57? (I suspect we should take it wherever we're taking Stylo, given that this issue can violate invariants that we implicitly depend on, as in bug 1402798.)

BTW, just for the record -- I marked this bug as security-sensitive, simply due to its relationship with security-sensitive bug 1402798 [whose patch already landed on trunk] -- we should keep the details/discussion hidden, to err on the side of safety until bug 1402798's patch has been uplifted & users are protected.

So even though this bug here is hidden as a sec bug, I don't think we need to bother with the sec-approval flag on this bug for landing/uplift (which is good, because this already landed on trunk). :)

Comment 14

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8913509 [details] [diff] [review]
Properly update the styles of first-letter childs of anon boxes.

Approval Request Comment
[Feature/Bug causing the regression]: stylo
[User impact if declined]: wrong dynamic styling of ::first-letter, but see also bug 1402798, which this would've prevented.
[Is this code covered by automated tests?]: yes
[Has the fix been verified in Nightly?]: no
[Needs manual test from QE? If yes, steps to reproduce]: no
[List of other uplifts needed for the feature/fix]: none
[Is the change risky?]: not risky
[Why is the change risky/not risky?]: pretty simple patch that makes us not skip work incorrectly.
[String changes made/needed]: none

Comment 15

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

https://hg.mozilla.org/mozilla-central/rev/ac3ccd004261

Comment 16

[Sylvestre Ledru [:Sylvestre]]

Comment on attachment 8913509 [details] [diff] [review]
Properly update the styles of first-letter childs of anon boxes.

Fix the feature, taking it.
should be in 57b5

Comment 17

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/3f6b01c6cc57

Comment 18

[Andrei Vaida [:avaida]]

(In reply to Emilio Cobos Álvarez [:emilio] from comment #14)
> [Is this code covered by automated tests?]: yes
> [Has the fix been verified in Nightly?]: no
> [Needs manual test from QE? If yes, steps to reproduce]: no

Setting qe-verify- based on Emilio's assessment on manual testing needs and the fact that this fix has automated coverage.