7083187: Class CSS.CssValue is missing implementations of equals() and hashCode() #13405

prsadhuk · 2023-04-10T12:44:08Z

Two CSS AttributeSet-s can be compared using the AttributeSet.isEqual() method which can fail due to missing implementation of equals method in CSS subclasses.
In this issue, even when two CSS AttributeSet has same 42 font size string value, Object equality fails.
Fixed by implementing the equality and hashCode method for CSS.FontSize class.

All jtreg/jck tests are ok

Progress

Change must be properly reviewed (1 review required, with at least 1 Reviewer)
Change must not contain extraneous whitespace
Commit message must refer to an issue

Issue

JDK-7083187: Class CSS.CssValue is missing implementations of equals() and hashCode() (Bug - P4)

Reviewers

Alexey Ivanov (@aivanov-jdk - Reviewer)
Phil Race (@prrace - Reviewer) ⚠️ Review applies to 4683844d

Contributors

Alexey Ivanov <aivanov@openjdk.org>

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.org/jdk.git pull/13405/head:pull/13405
$ git checkout pull/13405

Update a local copy of the PR:
$ git checkout pull/13405
$ git pull https://git.openjdk.org/jdk.git pull/13405/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 13405

View PR using the GUI difftool:
$ git pr show -t 13405

Using diff file

Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/13405.diff

Webrev

Link to Webrev Comment

…d hashCode()

bridgekeeper · 2023-04-10T12:44:52Z

👋 Welcome back psadhukhan! A progress list of the required criteria for merging this PR into master will be added to the body of your pull request. There are additional pull request commands available for use with this pull request.

openjdk · 2023-04-10T12:46:12Z

@prsadhuk The following label will be automatically applied to this pull request:

client

When this pull request is ready to be reviewed, an "RFR" email will be sent to the corresponding mailing list. If you would like to change these labels, use the /label pull request command.

mlbridge · 2023-04-10T12:58:29Z

Webrevs

aivanov-jdk

The change looks good but it doesn't address the whole problem raised in the JDK-7083187: CSS.CssValue does not implement equals. This fix addresses only one particular case: CSS.FontSize for font-size property.

I do not think it resolves the problem entirely: CssValue and all its subclasses must implement equals method, otherwise adding another CSS attribute to AttributeSet will lead to this same issue described in the bug report.

src/java.desktop/share/classes/javax/swing/text/html/CSS.java

test/jdk/javax/swing/text/html/CSS/CSSBug.java

prsadhuk · 2023-05-02T11:10:14Z

The change looks good but it doesn't address the whole problem raised in the JDK-7083187: CSS.CssValue does not implement equals. This fix addresses only one particular case: CSS.FontSize for font-size property.

I do not think it resolves the problem entirely: CssValue and all its subclasses must implement equals method, otherwise adding another CSS attribute to AttributeSet will lead to this same issue described in the bug report.

Yes, since CSS Attributes are quite extensive, I have only addressed part of it in this PR mainly CSS.Font properties..
Additional PR can be raised for other attributes in due course..

aivanov-jdk · 2023-05-02T12:18:13Z

The change looks good but it doesn't address the whole problem raised in the JDK-7083187: CSS.CssValue does not implement equals. This fix addresses only one particular case: CSS.FontSize for font-size property.
I do not think it resolves the problem entirely: CssValue and all its subclasses must implement equals method, otherwise adding another CSS attribute to AttributeSet will lead to this same issue described in the bug report.

Yes, since CSS Attributes are quite extensive, I have only addressed part of it in this PR mainly CSS.Font properties.. Additional PR can be raised for other attributes in due course..

It sounds reasonable but you have to update the bug summary to make it clear that this bug resolves the issue for CSS.FontSize only. The current bug subject suggests CSS.CssValue which is the super class of CSS.FontSize was fixed but it's not the case.

Then, I think you should create a new bug for CSS.CssValue right away: we know the problem exists. Someone else may take it up.

src/java.desktop/share/classes/javax/swing/text/html/CSS.java

test/jdk/javax/swing/text/html/CSS/CSSAttributeEqualityBug.java

src/java.desktop/share/classes/javax/swing/text/html/CSS.java

prsadhuk · 2023-05-17T03:46:41Z

@prrace @aivanov-jdk Any other comments for this PR?

aivanov-jdk

Can all the subclasses of CssValue be compared with correct result?

src/java.desktop/share/classes/javax/swing/text/html/CSS.java

aivanov-jdk · 2023-05-19T18:02:33Z

May I suggest a simplified version of the CSSAttributeEqualityBug.java test which contains a list of CSS declarations which produce equal attribute sets and another list of declarations which produce non-equal attribute sets.

The updated test covers all the existing cases as well as a few more cases for which I raised my concerns above.

To add a new test case, just add a new entry into either EQUALS or NOT_EQUALS array.

aivanov-jdk · 2023-05-19T18:11:55Z

The updated test covers all the existing cases as well as a few more cases for which I raised my concerns above.

To be more specific, it produces the following output:

margin-top=100%  is not equal to margin-top=100%
font-size=42px  is equal to font-size=22px
font-size=42px  is equal to font-size=42pt
font-size=42em  is equal to font-size=42ex
font-size=100%  is equal to font-size=200%
margin-top=42px  is equal to margin-top=42pt
java.lang.RuntimeException: 6 failure(s) detected: margin-top=100%  is not equal to margin-top=100%
        at CSSAttributeEqualityBug.main(CSSAttributeEqualityBug.java:105)
        at java.base/jdk.internal.reflect.DirectMethodHandleAccessor.invoke(DirectMethodHandleAccess
or.java:103)
        at java.base/java.lang.reflect.Method.invoke(Method.java:578)
        at com.sun.javatest.regtest.agent.MainWrapper$MainThread.run(MainWrapper.java:125)
        at java.base/java.lang.Thread.run(Thread.java:1630)

The new test does not provide direct references to CSS.CssValue subclasses, yet it's somewhat easy to infer. Comments in the arrays could refer to the class, for example margin-top uses CSS.LengthValue.

src/java.desktop/share/classes/javax/swing/text/html/CSS.java

prsadhuk · 2023-06-01T02:59:57Z

@prrace @aivanov-jdk Any further comments on this PR? I would like to get it in by RDP1 if it is possible and I guess contentious issues are sorted

src/java.desktop/share/classes/javax/swing/text/html/CSS.java

prrace

Please confirm this test (and all other relevant tests) still pass.
Approving in anticipation of confirmation of the above.

openjdk · 2023-06-06T22:32:31Z

@prsadhuk This change now passes all automated pre-integration checks.

ℹ️ This project also has non-automated pre-integration requirements. Please see the file CONTRIBUTING.md for details.

After integration, the commit message for the final commit will be:

7083187: Class CSS.CssValue is missing implementations of equals() and hashCode()

Co-authored-by: Alexey Ivanov <aivanov@openjdk.org>
Reviewed-by: aivanov, prr

You can use pull request commands such as /summary, /contributor and /issue to adjust it as needed.

At the time when this comment was updated there had been 1016 new commits pushed to the master branch:

3e0bbd2: 8285368: Overhaul doc-comment inheritance
3eeb681: 8167252: Some of Charset.availableCharsets() does not contain itself
653a8d0: 8310129: SetupNativeCompilation LIBS should match the order of the other parameters
947f149: 8308444: LoadStoreNode::result_not_used() is too conservative
8b4af46: 8309974: some JVMCI tests fail when VM options include -XX:+EnableJVMCI
0038491: 8309978: [x64] Fix useless padding
5f3613e: 8309960: ParallelGC young collections very slow in DelayInducer
83d9267: 8303513: C2: LoadKlassNode::make fails with 'expecting TypeKlassPtr'
de8aca2: 8307907: [ppc] Remove RTM locking implementation
4c0e164: 8309717: C2: Remove Arena::move_contents usage
... and 1006 more: https://git.openjdk.org/jdk/compare/ce10460e94c03f178943fa44dafae18acc480094...master

As there are no conflicts, your changes will automatically be rebased on top of these commits when integrating. If you prefer to avoid this automatic rebasing, please check the documentation for the /integrate command for further details.

➡️ To integrate this PR with the above commit message to the master branch, type /integrate in a new comment.

aivanov-jdk

Overall, it covers most of the values now. There are a couple of omissions though: BackgroundImage, BorderWidthValue (it could be handled automatically by its superclass, LengthValue).

I assume CssValue is never used directly, is it?

src/java.desktop/share/classes/javax/swing/text/html/CSS.java

aivanov-jdk · 2023-06-07T12:26:33Z

src/java.desktop/share/classes/javax/swing/text/html/CSS.java

+        public boolean equals(Object val) {
+            if (percentage) {
+                return val instanceof CSS.LengthValue lu
+                       && Objects.equals(svalue, lu.svalue);


Doesn't comparing span work for percentage values? The comment for span field implies it should contain the parsed value.

This comparison could fail for the case where there's a space before the % in the string.

This comparison could fail for the case where there's a space before the % in the string.

FontSize does handle {"font-size: 100%", "font-size: 100 %"} pair, but LengthValue doesn't.

No, it doesn't. For some reason, both 100% and 200% are parsed so that span = 1.0.

Let's leave it as is then. It handles the most common case.

Handling a space before the percent sign can be postponed to a later fix.

Aha, the value is capped at 100%:

jdk/src/java.desktop/share/classes/javax/swing/text/html/CSS.java

Lines 2627 to 2628 in 4683844

lv.span = Math.max(0, Math.min(1, lu.value));

lv.percentage = true;

This is why 200% is parsed as if it were 100%.

The following code

public boolean equals(Object val) { return val instanceof CSS.LengthValue lu && percentage == lu.percentage && span == lu.span && Objects.equals(units, lu.units); }

works correctly if you modify this line in the test

jdk/test/jdk/javax/swing/text/html/CSS/CSSAttributeEqualityBug.java

Line 86 in 4683844

{"margin-top: 100%", "margin-top: 200%"},

to

{"margin-top: 100%", "margin-top: 50%"},

The above code also handles the case "margin-top: 50 %" correctly.

I had already made that observation in this comment few days back in case you overlooked
Also, I kept the percentage check for string even though it fails for "space" within string because it seems "space" is not valid in between % value but we can go beyond 100% ie 50 % is not valid but 200% is,
as per https://developer.mozilla.org/en-US/docs/Web/CSS/margin-top
where if you specify margin-top: 50 % and then go to other block and come back, you will get a "X" but
margin-top: 200% is ok

I had already made that observation in this #13405 (comment) days back in case you overlooked

I did miss this comment. Sorry about that.

Even though space between the value and the percent sign or the units is invalid (I couldn't find it quickly in the W3C spec for CSS), you should compare the parsed values. We pass 100% and 200% but the parsed value is 100% in both cases — the attribute sets are indeed equal. If you apply either attribute set to a document, you'll get the same result. Does it make sense?

There could be other cases where the computed/parsed values are the same even though the input is different, for example "font-size: medium" has a numeric value in points or pixels, so the attribute set with the same value should be equal, don't you agree?

I am not sure on 100%, 200% ie >= 100% should be considered equal or not..I could not find in spec...also the URL I gave has 200% as valid value...so as of now I have considered what is normal and made equals return false for different percentages irrespective of < or > 100%..

I also think that capping at 100% is wrong but it's another bug.

Currently, both 100% and 200% result in attribute sets which behave as if both were 100%, therefore they should be equal. I strongly believe we should compare the parsed values not the input string.

OK. Fair enough..guess CSS implementation and equality should match...Modified...
Upper capping and equality can be looked at later if needed

src/java.desktop/share/classes/javax/swing/text/html/CSS.java

aivanov-jdk · 2023-06-07T12:47:53Z

@prsadhuk, could you add me as the contributor for the test and LengthUnit, please?

aivanov-jdk · 2023-06-07T13:56:35Z

BorderWidthValue (it could be handled automatically by its superclass, LengthValue)

Yep, it's handled correctly "border-width: medium" passes the test.

aivanov-jdk · 2023-06-07T14:23:12Z

So, there are no show-stoppers except for my minor comments.

prsadhuk · 2023-06-08T03:54:06Z

Please confirm this test (and all other relevant tests) still pass. Approving in anticipation of confirmation of the above.

Yes, all "clientlibs" test passed along with this test

prsadhuk · 2023-06-08T03:54:31Z

/contributor add @aivanov-jdk

openjdk · 2023-06-08T03:54:54Z

@prsadhuk
Contributor Alexey Ivanov <aivanov@openjdk.org> successfully added.

src/java.desktop/share/classes/javax/swing/text/html/CSS.java

aivanov-jdk

Thank you for updating the code.

src/java.desktop/share/classes/javax/swing/text/html/CSS.java

aivanov-jdk · 2023-06-08T11:05:35Z

src/java.desktop/share/classes/javax/swing/text/html/CSS.java

+            return val instanceof CSS.LengthValue lu
+                   && span == lu.span
+                   && Objects.equals(units, lu.units);


The percentage field must also be part of equals:

Suggested change

return val instanceof CSS.LengthValue lu

&& span == lu.span

&& Objects.equals(units, lu.units);

return val instanceof CSS.LengthValue lu

&& percentage == lu.percentage

&& span == lu.span

&& Objects.equals(units, lu.units);

You have included it in hashCode.

If percentage isn't taken into account, the pair {"margin-top: 100%", "margin-top: 1"} is considered equal. (And it essentially is; however, I think we should include the percentage field, it is part of the object and the behaviour will be different.)

By the way, this is another quirk of Swing's CSS implementation: in the CSS spec, values without units are considered an error except for a few cases where such usage is specifically allowed.

src/java.desktop/share/classes/javax/swing/text/html/CSS.java

aivanov-jdk

Perfect! Looks to me now.

prsadhuk · 2023-06-19T08:50:34Z

/integrate

openjdk · 2023-06-19T08:52:10Z

Going to push as commit d2a858e.
Since your change was applied there have been 1050 commits pushed to the master branch:

4229baf: 8310015: ZGC: Unbounded asynchronous unmapping can lead to running out of address space
266f983: 8308855: ARM32: TestBooleanVector crashes after 8300257
6a63bad: 8310191: com/sun/tools/attach/warnings/DynamicLoadWarningTest.java second failure on AIX
6473a7d: 8310107: os::trace_page_sizes_for_requested_size should name alignment as requested page size
02aaab1: 8310126: C1: Missing receiver null check in Reference::get intrinsic
492d25c: 8309601: [JVMCI] AMD64#getLargestStorableKind returns incorrect mask kind
959a61f: 8310259: Pin msys2/setup-msys2 github action to a specific commit
bcc4d36: 8309511: Regression test ExtraImportSemicolon.java refers to the wrong bug
71baf00: 8309605: StubRoutines are not used by SA
16134f4: 8310211: serviceability/jvmti/thread/GetStackTrace/getstacktr03/getstacktr03.java failing
... and 1040 more: https://git.openjdk.org/jdk/compare/ce10460e94c03f178943fa44dafae18acc480094...master

Your commit was automatically rebased without conflicts.

openjdk · 2023-06-19T08:52:20Z

@prsadhuk Pushed as commit d2a858e.

💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored.

7083187: Class CSS.CssValue is missing implementations of equals() an…

476f707

…d hashCode()

openjdk bot added the client client-libs-dev@openjdk.org label Apr 10, 2023

prsadhuk added 2 commits April 10, 2023 13:47

Test fix

76d2d38

Test fix

fddecfb

openjdk bot added the rfr Pull request is ready for review label Apr 10, 2023

aivanov-jdk suggested changes Apr 12, 2023

View reviewed changes

prsadhuk added 3 commits May 2, 2023 12:04

Review comment address

7e3774f

Test fix

7fd61bb

Test fix

e24c140

mickleness reviewed May 2, 2023

View reviewed changes

src/java.desktop/share/classes/javax/swing/text/html/CSS.java Outdated Show resolved Hide resolved

aivanov-jdk suggested changes May 2, 2023

View reviewed changes

prsadhuk added 2 commits May 3, 2023 04:41

Review update

d30156f

More Attrbutes added

2bc265c

aivanov-jdk suggested changes May 4, 2023

View reviewed changes

test/jdk/javax/swing/text/html/CSS/CSSAttributeEqualityBug.java Outdated Show resolved Hide resolved

aivanov-jdk suggested changes May 5, 2023

View reviewed changes

prsadhuk added 3 commits May 9, 2023 04:18

Review comment

5219a8c

Test update

4036b7c

hashCode fix

717d1a1

aivanov-jdk suggested changes May 19, 2023

View reviewed changes

Test and fix updated as per review comments

88841b8

aivanov-jdk suggested changes May 22, 2023

View reviewed changes

src/java.desktop/share/classes/javax/swing/text/html/CSS.java Outdated Show resolved Hide resolved

Percentage equality fix

745fe23

Fix optimization w.r.t null check

4a3a897

Use relative variable in equals/hashcode

833ff8f

prrace reviewed Jun 1, 2023

View reviewed changes

src/java.desktop/share/classes/javax/swing/text/html/CSS.java Show resolved Hide resolved

Optimization removed to use local variables for non-percentage units

4683844

prrace approved these changes Jun 6, 2023

View reviewed changes

openjdk bot added the ready Pull request is ready to be integrated label Jun 6, 2023

aivanov-jdk reviewed Jun 7, 2023

View reviewed changes

Address hashcode review comment

9d03fa8

aivanov-jdk reviewed Jun 8, 2023

View reviewed changes

src/java.desktop/share/classes/javax/swing/text/html/CSS.java Outdated Show resolved Hide resolved

aivanov-jdk reviewed Jun 8, 2023

View reviewed changes

src/java.desktop/share/classes/javax/swing/text/html/CSS.java Outdated Show resolved Hide resolved

Fix percentage equality as per span parsed value

94249f7

aivanov-jdk suggested changes Jun 8, 2023

View reviewed changes

Fix per review comment

bc2c13b

aivanov-jdk approved these changes Jun 8, 2023

View reviewed changes

openjdk bot added the integrated Pull request has been integrated label Jun 19, 2023

openjdk bot closed this Jun 19, 2023

openjdk bot removed ready Pull request is ready to be integrated rfr Pull request is ready for review labels Jun 19, 2023

prsadhuk deleted the JDK-7083187 branch June 19, 2023 08:52

	lv.span = Math.max(0, Math.min(1, lu.value));
	lv.percentage = true;

7083187: Class CSS.CssValue is missing implementations of equals() and hashCode() #13405

7083187: Class CSS.CssValue is missing implementations of equals() and hashCode() #13405

Conversation

prsadhuk commented Apr 10, 2023 • edited by openjdk bot Loading

Progress

Issue

Reviewers

Contributors

Reviewing

Webrev

bridgekeeper bot commented Apr 10, 2023

openjdk bot commented Apr 10, 2023

mlbridge bot commented Apr 10, 2023 • edited Loading

Webrevs

aivanov-jdk left a comment

Choose a reason for hiding this comment

prsadhuk commented May 2, 2023

aivanov-jdk commented May 2, 2023

prsadhuk commented May 17, 2023

aivanov-jdk left a comment

Choose a reason for hiding this comment

aivanov-jdk commented May 19, 2023

aivanov-jdk commented May 19, 2023

prsadhuk commented Jun 1, 2023

prrace left a comment

Choose a reason for hiding this comment

openjdk bot commented Jun 6, 2023 • edited Loading

aivanov-jdk left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aivanov-jdk commented Jun 7, 2023

aivanov-jdk commented Jun 7, 2023

aivanov-jdk commented Jun 7, 2023

prsadhuk commented Jun 8, 2023

prsadhuk commented Jun 8, 2023

openjdk bot commented Jun 8, 2023

aivanov-jdk left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aivanov-jdk Jun 8, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aivanov-jdk left a comment

Choose a reason for hiding this comment

prsadhuk commented Jun 19, 2023

openjdk bot commented Jun 19, 2023

openjdk bot commented Jun 19, 2023

prsadhuk commented Apr 10, 2023 •

edited by openjdk bot

Loading

mlbridge bot commented Apr 10, 2023 •

edited

Loading

openjdk bot commented Jun 6, 2023 •

edited

Loading

aivanov-jdk left a comment •

edited

Loading

aivanov-jdk Jun 8, 2023 •

edited

Loading