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

RESOLVED FIXED in Firefox 57

Status

()

RESOLVED FIXED
2 years ago
a year ago

People

(Reporter: dholbert, Assigned: emilio)

Tracking

({sec-other})

Trunk
mozilla58
sec-other
Points:
---
Bug Flags:
in-testsuite +
qe-verify -

Firefox Tracking Flags

(firefox-esr52 unaffected, firefox55 unaffected, firefox56 disabled, firefox57 fixed, firefox58 fixed)

Details

(Whiteboard: keep hidden while bug 1402798 is hidden)

Attachments

(2 attachments, 1 obsolete attachment)

(Reporter)

Description

2 years ago
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.)
(Reporter)

Comment 2

2 years ago
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.
(Assignee)

Comment 3

2 years ago
Seems like we're "just" missing to restyle the first letter continuation frame.
(Assignee)

Comment 4

2 years ago
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.
(Assignee)

Comment 5

2 years ago
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.
(Assignee)

Updated

2 years ago
Assignee: nobody → emilio
Summary: stylo: "::first-letter" child of dynamically restyled SVG text frame has the wrong style context → stylo: "::first-letter" not restyled properly on owned anon boxes.
(Assignee)

Comment 7

2 years ago
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.
(Assignee)

Comment 8

2 years ago
With the logic a bit tweaked to avoid action at a distance and for it to be (marginally, I guess) faster.
Attachment #8913503 - Attachment is obsolete: true
Attachment #8913503 - Flags: review?(bzbarsky)
Attachment #8913509 - Flags: review?(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>
Attachment #8913509 - Flags: review?(bzbarsky) → review+
(Assignee)

Comment 11

2 years ago
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.
(Assignee)

Comment 12

2 years ago
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
(Reporter)

Comment 13

2 years ago
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). :)
status-firefox57: --- → affected
Flags: needinfo?(emilio)
status-firefox55: --- → unaffected
status-firefox56: --- → disabled
status-firefox-esr52: --- → unaffected
(Assignee)

Comment 14

2 years ago
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
Flags: needinfo?(emilio)
Attachment #8913509 - Flags: approval-mozilla-beta?
Keywords: sec-other
Whiteboard: keep hidden while bug 1402798 is hidden
https://hg.mozilla.org/mozilla-central/rev/ac3ccd004261
Status: NEW → RESOLVED
Last Resolved: 2 years ago
status-firefox58: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla58
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
Attachment #8913509 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Group: layout-core-security → core-security-release
https://hg.mozilla.org/releases/mozilla-beta/rev/3f6b01c6cc57
status-firefox57: affected → fixed
Flags: in-testsuite+
(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.
Flags: qe-verify-
Depends on: 1406562
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.