8256515: javax.xml.XMLEventReader produces incorrect START_DOCUMENT event #1056

MariusVolkhart · 2020-11-04T14:01:53Z

The default implementation of javax.xml.stream.XMLEventReader produced a StartDocument event that always indicated that the "standalone" attribute was set.

The root cause of this was that the com.sun.xml.internal.stream.events.XMLEventAllocatorImpl always set the "standalone" attribute, rather than asking streamReader.standaloneSet() before setting the property of the StartDocumentEvent being created.

Progress

Change must not contain extraneous whitespace
Commit message must refer to an issue
Change must be properly reviewed

Issue

JDK-8256515: javax.xml.XMLEventReader produces incorrect START_DOCUMENT event

Reviewers

Joe Wang (@JoeWang-Java - Reviewer)

Download

$ git fetch https://git.openjdk.java.net/jdk pull/1056/head:pull/1056
$ git checkout pull/1056

bridgekeeper · 2020-11-04T14:02:37Z

👋 Welcome back MariusVolkhart! 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 · 2020-11-04T14:03:50Z

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

core-libs

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.

MariusVolkhart · 2020-11-04T14:03:53Z

test/jdk/javax/xml/stream/XmlInputFactoryTest.java

+import static org.testng.Assert.assertFalse;
+import static org.testng.Assert.assertTrue;
+
+public class XmlInputFactoryTest {


This test is written correctly for regular TestNG, but I don't know if it's "right" for jtreg. I'm happy to make any changes necessary.

jerboaa · 2020-11-18T09:42:49Z

@MariusVolkhart I'll work with you getting the fix in and get the test running using jtreg. Give me a few moments to reproduce.

MariusVolkhart · 2020-11-19T16:34:36Z

Thanks @jerboaa

I have a JBS number now: 8256515. Since opening the PR I've learned that I've gone about the process backwards, and should have found a Sponsor before making the PR. As such, I haven't updated the commit message to avoid sending a premature RFR to the mailing list

JoeWang-Java · 2020-11-19T17:07:13Z

Hi Marius,

Saw the JBS issue, pls edit the title to sth. like the following:
8256515: javax.xml.XMLEventReader produces incorrect START_DOCUMENT event

Thanks,
Joe

jerboaa · 2020-11-20T09:46:59Z

@MariusVolkhart I think doing a PR is fine before you find a sponsor provided you're fairly certain it's an actual bug. It'll require one to then send an email to the mailing lists asking for a sponsor. That email could potentially reference a PR. That should be fine. The sponsor (committer) will then help you get it integrated. It somewhat depends on the issue. No hard-and-fast rules that I'm aware of in that regard.

jerboaa · 2020-11-20T11:41:53Z

@MariusVolkhart Here is a PR for your branch so the test works with jtreg:
MariusVolkhart#1

Fails before the JDK patch and passes after.

Run it with:

$ rm -rf JTwork JTreport && jtreg -jdk:./build/linux-x86_64-server-release/images/jdk -verbose:summary test/jdk/javax/xml/jaxp/8256515/XmlInputFactoryTest.java
Passed: javax/xml/jaxp/8256515/XmlInputFactoryTest.java
Test results: passed: 1

Results are in *.jtr files:

$ find JTwork/ -name \*.jtr
JTwork/javax/xml/jaxp/8256515/XmlInputFactoryTest.jtr

Or using the test framework of OpenJDK with:

$ bash configure --with-jtreg=/path/to/jtreg [...]
$ make test TEST="jdk/javax/xml/jaxp/8256515/XmlInputFactoryTest.java"

HTH

openjdk · 2020-12-03T14:41:16Z

⚠️ @MariusVolkhart This pull request contains merges that bring in commits not present in the target repository. Since this is not a "merge style" pull request, these changes will be squashed when this pull request in integrated. If this is your intention, then please ignore this message. If you want to preserve the commit structure, you must change the title of this pull request to Merge <project>:<branch> where <project> is the name of another project in the OpenJDK organization (for example Merge jdk:master).

MariusVolkhart · 2020-12-03T14:44:05Z

@jerboaa Thanks! That PR was immensely helpful, not just in helping get this change moving, but in helping me understand how to write the tests for next time!

mlbridge · 2020-12-03T14:47:02Z

Webrevs

03: Full - Incremental (68ab39a)
02: Full (0fa81e4)
01: Full (cb6833921be7a9993eb795d7c56501a1ba8f1b09)
00: Full (e37f5b2e4e134f1a2c6f5312c56bd535674ddda3)

JoeWang-Java · 2020-12-04T01:17:53Z

When you update, pls do a merge from the latest upstream jdk master. The changeset is pretty far behind the master.

…T event The default implementation of javax.xml.stream.XMLEventReader produced a StartDocument event that always indicated that the "standalone" attribute was set. The root cause of this was that the com.sun.xml.internal.stream.events.XMLEventAllocatorImpl always set the "standalone" attribute, rather than asking streamReader.standaloneSet() before setting the property of the StartDocumentEvent being created.

JoeWang-Java · 2020-12-03T23:40:40Z

src/java.xml/share/classes/com/sun/xml/internal/stream/events/XMLEventAllocatorImpl.java

-                sdEvent.setStandalone(streamReader.isStandalone());
+                if (streamReader.standaloneSet()) {
+                    sdEvent.setStandalone(streamReader.isStandalone());
+                }


The change looked fine at the first glance. However, when I looked at your test, I noticed that in your first test case, isStandalone will return true. This is because standalone was initiated as true and with the added condition, setStandalone is skipped.

To fix the issue, consider, instead of making it conditional, adding standaloneSet to the setStandalone method.

Pls update the copyright year at line 2, e.g. 2016 -> 2020.

Updates made. I understood your comment about modifying the setStandalone method to mean StartDocumentEvent#setStandalone. If it was something else, please let me know!

test/jdk/javax/xml/jaxp/8256515/XmlInputFactoryTest.java

…DOCUMENT event

openjdk · 2020-12-08T19:13:12Z

@MariusVolkhart 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:

8256515: javax.xml.XMLEventReader produces incorrect START_DOCUMENT event

Reviewed-by: joehw

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 52 new commits pushed to the master branch:

291ba97: 8251267: CDS tests should use CDSTestUtils.getOutputDir instead of System.getProperty("user.dir")
f48d5d1: 8257789: Fix incremental build of test-image and bundles
1a9ed92: 8200102: NativeLibraryTest.java fails intermittently, unloaded count is not same as expected
264feb3: 8257905: Make fixpath.sh more liberal in accepting paths embedded in arguments
044616b: 8252049: Native memory leak in ciMethodData ctor
fab6158: 8236413: AbstractConnectTimeout should tolerate both NoRouteToHostException and UnresolvedAddressException
936a7ac: 8252797: Non-PCH build fails on Ubuntu 16.4 when building with gtests
d0c5265: 8256149: Weird AST structure for incomplete member select
a708024: 8257194: Add 'foreign linker API' in 'jdk.incubator.foreign' module desc/summary
35e8153: 8257848: -XX:CompileCommand=blackhole,* should be diagnostic
... and 42 more: https://git.openjdk.java.net/jdk/compare/c4339c3064190bd10fdba64f85a501a2f3d52685...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.

As you do not have Committer status in this project an existing Committer must agree to sponsor your change. Possible candidates are the reviewers of this PR (@JoeWang-Java) but any other Committer may sponsor as well.

➡️ To flag this PR as ready for integration with the above commit message, type /integrate in a new comment. (Afterwards, your sponsor types /sponsor in a new comment to perform the integration).

JoeWang-Java · 2020-12-08T20:02:46Z

/contributor add @MariusVolkhart

openjdk · 2020-12-08T20:03:00Z

@JoeWang-Java Only the author (@MariusVolkhart) is allowed to issue the contributor command.

JoeWang-Java · 2020-12-08T20:25:02Z

Hi Marius,

As the bot suggested, this PR is ready to be integrated. You may issue the integrate command when you're ready, I'll then sponsor it for you.

Thanks for contributing the fix!

Regards,
Joe

MariusVolkhart · 2020-12-08T20:32:29Z

/integrate

openjdk · 2020-12-08T20:33:00Z

@MariusVolkhart
Your change (at version 68ab39a) is now ready to be sponsored by a Committer.

JoeWang-Java · 2020-12-08T20:43:03Z

/sponsor

openjdk · 2020-12-08T20:43:58Z

@JoeWang-Java @MariusVolkhart Since your change was applied there have been 52 commits pushed to the master branch:

291ba97: 8251267: CDS tests should use CDSTestUtils.getOutputDir instead of System.getProperty("user.dir")
f48d5d1: 8257789: Fix incremental build of test-image and bundles
1a9ed92: 8200102: NativeLibraryTest.java fails intermittently, unloaded count is not same as expected
264feb3: 8257905: Make fixpath.sh more liberal in accepting paths embedded in arguments
044616b: 8252049: Native memory leak in ciMethodData ctor
fab6158: 8236413: AbstractConnectTimeout should tolerate both NoRouteToHostException and UnresolvedAddressException
936a7ac: 8252797: Non-PCH build fails on Ubuntu 16.4 when building with gtests
d0c5265: 8256149: Weird AST structure for incomplete member select
a708024: 8257194: Add 'foreign linker API' in 'jdk.incubator.foreign' module desc/summary
35e8153: 8257848: -XX:CompileCommand=blackhole,* should be diagnostic
... and 42 more: https://git.openjdk.java.net/jdk/compare/c4339c3064190bd10fdba64f85a501a2f3d52685...master

Your commit was automatically rebased without conflicts.

Pushed as commit c47ab5f.

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

openjdk bot added the core-libs core-libs-dev@openjdk.org label Nov 4, 2020

MariusVolkhart commented Nov 4, 2020

View reviewed changes

MariusVolkhart changed the title ~~Fix: javax.xml.XMLEventReader produces incorrect START_DOCUMENT event~~ 8256515: javax.xml.XMLEventReader produces incorrect START_DOCUMENT event Dec 3, 2020

openjdk bot added the rfr Pull request is ready for review label Dec 3, 2020

MariusVolkhart force-pushed the mv/xmlEventAllocator branch from e37f5b2 to cb68339 Compare December 4, 2020 13:27

MariusVolkhart and others added 3 commits December 5, 2020 12:58

Add test for XmlInputFactory

fd421a3

Adjust test so it works with jtreg

0fa81e4

MariusVolkhart force-pushed the mv/xmlEventAllocator branch from cb68339 to 0fa81e4 Compare December 5, 2020 11:59

JoeWang-Java reviewed Dec 7, 2020

View reviewed changes

MariusVolkhart added 2 commits December 8, 2020 16:44

fixup! Fix: javax.xml.stream.XMLEventReader produces incorrect START_…

68a124c

…DOCUMENT event

fixup! Fix: javax.xml.stream.XMLEventReader produces incorrect START_…

68ab39a

…DOCUMENT event

JoeWang-Java approved these changes Dec 8, 2020

View reviewed changes

openjdk bot added the ready Pull request is ready to be integrated label Dec 8, 2020

openjdk bot added the sponsor Pull request is ready to be sponsored label Dec 8, 2020

openjdk bot closed this Dec 8, 2020

openjdk bot added integrated Pull request has been integrated and removed sponsor Pull request is ready to be sponsored ready Pull request is ready to be integrated rfr Pull request is ready for review labels Dec 8, 2020

MariusVolkhart deleted the mv/xmlEventAllocator branch December 9, 2020 10:28

8256515: javax.xml.XMLEventReader produces incorrect START_DOCUMENT event #1056

8256515: javax.xml.XMLEventReader produces incorrect START_DOCUMENT event #1056

Uh oh!

Conversation

MariusVolkhart commented Nov 4, 2020 • edited by openjdk bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Progress

Issue

Reviewers

Download

Uh oh!

bridgekeeper bot commented Nov 4, 2020

Uh oh!

openjdk bot commented Nov 4, 2020

Uh oh!

MariusVolkhart Nov 4, 2020

Choose a reason for hiding this comment

Uh oh!

jerboaa commented Nov 18, 2020

Uh oh!

MariusVolkhart commented Nov 19, 2020

Uh oh!

JoeWang-Java commented Nov 19, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jerboaa commented Nov 20, 2020

Uh oh!

jerboaa commented Nov 20, 2020

Uh oh!

openjdk bot commented Dec 3, 2020

Uh oh!

MariusVolkhart commented Dec 3, 2020

Uh oh!

mlbridge bot commented Dec 3, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Webrevs

Uh oh!

JoeWang-Java commented Dec 4, 2020

Uh oh!

JoeWang-Java Dec 3, 2020

Choose a reason for hiding this comment

Uh oh!

MariusVolkhart Dec 8, 2020

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

openjdk bot commented Dec 8, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

JoeWang-Java commented Dec 8, 2020

Uh oh!

openjdk bot commented Dec 8, 2020

Uh oh!

JoeWang-Java commented Dec 8, 2020

Uh oh!

MariusVolkhart commented Dec 8, 2020

Uh oh!

openjdk bot commented Dec 8, 2020

Uh oh!

JoeWang-Java commented Dec 8, 2020

Uh oh!

openjdk bot commented Dec 8, 2020

Uh oh!

Uh oh!

MariusVolkhart commented Nov 4, 2020 •

edited by openjdk bot

Loading

JoeWang-Java commented Nov 19, 2020 •

edited

Loading

mlbridge bot commented Dec 3, 2020 •

edited

Loading

openjdk bot commented Dec 8, 2020 •

edited

Loading