1159729 - Use of the wrong variable (aoffset instead of offset) in nsCSSRendering::GetTextDecorationRectInternal ?

Status: RESOLVED FIXED

(Core :: Layout, defect)

Description

[Sylvestre Ledru [:Sylvestre]]

scan-build (based on clang-analyzer) found that the offset variable is useless.
https://people.mozilla.org/~sledru/reports/fx-scan-build/report-nsCSSRendering.cpp-GetTextDecorationRectInternal-96-1.html#EndPath

This shows that something is wrong.
Either the whole switch case is useless

Or this code should use offset instead of aOffset
r.y = baseline + floor(aOffset + 0.5);

https://dxr.mozilla.org/mozilla-central/source/layout/base/nsCSSRendering.cpp?from=layout/base/nsCSSRendering.cpp#4522

Comment 1

[Sylvestre Ledru [:Sylvestre]]

Jonathan, I see that you touched at the end of this method. Does it ring a bell? thanks

Comment 2

[Boris Zbarsky [:bzbarsky]]

> r.y = baseline + floor(aOffset + 0.5);

This line used to be:

  r.y = baseline - floor(offset + 0.5);

until https://hg.mozilla.org/mozilla-central/rev/9c8f4a869327

Sounds like we need regression tests here....

Comment 3

[Jonathan Kew [:jfkthame]]

Yes, looks like this should still be |offset| rather than |aOffset| (and likewise a few lines earlier). Which does indicate a gap in test coverage, given that nothing went orange.

I'll run a fix through tryserver to check for anything unexpected, and see if I can come up with a test that breaks appropriately.

Comment 4

[Jonathan Kew [:jfkthame]]

Attachment: Use the adjusted |offset| value in GetTextDecorationRectInternal, instead of ignoring it

This is clearly what was intended here. I'm experimenting with possible tests that could have detected this, though it's a bit of a pain because the effect (if any) of this code is dependent on font metrics and platform-specific details of how we interpret them. Still, we should be able to have a test on at least some platforms.

Comment 5

[Jonathan Kew [:jfkthame]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=6e8860a2c62d

Comment 6

[Jonathan Kew [:jfkthame]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=641e3e868a3e

Comment 7

[Jonathan Kew [:jfkthame]]

Attachment: Reftest to check that text decoration is properly offset when needed

Here's a test that fails on current trunk due to this bug (try run in comment 5) because it fails to lift the overline clear of the text, and passes once it's fixed (comment 6).

Comment 8

[Jonathan Kew [:jfkthame]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/e03d014c257f
https://hg.mozilla.org/integration/mozilla-inbound/rev/f4be5f25b35a

Comment 9

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/e03d014c257f
https://hg.mozilla.org/mozilla-central/rev/f4be5f25b35a