1485179 - Fix -Wbitfield-enum-conversion warnings where BCBorderInfo bit-field 'ownerElem' is not wide enough to store all enumerators of 'BCBorderOwner'

Status: RESOLVED FIXED

(Core :: Layout: Tables, defect, P3)

Description

[Chris Peterson [:cpeterson]]

clang's opt-in -Wbitfield-enum-conversion check [1] warns when assigning an enum value to a bit field that is not wide enough to store all the enum values:

layout/tables/nsTableFrame.cpp:5318:14 [-Wbitfield-enum-conversion] bit-field 'ownerElem' is not wide enough to store all enumerators of 'BCBorderOwner'
layout/tables/nsTableFrame.cpp:5358:16 [-Wbitfield-enum-conversion] bit-field 'ownerElem' is not wide enough to store all enumerators of 'BCBorderOwner'
layout/tables/nsTableFrame.cpp:5374:18 [-Wbitfield-enum-conversion] bit-field 'subElem' is not wide enough to store all enumerators of 'BCBorderOwner'
layout/tables/nsTableFrame.cpp:5385:18 [-Wbitfield-enum-conversion] bit-field 'subElem' is not wide enough to store all enumerators of 'BCBorderOwner'

The assignment at [2] truncates a BCBorderOwner enum ([3] which has values 0-10, thus requiring at least 4 bits) to BCCornerInfo's ownerElem member variable ([4] which is a 3-bit bit field that can only hold up values 0-7). Thus BCCornerInfo's ownerElem cannot hold BCBorderOwner enum values eAjaRowOwner, eCellOwner, or eAjaCellOwner. I added an assert before this truncating assignment and it fails on many tests.

BCCornerInfo's ownerElem and subElem bit fields can expanded from 3 to 4 bits to fix all BCBorderOwner enum values. Extra bits can be found by reducing  BCCornerInfo's ownerStyle and subStyle from 8 to 4 bits because they [6] only hold NS_STYLE_BORDER_STYLE_* values 0-10 [7].

[1] https://clang.llvm.org/docs/DiagnosticsReference.html#wbitfield-enum-conversion
[2] https://searchfox.org/mozilla-central/rev/847b64cc28b74b44c379f9bff4f415b97da1c6d7/layout/tables/nsTableFrame.cpp#5318
[3] https://searchfox.org/mozilla-central/rev/847b64cc28b74b44c379f9bff4f415b97da1c6d7/layout/tables/celldata.h#132-145
[4] https://searchfox.org/mozilla-central/rev/847b64cc28b74b44c379f9bff4f415b97da1c6d7/layout/tables/nsTableFrame.cpp#5303
[5] https://searchfox.org/mozilla-central/rev/847b64cc28b74b44c379f9bff4f415b97da1c6d7/layout/tables/nsTableFrame.cpp#5311
[6] https://searchfox.org/mozilla-central/rev/3fa761ade83ed0b8ab463acb057c2cf0b104689e/layout/tables/nsTableFrame.cpp#4428-4429
[7] https://searchfox.org/mozilla-central/rev/3fa761ade83ed0b8ab463acb057c2cf0b104689e/layout/style/nsStyleConsts.h#353-364

Comment 1

[Daniel Holbert [:dholbert]]

This seems to date back to bug 41262 --> adding dependency.

(The patch there, attachment 70247 [details] [diff] [review], has "PRUint32  ownerElem:3;" into which is assigned "aBorderOwner" of type BCBorderOwner, which is defined as having enum values 0-10 in that patch.)

Comment 2

[Chris Peterson [:cpeterson]]

Here is an example mochitest failure in layout/reftests/table-bordercollapse/frame_above_rules_all.html after expanding the bit field:

https://hg.mozilla.org/mozilla-central/raw-file/tip/layout/tools/reftest/reftest-analyzer.xhtml#logurl=https://queue.taskcluster.net/v1/task/R_rkPPacSECjwKaAPB27SA/runs/1/artifacts/public/logs/live_backing.log&only_show_unexpected=1&only_show_unexpected=1

from this Try run:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=879c7a41e7b693f42dcdb6a6d27c116605d58f8a

Comment 3

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

All of the failures seem to be broken corner drawing in the reference.  Maybe something in the corner-drawing code doesn't handle one of the newly-exposed values?

Comment 4

[Chris Peterson [:cpeterson]]

Attachment: frame_above_rules_all.png

Here is a screenshot comparing layout/reftests/table-bordercollapse/frame_above_rules_all.html (left) and frame_above_rules_all_ref.html (right) in Firefox 61 (top), Chrome 69 (middle), and Edge 18 (bottom).

In Firefox, both test files are missing border pixels at bottom right corners of "Row 1, Cell 2" and "Row 2, Cell 1".

In Chrome, there are no missing pixels, but the borders are grey in frame_above_rules_all.html and black in frame_above_rules_all_ref.html.

In Edge, both test files look correct.

Comment 5

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

... or it could be BCCornerInfo::Update not handling the new data correctly?

Comment 6

[Chris Peterson [:cpeterson]]

Even though fixing this BCBorderOwner enum truncation bug causes the frame_above_rules_all_ref.html test (but not the frame_above_rules_all.html test) to have three missing border pixels instead of two, it's not the cause of the missing pixels.

The missing border pixels are actually a regression from device pixel scaling bug 895096 in Firefox 57:

https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=48448bff8f92f3e56847063f5c2cb532bb05e46f&tochange=405957d41476f08d4e8a0468fd073b1040759ffe

Comment 7

[Chris Peterson [:cpeterson]]

Attachment: wip-enable-and-fix-Wbitfield-enum-conversion-warnings.patch

Attaching WIP patch

Comment 8

[Chris Peterson [:cpeterson]]

I'm not working on this right now.

Comment 9

[Chris Peterson [:cpeterson]]

Attachment: frame_above_rules_all_ref.html

I spent some time today trying to debug this border-collapse bug (which was actually revealed by the fix for device pixel bug 895096), but I'm an dead end. Here is a simplified test case. You must zoom the page for the borders to be drawn in Firefox (but not Chrome or Safari).

The red (in my test case) border height problem has something to do with colspan=2 when top row is in <thead> and the colspan=2 bottom row is in <tbody>. Moving the two rows both into <thead> or <tbody> makes the red border height problem go away.

BCBlockDirSeg::GetBEndCorner() has cornerSubWidth=0 in the short case (in the middle of the colspan=2) and cornerSubWidth=54 in the good case.

btw, changing the solid borders to dashed or dotted reveals some interesting bugs in Chrome and Safari where the dash and dot patterns have inconsistent spacing in different columns.

Comment 10

[Chris Peterson [:cpeterson]]

I'd like to fix this warning so I can enable clang's -Wbitfield-enum-conversion warning tree-wide, but I don't have a fix for the new test failure.

This following patches explicitly mask the BCBorderOwner enum values to recreate the previous implicit truncation when assigning the 4-bit enum value to the 3-bit field. The buggy behavior will exist, but we will be in a strictly better place: the code will have a FIXME comment linking to this bug and the enabled -Wbitfield-enum-conversion warning will prevent any other code from introducing enum/bitfield truncation.

I will leave this bug open to note that the truncation behavior and test failure are unfixed. This same test case produces different results in Chrome and Edge, so at least this bug appears to be a corner case and not a serious web compat problem.

Comment 11

[Chris Peterson [:cpeterson]]

Attachment: Bug 1485179 - Part 1: Define a BORDER_STYLE_UNSET macro to abstract ownerStyle's sentinel value. r?dholbert

The value of BORDER_STYLE_UNSET will change in patch part 2 when we must shrink the sentinel value from 0xFF to 0xF to fit in the new 4-bit style bit fields.

Comment 12

[Chris Peterson [:cpeterson]]

Attachment: Bug 1485179 - Part 2: Resize BCCornerInfo's bit fields to fit all BCBorderOwner enum values. r?dholbert

Unfortunately, accepting all BCBorderOwner enum values causes the layout/reftests/table-bordercollapse/frame_above_rules_all.html test to fail. To fix the -Wbitfield-enum-conversion warnings without breaking the test, this patch explicitly masks the BCBorderOwner enum values to recreate the previous implicit truncation.

This enum truncation was reported by clang's -Wbitfield-enum-conversion warnings:

layout/tables/nsTableFrame.cpp:5318:14 [-Wbitfield-enum-conversion] bit-field 'ownerElem' is not wide enough to store all enumerators of 'BCBorderOwner'
layout/tables/nsTableFrame.cpp:5358:16 [-Wbitfield-enum-conversion] bit-field 'ownerElem' is not wide enough to store all enumerators of 'BCBorderOwner'
layout/tables/nsTableFrame.cpp:5374:18 [-Wbitfield-enum-conversion] bit-field 'subElem' is not wide enough to store all enumerators of 'BCBorderOwner'
layout/tables/nsTableFrame.cpp:5385:18 [-Wbitfield-enum-conversion] bit-field 'subElem' is not wide enough to store all enumerators of 'BCBorderOwner'

Depends on D12381

Comment 13

[Daniel Holbert [:dholbert]]

(In reply to Chris Peterson [:cpeterson] from comment #10)
> I will leave this bug open to note that the truncation behavior and test
> failure are unfixed.

I'd rather not do that...

That leaves this bug "dual-purpose", patch-wise (some patches landing now to adjust field width, and other patches landing in $FUTURE to fix the truncation behavior).

I'm not sure how well that works with phabricator (we'd end up with two phabricator stacks owned by different people, landing at different times/releases, but both associated with this same bug.  I'm guessing this isn't possible, or at least is confusing to manage.)

Would you mind spinning off a followup bug for the truncation behavior (and noting that bug # in the FIXME in part 2), and adjusting this bug's summary to indicate that it's just about addressing the warning & busted field width?

Comment 14

[Chris Peterson [:cpeterson]]

(In reply to Daniel Holbert [:dholbert] from comment #13)
> Would you mind spinning off a followup bug for the truncation behavior (and
> noting that bug # in the FIXME in part 2), and adjusting this bug's summary
> to indicate that it's just about addressing the warning & busted field width?

Good idea. I filed follow-up bug 1508921 and attached a simplified test case.

Comment 15

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/85587782f51f
Part 1: Define a BORDER_STYLE_UNSET macro to abstract ownerStyle's sentinel value. r=dholbert
https://hg.mozilla.org/integration/mozilla-inbound/rev/0119ff5d4de9
Part 2: Resize BCCornerInfo's bit fields to fit all BCBorderOwner enum values. r=dholbert

Comment 16

[Bogdan Tara[:bogdan_tara | bogdant]]

https://hg.mozilla.org/mozilla-central/rev/85587782f51f
https://hg.mozilla.org/mozilla-central/rev/0119ff5d4de9