Re: RFR: 8296546: Add @spec tags to API [v3]
On Wed, 23 Nov 2022 18:57:03 GMT, Jonathan Gibbons wrote: >> Please review a "somewhat automated" change to insert `@spec` tags into doc >> comments, as appropriate, to leverage the recent new javadoc feature to >> generate a new page listing the references to all external specifications >> listed in the `@spec` tags. >> >> "Somewhat automated" means that I wrote and used a temporary utility to scan >> doc comments looking for HTML links to selected sites, such as `ietf.org`, >> `unicode.org`, `w3.org`. These links may be in the main description of a doc >> comment, or in `@see` tags. For each link, the URL is examined, and >> "normalized", and inserted into the doc comment with a new `@spec` tag, >> giving the link and tile for the spec. >> >> "Normalized" means... >> * Use `https:` where possible (includes pretty much all cases) >> * Use a single consistent host name for all URLs coming from the same spec >> site (i.e. don't use different aliases for the same site) >> * Point to the root page of a multi-page spec >> * Use a consistent form of the spec, preferring HTML over plain text where >> both are available (this mostly applies to IETF specs) >> >> In addition, a "standard" title is determined for all specs, determined >> either from the content of the (main) spec page or from site index pages. >> >> The net effect is (or should be) that **all** the changes are to just >> **add** new `@spec` tags, based on the links found in each doc comment. >> There should be no other changes to the doc comments, or to the >> implementation of any classes and interfaces. >> >> That being said, the utility I wrote does have additional abilities, to >> update the links that it finds (e.g. changing to use `https:` etc,) but >> those features are _not_ being used here, but could be used in followup PRs >> if component teams so desired. I did notice while working on this overall >> feature that many of our links do point to "outdated" pages, some with >> eye-catching notices declaring that the spec has been superseded. >> Determining how, when and where to update such links is beyond the scope of >> this PR. >> >> Going forward, it is to be hoped that component teams will maintain the >> underlying links, and the URLs in `@spec` tags, such that if references to >> external specifications are updated, this will include updating the `@spec` >> tags. >> >> To see the effect of all these new `@spec` tags, see >> http://cr.openjdk.java.net/~jjg/8296546/api.00/ >> >> In particular, see the new [External >> Specifications](http://cr.openjdk.java.net/~jjg/8296546/api.00/external-specs.html) >> page, which you can also find via the new link near the top of the >> [Index](http://cr.openjdk.java.net/~jjg/8296546/api.00/index-files/index-1.html) >> pages. > > Jonathan Gibbons has updated the pull request incrementally with one > additional commit since the last revision: > > Remove updates from unexported files src/java.desktop/share/classes/java/awt/package-info.java line 58: > 56: * The AWT Modality > 57: * > 58: * The Java AWT Native Interface (JAWT) Why only 1 of these 3 ? src/java.desktop/share/classes/java/awt/package-info.java line 62: > 60: * > 61: * @spec AWT_Native_Interface.html The Java AWT Native Interface > Specification and Guide > 62: * @since 1.0 I wonder if links to html we include in the javadoc should be really treated in the same manner as referecnes to externally defined specifactions ? But I also wonder why only the native_interface spec was added and not the other two ? src/java.desktop/share/classes/javax/imageio/plugins/tiff/BaselineTIFFTagSet.java line 226: > 224: * @spec https://www.ietf.org/rfc/rfc1951.html RFC 1951: DEFLATE > Compressed Data Format Specification version 1.3 > 225: * @see #TAG_COMPRESSION > 226: * @see https://tools.ietf.org/html/rfc1951";>DEFLATE > specification Does having @spec and @see mean we have two clickable links to the same place adjacent to each other ? - PR: https://git.openjdk.org/jdk/pull/11073
Re: [jdk20] RFR: 8298133: JDK 20 RDP1 L10n resource files update - msgdrop 10 [v6]
On Fri, 16 Dec 2022 17:02:42 GMT, Damon Nguyen wrote: >> Open l10n drop >> All tests passed > > Damon Nguyen has updated the pull request incrementally with one additional > commit since the last revision: > > Fix missing dash src/demo/share/jfc/SwingSet2/resources/swingset_zh_CN.properties line 430: > 428: > ProgressBarDemo.accessible_text_area_name=\u52A0\u8F7D\u7684\u6587\u672C\u6B63\u5728\u9010\u6E10\u589E\u591A > 429: > 430: ProgressBarDemo.accessible_text_area_description=\u8FD9\u4E2A JTextArea > \u7531\u6765\u81EA\u7F13\u51B2\u533A\u7684\u6587\u672C\u9010\u4E2A\u5B57\u7B26\u5730\u586B\u5145, > > \u540C\u65F6\u7A97\u53E3\u5E95\u90E8\u7684\u8FDB\u5EA6\u680F\u5C06\u663E\u793A\u52A0\u8F7D\u7684\u8FDB\u5EA6 I was asked to review SwingSet2 changes but this isn't code. But apparently I am not the right person to review this since I cannot understand the changes (not a DE, CN, or JA language expert) nor the reason for them. If this is a change to a unicode comma (as implied in an off-review message), I have no idea why .. it seems to be just asking for trouble. - PR: https://git.openjdk.org/jdk20/pull/35
Re: [jdk20] RFR: 8300719: JDK 20 RDP2 L10n resource files update [v2]
On Tue, 24 Jan 2023 22:12:26 GMT, Damon Nguyen wrote: >> Open l10n drop. Files have been updated with translated versions. Whitespace >> tool has been ran on files. >> All tests passed > > Damon Nguyen has updated the pull request incrementally with one additional > commit since the last revision: > > Change German help of jar command Removing the trailing white space seems OK in the desktop resources. I checked the base (English) version of the files and none of them have trailing white space so it seems unlikely it is significant. - Marked as reviewed by prr (Reviewer). PR: https://git.openjdk.org/jdk20/pull/116
Re: RFR: 8303480: Miscellaneous fixes to mostly invisible doc comments
On Thu, 2 Mar 2023 12:03:44 GMT, Pavel Rappo wrote: > Please review this superficial documentation cleanup that was triggered by > unrelated analysis of doc comments in JDK API. > > The only effect that this multi-area PR has on the JDK API Documentation > (i.e. the observable effect on the generated HTML pages) can be summarized as > follows: > > > diff -ur build/macosx-aarch64/images/docs-before/api/serialized-form.html > build/macosx-aarch64/images/docs-after/api/serialized-form.html > --- build/macosx-aarch64/images/docs-before/api/serialized-form.html > 2023-03-02 11:47:44 > +++ build/macosx-aarch64/images/docs-after/api/serialized-form.html > 2023-03-02 11:48:45 > @@ -17084,7 +17084,7 @@ > throws href="java.base/java/io/IOException.html" title="class in > java.io">IOException, > ClassNotFoundException > readObject is called to restore the > state of the > - (@code BasicPermission} from a stream. > + BasicPermission from a stream. > > Parameters: > s - the ObjectInputStream from which data > is read > > Notes > - > > * I'm not an expert in any of the affected areas, except for jdk.javadoc, and > I was merely after misused tags. Because of that, I would appreciate reviews > from experts in other areas. > * I discovered many more issues than I included in this PR. The excluded > issues seem to occur in infrequently updated third-party code (e.g. > javax.xml), which I assume we shouldn't touch unless necessary. > * I will update copyright years after (and if) the fix had been approved, as > required. java.desktop changes are fine - Marked as reviewed by prr (Reviewer). PR: https://git.openjdk.org/jdk/pull/12826
Re: RFR: 8304915: Create jdk.internal.util.Architecture enum and apply [v7]
On Thu, 6 Apr 2023 19:25:19 GMT, Roger Riggs wrote:
>> Define an internal jdk.internal.util.Architecture enumeration and static
>> methods to replace uses of the system property `os.arch`.
>> The enumeration values are defined to match those used in the build.
>> The initial values are: `X64, X86, IA64, ARM, AARCH64, RISCV64, S390X,
>> PPC64LE`
>> Note that `amd64` and `x86_64` in the build are represented by `X64`.
>> The values of the system property `os.arch` is unchanged.
>>
>> The API is similar to the jdk.internal.util.OperatingSystem enum created by
>> #[12931](https://git.openjdk.org/jdk/pull/12931).
>> Uses in `java.base` and a few others are included but other modules will be
>> done in separate PRs.
>
> Roger Riggs has updated the pull request incrementally with one additional
> commit since the last revision:
>
> Remove unneeded qualified export from java.base to jdk.attach
src/jdk.accessibility/windows/classes/com/sun/java/accessibility/internal/AccessBridge.java
line 169:
> 167: // Load the appropriate DLLs
> 168: boolean is32on64 = false;
> 169: if (Architecture.isX86()) {
This would be another coupling of java.desktop into java.base internals and I
don't think it worth it.
Particularly because this check is obsolete !
It is a remnant of when accessbridge was an unbundled download so provided both
32 and 64 bit.
We don't even have the file it is looking for in the build - even if someone
did a 32 bit build.
Instead of making this change a bug should be filed to remove this code.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/13357#discussion_r1160314934
Re: RFR: 8308286 Fix clang warnings in linux code
On Wed, 17 May 2023 12:28:47 GMT, Artem Semenov wrote: > When using the clang compiler to build OpenJDk on Linux, we encounter various > "warnings as errors". > They can be fixed with small changes. I don't like this approach at all. if github had a "reject" button I'd be pushing it now. updating the makefiles is the normal way to do this but I don't know if we even want that. Until clang is the supported compiler for Linux then I see no reason for this to be in JDK at all. Code changes which make it so there's no warning would be the preferred way but I'd expect that to be tested properly. Also if the warning is spurious - sometimes they are - then that would NOT be appropriate. - Changes requested by prr (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/14033#pullrequestreview-1437172390
Re: RFR: JDK-8308288: Fix xlc17 clang warnings in shared code [v2]
On Fri, 26 May 2023 08:31:46 GMT, JoKern65 wrote: >> When using the new xlc17 compiler (based on a recent clang) to build OpenJDk >> on AIX , we run into various "warnings as errors". >> Some of those are in shared codebase and could be addressed by small >> adjustments. >> A lot of those changes are in hotspot, some might be somewhere else in the >> OpenJDK C/C++ code. > > JoKern65 has updated the pull request incrementally with one additional > commit since the last revision: > > forgotton _ I am Ok with the client-libs changes here. I did not look at anything else. So you'll need more approvals before its ready to push. - Marked as reviewed by prr (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/14146#pullrequestreview-1454101589
Re: RFR: 8310187: Improve Generational ZGC jtreg testing [v3]
On Mon, 19 Jun 2023 06:55:52 GMT, Axel Boldt-Christmas
wrote:
>> The current implementation for testing generational ZGC with jtreg is
>> implemented with a filter on the mode flag `ZGenerational`. Because of this
>> only environments which set this flag explicitly will run most of the tests.
>> So they get missed in Github Actions and for developers running jtreg
>> locally without supplying the `ZGenerational` flag.
>>
>> The proposed change here is to introduce two new jtreg requirement
>> properties, `vm.gc.ZGenerational` and `vm.gc.ZSingelgen`. These flags will
>> effectively behave the same as the existing `vm.gc.` flags but also take
>> the specific ZGC mode in account.
>>
>> If no gc flags are supplied to jtreg and the `vm.gc.Z` is true (the build
>> includes ZGC) both `vm.gc.ZGenerational` and `vm.gc.ZSinglegen` will be true.
>>
>> If `-XX:+UseZGC` is supplied then both `vm.gc.ZGenerational` and
>> `vm.gc.ZSinglegen` will also be true.
>>
>> If `-XX:{+,-}ZGenerational` is supplied then either `vm.gc.ZGenerational`
>> or `vm.gc.ZSinglegen` be true depending on the flags value.
>>
>> And if `vm.gc.Z` is false both `vm.gc.ZGenerational` and `vm.gc.ZSinglegen`
>> will be false.
>>
>> This change also splits the relevant tests into two distinct runs for the
>> two modes. And the respective test ids are set to `ZGenerational` or
>> `ZSinglegen` to make it easier to distinguish the runs.
>>
>> This also solves the issue that some compiler tests will never run with
>> generational ZGC unless the `TEST_VM_FLAGLESS` is set. This is because the
>> current filter `vm.opt.final.ZGenerational` requires the flag to be
>> explicit, but these compiler tests uses `vm.flagless`.
>>
>> The introduction of `vm.gc.ZGenerational` and `vm.gc.ZSinglegen` harmonizes
>> the way you specify generational / single gen ZGC test with the way it is
>> done for other gcs with `vm.gc.`
>>
>> To support this feature the Whitebox API is extended with `isDefaultVMFlag`
>> to enable checking if `ZGenerational` is default or not.
>>
>> `vm.opt.final.ZGenerational` is still kept because we still have some
>> reasons to filter based on the supplied flags.
>> - `test/hotspot/jtreg/gc/cslocker/TestCSLocker.java` is disabled when
>> running with ZGenerational
>> - `test/jdk/java/lang/ProcessBuilder/CloseRace.java` is ran with a different
>> max heap size for ZGenerational, but it is not the intent to dispatch the
>> test to both G1 and generational ZGC if Generational ZGC is not specified.
>>
>> `test/jdk/java/lang/management/MemoryMXBean/MemoryTest.java` was also
>> changed to not filter but instead dispatch. However u...
>
> Axel Boldt-Christmas has updated the pull request incrementally with one
> additional commit since the last revision:
>
> Add explicit id to all Skynet.java @test
I have no issues with the enhanced Java 2D tests to cover both generational and
original ZGC.
But I'll leave it to hotspot reviews to formally approve the fix since those
tests are such a tiny part of this.
-
PR Comment: https://git.openjdk.org/jdk/pull/14509#issuecomment-1599445397
Re: RFR: 8292350: Use static methods for hashCode/toString primitives
On Wed, 10 Aug 2022 08:01:41 GMT, Andrey Turbanov wrote: > It's a bit shorter and clearer. Approving (just) the client change - Marked as reviewed by prr (Reviewer). PR: https://git.openjdk.org/jdk/pull/9816
Re: RFR: 8289610: Degrade Thread.stop [v2]
On Wed, 14 Sep 2022 14:09:52 GMT, Alan Bateman wrote: >> Degrade Thread.stop to throw UOE unconditionally, deprecate ThreadDeath for >> removal, and remove the remaining special handling of ThreadDeath from the >> JDK. >> >> Thread.stop is inherently unsafe and has been deprecated since JDK 1.2 >> (1998) with a link to a supplementary page that explains the rationale. Some >> of the API surface has already been degraded or removed: >> Thread.stop(Throwable) was degraded to throw UOE in Java 8 and removed in >> Java 11, and ThreadGroup.stop was degraded to throw UOE in Java 19. As of >> Java 19, the no-arg Thread.stop continues to work as before for platform >> threads but throws UOE for virtual threads. The next step in the glacial >> pace removal is the degrading of the no-arg Thread.stop method to throw UOE >> for all threads. >> >> To keep things manageable, the change proposed here leaves JVM_StopThread in >> place. A separate issue will remove it and do other cleanup/removal in the >> VM. We have another JBS issue for the updates to the JLS and JVMS where >> asynchronous exceptions are defined. There is also some remaining work on a >> test class used by 6 jshell tests - if they aren't done in time then we will >> temporarily exclude them. >> >> The change here has no impact on the debugger APIs (JVM TI StopThread, JDWP >> ThreadReference/Stop, and JDI ThreadReference.stop). Debuggers can continue >> to cause threads to throw an asynchronous exception, as might be done when >> simulating code throwing an exception at some point in the code. > > Alan Bateman has updated the pull request with a new target base due to a > merge or a rebase. The incremental webrev excludes the unrelated changes > brought in by the merge/rebase. The pull request contains six additional > commits since the last revision: > > - Merge > - Remove stopThread permission from RuntimePermission table, exclude jshell > tests > - Deprecate for removal > - Next iteration, update dates in headers > - Merge > - Initial commit Marked as reviewed by prr (Reviewer). - PR: https://git.openjdk.org/jdk/pull/10230
Re: RFR: JDK-8294618: Update openjdk.java.net => openjdk.org [v4]
On Fri, 30 Sep 2022 14:16:24 GMT, Weijun Wang wrote: >> src/jdk.accessibility/windows/native/include/bridge/AccessBridgeCalls.h line >> 34: >> >>> 32: * the following link: >>> 33: * >>> 34: * >>> http://hg.openjdk.org/jdk9/jdk9/jdk/raw-file/tip/src/jdk.accessibility/windows/native/bridge/AccessBridgeCalls.c >> >> The domain for hg wasn't changed to openjdk.org. The original URL is >> correct. > > Why do we need to link to a URL? Why not `../../bridge/AccessBridgeCalls.c`? This is correct. AccessBridge.h is published with the include/header files of the JDK and anyone reading it there can't exactly make use of "../" - PR: https://git.openjdk.org/jdk/pull/10501
Re: RFR: JDK-8294618: Update openjdk.java.net => openjdk.org [v7]
On Mon, 3 Oct 2022 17:29:45 GMT, Joe Darcy wrote: >> With the domain change from openjdk.java.net to openjdk.org, references to >> URLs in the sources should be updated. >> >> Updates were made using a shell script. I"ll run a copyright updater before >> any push. > > Joe Darcy has updated the pull request incrementally with one additional > commit since the last revision: > > Update make directory. src/jdk.accessibility/windows/native/include/bridge/AccessBridgeCalls.h line 36: > 34: * > https://git.openjdk.org/jdk/blob/master/src/jdk.accessibility/windows/native/bridge/AccessBridgeCalls.c > 35: * > 36: * Also note that the API is used in the jaccessinspector and > jaccesswalker tools. The problem with this is, is that anyone who gets JDK 20 (or 21 the LTS) will be forever more then pointed at the OpenJDK "tip" and if we made an incompatible ABI change, that would be a problem. At this point I'd prefer that this be updated to point to JDK 17, as in https://github.com/openjdk/jdk17/blob/master/src/jdk.accessibility/windows/native/jaccesswalker/jaccesswalker.cpp So it is a defined, known, compatible version. - PR: https://git.openjdk.org/jdk/pull/10501
Re: RFR: JDK-8294618: Update openjdk.java.net => openjdk.org [v8]
On Mon, 3 Oct 2022 20:37:11 GMT, Joe Darcy wrote: >> With the domain change from openjdk.java.net to openjdk.org, references to >> URLs in the sources should be updated. >> >> Updates were made using a shell script. I"ll run a copyright updater before >> any push. > > Joe Darcy has updated the pull request with a new target base due to a merge > or a rebase. The incremental webrev excludes the unrelated changes brought in > by the merge/rebase. The pull request contains 11 additional commits since > the last revision: > > - Update accessibility URLs. > - Merge branch 'master' into JDK-8294618 > - Update make directory. > - Update doc directory files. > - Update hg URLs to git. > - Merge branch 'master' into JDK-8294618 > - http -> https > - Undo manpage update. > - Update copyright. > - Revert unintended update to binary file. > - ... and 1 more: https://git.openjdk.org/jdk/compare/571e4932...4055f1a6 Marked as reviewed by prr (Reviewer). - PR: https://git.openjdk.org/jdk/pull/10501
Re: RFR: 8295729: Remove trailing whitespace from non-value lines in properties files [v2]
On Fri, 21 Oct 2022 08:17:46 GMT, Magnus Ihse Bursie wrote: >> Properties files is essentially source code. It should have the same >> whitespace checks as all other source code, so we don't get spurious >> trailing whitespace changes. >> >> With the new Skara jcheck, it is possible to increase the coverage of the >> whitespace checks (in the old mercurial version, this was more or less >> impossible). >> >> The only manual change is to `.jcheck/conf`. All other changes were made by >> running `find . -type f -iname "*.properties" | xargs gsed -i -e 's/[ >> \t]*$//'`. > > Magnus Ihse Bursie has updated the pull request incrementally with two > additional commits since the last revision: > > - Remove check for .properties from jcheck > - Restore trailing whitespace for property values Marked as reviewed by prr (Reviewer). - PR: https://git.openjdk.org/jdk/pull/10792
Re: RFR: 8294241: Deprecate URL public constructors [v2]
On Fri, 28 Oct 2022 14:54:26 GMT, Daniel Fuchs wrote:
>> Deprecate URL constructors. Developers are encouraged to use `java.net.URI`
>> to parse or construct any URL.
>>
>> The `java.net.URL` class does not itself encode or decode any URL components
>> according to the escaping mechanism defined in RFC2396. It is the
>> responsibility of the caller to encode any fields, which need to be escaped
>> prior to calling URL, and also to decode any escaped fields, that are
>> returned from URL.
>>
>> This has lead to many issues in the past. Indeed, if used improperly, there
>> is no guarantee that `URL::toString` or `URL::toExternalForm` will lead to a
>> URL string that can be parsed back into the same URL. This can lead to
>> constructing misleading URLs. Another issue is with `equals()` and
>> `hashCode()` which may have to perform a lookup, and do not take
>> encoding/escaping into account.
>>
>> In Java SE 1.4 a new class, `java.net.URI`, has been added to mitigate some
>> of the shortcoming of `java.net.URL`. Conversion methods to create a URL
>> from a URI were also added. However, it was left up to the developers to use
>> `java.net.URI`, or not. This RFE proposes to deprecate all public
>> constructors of `java.net.URL`, in order to provide a stronger warning about
>> their potential misuses. To construct a URL, using `URI::toURL` should be
>> preferred.
>>
>> In order to provide an alternative to the constructors that take a stream
>> handler as parameter, a new factory method `URL::fromURI(java.net.URI,
>> java.net.URLStreamHandler)` is provided as part of this change.
>>
>> Places in the JDK code base that were constructing `java.net.URL` have been
>> temporarily annotated with `@SuppressWarnings("deprecation")`. Some related
>> issues will be logged to revisit the calling code.
>>
>> The CSR can be reviewed here: https://bugs.openjdk.org/browse/JDK-8295949
>
> Daniel Fuchs has updated the pull request with a new target base due to a
> merge or a rebase. The incremental webrev excludes the unrelated changes
> brought in by the merge/rebase. The pull request contains four additional
> commits since the last revision:
>
> - Updated after review comments. In particular var tmp => var => _unused -
> and avoid var in java.xml
> - Merge branch 'master' into deprecate-url-ctor-8294241
> - Fix whitespace issues
> - 8294241
Deprecate URL constructors.
Developers are encouraged to use java.net.URI to parse or construct any URL.
...
To construct a URL, using URI::toURL should be preferred.
You have jumped through some refactoring hoops to be able to apply the
deprecation suppression
to as little code as possible .. having made such changes, then why didn't you
just make the
recommended change instead ?
Should I presume that the recommended route will have some nasty little
incompatibilities
we will need to be careful of first ?
And what about Peter Firmstone's comment
"We stopped using java.net.URI some years ago as it's obsolete.?"
I can't reconcile that with the recommendation to use it ..
-
PR: https://git.openjdk.org/jdk/pull/10874
Re: RFR: 8294241: Deprecate URL public constructors [v3]
On Tue, 1 Nov 2022 16:10:47 GMT, Daniel Fuchs wrote:
>> Deprecate URL constructors. Developers are encouraged to use `java.net.URI`
>> to parse or construct any URL.
>>
>> The `java.net.URL` class does not itself encode or decode any URL components
>> according to the escaping mechanism defined in RFC2396. It is the
>> responsibility of the caller to encode any fields, which need to be escaped
>> prior to calling URL, and also to decode any escaped fields, that are
>> returned from URL.
>>
>> This has lead to many issues in the past. Indeed, if used improperly, there
>> is no guarantee that `URL::toString` or `URL::toExternalForm` will lead to a
>> URL string that can be parsed back into the same URL. This can lead to
>> constructing misleading URLs. Another issue is with `equals()` and
>> `hashCode()` which may have to perform a lookup, and do not take
>> encoding/escaping into account.
>>
>> In Java SE 1.4 a new class, `java.net.URI`, has been added to mitigate some
>> of the shortcoming of `java.net.URL`. Conversion methods to create a URL
>> from a URI were also added. However, it was left up to the developers to use
>> `java.net.URI`, or not. This RFE proposes to deprecate all public
>> constructors of `java.net.URL`, in order to provide a stronger warning about
>> their potential misuses. To construct a URL, using `URI::toURL` should be
>> preferred.
>>
>> In order to provide an alternative to the constructors that take a stream
>> handler as parameter, a new factory method `URL::fromURI(java.net.URI,
>> java.net.URLStreamHandler)` is provided as part of this change.
>>
>> Places in the JDK code base that were constructing `java.net.URL` have been
>> temporarily annotated with `@SuppressWarnings("deprecation")`. Some related
>> issues will be logged to revisit the calling code.
>>
>> The CSR can be reviewed here: https://bugs.openjdk.org/browse/JDK-8295949
>
> Daniel Fuchs has updated the pull request with a new target base due to a
> merge or a rebase. The incremental webrev excludes the unrelated changes
> brought in by the merge/rebase. The pull request contains six additional
> commits since the last revision:
>
> - Integrated review feedback
> - Merge branch 'master' into deprecate-url-ctor-8294241
> - Updated after review comments. In particular var tmp => var => _unused -
> and avoid var in java.xml
> - Merge branch 'master' into deprecate-url-ctor-8294241
> - Fix whitespace issues
> - 8294241
Marked as reviewed by prr (Reviewer).
-
PR: https://git.openjdk.org/jdk/pull/10874
Re: RFR: JDK-8315897: some PrivilegedActions missing in JDK code for getting properties
On Tue, 12 Sep 2023 08:21:35 GMT, Matthias Baesken wrote:
> > > So what about FontConfiguration? Is that something using the class
> > > directly too?
> >
> >
> > I think this is not needed either. As far as I can see, the instance of
> > `FontConfiguration` is created from `doPrivileged`, therefore these two
> > system properties are read inside `doPrivileged` already.
>
> Hi there is a public constructor ` public FontConfiguration(SunFontManager
> fm) {` calling setOsNameAndVersion(), so is it really always called from
> `doPrivileged` ? (however it is currently only exported in a qualified way)
Regardless of that, these properties are allowed to be read without permissions.
See java.policy
grant {
...
// "standard" properies that can be read by anyone
permission java.util.PropertyPermission "os.name", "read";
permission java.util.PropertyPermission "os.version", "read";
-
PR Comment: https://git.openjdk.org/jdk/pull/15629#issuecomment-1726467543
Re: RFR: 8325950: Make sure all files in the JDK pass jcheck
On Thu, 15 Feb 2024 12:19:31 GMT, Magnus Ihse Bursie wrote: > Since jcheck only checks file in a commit, there is a possibility of us > getting files in the repository that would not be accepted by jcheck. This > can happen when extending the set of files checked by jcheck, or if jcheck > changes how it checks files (perhaps due to bug fixes). > > I have now run jcheck over the entire code base, and fixed a handful of > issues. With these changes, jcheck accept the entire code base. desktop changes look fine to me. - Marked as reviewed by prr (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/17871#pullrequestreview-1884009342
Re: RFR: 8303689: javac -Xlint could/should report on "dangling" doc comments [v10]
On Fri, 26 Apr 2024 16:04:09 GMT, Jonathan Gibbons wrote:
>> Please review the updates to support a proposed new
>> `-Xlint:dangling-doc-comments` option.
>>
>> The work can be thought of as in 3 parts:
>>
>> 1. An update to the `javac` internal class `DeferredLintHandler` so that it
>> is possible to specify the appropriately configured `Lint` object when it is
>> time to consider whether to generate the diagnostic or not.
>>
>> 2. Updates to the `javac` front end to record "dangling docs comments" found
>> near the beginning of a declaration, and to report them using an instance of
>> `DeferredLintHandler`. This allows the warnings to be enabled or disabled
>> using the standard mechanisms for `-Xlint` and `@SuppressWarnings`. The
>> procedure for handling dangling doc comments is described in this comment in
>> `JavacParser`.
>>
>> * Dangling documentation comments are handled as follows.
>> * 1. {@code Scanner} adds all doc comments to a queue of
>> * recent doc comments. The queue is flushed whenever
>> * it is known that the recent doc comments should be
>> * ignored and should not cause any warnings.
>> * 2. The primary documentation comment is the one obtained
>> * from the first token of any declaration.
>> * (using {@code token.getDocComment()}.
>> * 3. At the end of the "signature" of the declaration
>> * (that is, before any initialization or body for the
>> * declaration) any other "recent" comments are saved
>> * in a map using the primary comment as a key,
>> * using this method, {@code saveDanglingComments}.
>> * 4. When the tree node for the declaration is finally
>> * available, and the primary comment, if any,
>> * is "attached", (in {@link #attach}) any related
>> * dangling comments are also attached to the tree node
>> * by registering them using the {@link #deferredLintHandler}.
>> * 5. (Later) Warnings may be genereated for the dangling
>> * comments, subject to the {@code -Xlint} and
>> * {@code @SuppressWarnings}.
>>
>>
>> 3. Updates to the make files to disable the warnings in modules for which
>> the
>> warning is generated. This is often because of the confusing use of `/**` to
>> create box or other standout comments.
>
> Jonathan Gibbons has updated the pull request incrementally with one
> additional commit since the last revision:
>
> fix typo
Marked as reviewed by prr (Reviewer).
-
PR Review: https://git.openjdk.org/jdk/pull/18527#pullrequestreview-2025744069
Re: RFR: 8331671: Implement JEP 472: Prepare to Restrict the Use of JNI [v8]
On Fri, 17 May 2024 13:38:25 GMT, Maurizio Cimadamore wrote: >> This PR implements [JEP 472](https://openjdk.org/jeps/472), by restricting >> the use of JNI in the following ways: >> >> * `System::load` and `System::loadLibrary` are now restricted methods >> * `Runtime::load` and `Runtime::loadLibrary` are now restricted methods >> * binding a JNI `native` method declaration to a native implementation is >> now considered a restricted operation >> >> This PR slightly changes the way in which the JDK deals with restricted >> methods, even for FFM API calls. In Java 22, the single >> `--enable-native-access` was used both to specify a set of modules for which >> native access should be allowed *and* to specify whether illegal native >> access (that is, native access occurring from a module not specified by >> `--enable-native-access`) should be treated as an error or a warning. More >> specifically, an error is only issued if the `--enable-native-access flag` >> is used at least once. >> >> Here, a new flag is introduced, namely >> `illegal-native-access=allow/warn/deny`, which is used to specify what >> should happen when access to a restricted method and/or functionality is >> found outside the set of modules specified with `--enable-native-access`. >> The default policy is `warn`, but users can select `allow` to suppress the >> warnings, or `deny` to cause `IllegalCallerException` to be thrown. This >> aligns the treatment of restricted methods with other mechanisms, such as >> `--illegal-access` and the more recent `--sun-misc-unsafe-memory-access`. >> >> Some changes were required in the package-info javadoc for >> `java.lang.foreign`, to reflect the changes in the command line flags >> described above. > > Maurizio Cimadamore has updated the pull request incrementally with one > additional commit since the last revision: > > Address review comments Have you looked into / thought about how this will work for jpackaged apps ? I suspect that both the existing FFM usage and this will be options the application packager will need to supply when building the jpackaged app - the end user cannot pass in command line VM options. Seems there should be some testing of this as some kind of native access could be a common case for jpackaged apps. - PR Review: https://git.openjdk.org/jdk/pull/19213#pullrequestreview-2066794950
Re: RFR: 8331671: Implement JEP 472: Prepare to Restrict the Use of JNI [v8]
On Mon, 13 May 2024 10:49:30 GMT, Maurizio Cimadamore
wrote:
>> Maurizio Cimadamore has updated the pull request incrementally with one
>> additional commit since the last revision:
>>
>> Address review comments
>
> make/conf/module-loader-map.conf line 105:
>
>> 103: java.smartcardio \
>> 104: jdk.accessibility \
>> 105: jdk.attach \
>
> The list of allowed modules has been rewritten from scratch, by looking at
> the set of modules containing at least one `native` method declaration.
Should I understand this list to be the set of modules exempt from needing to
specific that native access is allowed ?
ie they always have native access without any warnings, and further that any
attempt to enable warnings, or to disable native access for these modules is
ignored ?
> src/java.desktop/macosx/classes/com/apple/eio/FileManager.java line 61:
>
>> 59: }
>> 60:
>> 61: @SuppressWarnings({"removal", "restricted"})
>
> There are several of these changes. One option might have been to just
> disable restricted warnings when building. But on a deeper look, I realized
> that in all these places we already disabled deprecation warnings for the use
> of security manager, so I also added a new suppression instead.
Sounds reasonable.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/19213#discussion_r1607136237
PR Review Comment: https://git.openjdk.org/jdk/pull/19213#discussion_r1607136808
Re: RFR: 8331671: Implement JEP 472: Prepare to Restrict the Use of JNI [v8]
On Fri, 17 May 2024 13:38:25 GMT, Maurizio Cimadamore wrote: >> This PR implements [JEP 472](https://openjdk.org/jeps/472), by restricting >> the use of JNI in the following ways: >> >> * `System::load` and `System::loadLibrary` are now restricted methods >> * `Runtime::load` and `Runtime::loadLibrary` are now restricted methods >> * binding a JNI `native` method declaration to a native implementation is >> now considered a restricted operation >> >> This PR slightly changes the way in which the JDK deals with restricted >> methods, even for FFM API calls. In Java 22, the single >> `--enable-native-access` was used both to specify a set of modules for which >> native access should be allowed *and* to specify whether illegal native >> access (that is, native access occurring from a module not specified by >> `--enable-native-access`) should be treated as an error or a warning. More >> specifically, an error is only issued if the `--enable-native-access flag` >> is used at least once. >> >> Here, a new flag is introduced, namely >> `illegal-native-access=allow/warn/deny`, which is used to specify what >> should happen when access to a restricted method and/or functionality is >> found outside the set of modules specified with `--enable-native-access`. >> The default policy is `warn`, but users can select `allow` to suppress the >> warnings, or `deny` to cause `IllegalCallerException` to be thrown. This >> aligns the treatment of restricted methods with other mechanisms, such as >> `--illegal-access` and the more recent `--sun-misc-unsafe-memory-access`. >> >> Some changes were required in the package-info javadoc for >> `java.lang.foreign`, to reflect the changes in the command line flags >> described above. > > Maurizio Cimadamore has updated the pull request incrementally with one > additional commit since the last revision: > > Address review comments client parts look fine. - Marked as reviewed by prr (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/19213#pullrequestreview-2069134455
Re: RFR: 8333827: JDK 23 RDP1 L10n resource files update
On Fri, 7 Jun 2024 22:46:44 GMT, Damon Nguyen wrote: > This issue is responsible for updating the translations of all the > localize(able) resources in the JDK. Primarily, the changes between JDK 22 > RDP 1 and the integration of the JDK 23 RDP 1 L10n drop will be translated. > > The translation tool adjusted some definitions, which causes some changes in > localized files where the source file had no changes. This causes some words > being reverted from localized languages to English, and some had its > definitions changed. > > Alternatively, the diffs are viewable here and was generated using Jonathan > Gibbons' diff tool for l10n: > https://cr.openjdk.org/~dnguyen/output2/ src/java.desktop/share/classes/sun/print/resources/serviceui_de.properties line 66: > 64: label.size=G&röße: > 65: label.source=&Quelle: > 66: label.outputbins=A&usgabefächer: How confident are we in this ? google translate says that the old text referred to 'compartments' and the new one to 'areas' and I wonder why that is better ? https://translate.google.com/?sl=auto&tl=en&text=Ausgabebereiche%0AAusgebefacher&op=translate src/java.desktop/share/classes/sun/print/resources/serviceui_zh_CN.properties line 32: > 30: border.jobattributes=作业属性 > 31: border.media=介质 > 32: border.output=出纸 What triggered this ? Just someone noticing it and deciding there's a better translation ? What does it say ? - PR Review Comment: https://git.openjdk.org/jdk/pull/19609#discussion_r1633781646 PR Review Comment: https://git.openjdk.org/jdk/pull/19609#discussion_r1633782742
Re: RFR: 8333827: JDK 23 RDP1 L10n resource files update [v3]
On Tue, 11 Jun 2024 19:28:27 GMT, Damon Nguyen wrote: >> This issue is responsible for updating the translations of all the >> localize(able) resources in the JDK. Primarily, the changes between JDK 22 >> RDP 1 and the integration of the JDK 23 RDP 1 L10n drop will be translated. >> >> The translation tool adjusted some definitions, which causes some changes in >> localized files where the source file had no changes. This causes some words >> being reverted from localized languages to English, and some had its >> definitions changed. >> >> Alternatively, the diffs are viewable here and was generated using Jonathan >> Gibbons' diff tool for l10n: >> https://cr.openjdk.org/~dnguyen/output2/ > > Damon Nguyen has updated the pull request incrementally with one additional > commit since the last revision: > > Revert updated translation Marked as reviewed by prr (Reviewer). - PR Review: https://git.openjdk.org/jdk/pull/19609#pullrequestreview-2114049394
Re: RFR: 8336289: Obliterate most references to _snprintf in the Windows JDK [v3]
On Tue, 16 Jul 2024 08:59:20 GMT, Julian Waters wrote: >> snprintf has been available for all officially and unofficially supported >> compilers for Windows, Visual Studio since version 2015 and gcc since, well, >> forever. snprintf is conforming to C99 since the start when compiling using >> gcc, and 2015 when using Visual Studio. Since it conforms to C99 and >> provides better semantics than _snprintf, and is not compiler specific, we >> should use it in most places where we currently use _snprintf. This makes >> JDK code where this is used more robust due to the null terminating >> guarantee of snprintf. Since most of the changes are extremely simple, I've >> included all of them hoping to get this all done in one shot. The only >> places _snprintf is not replaced is places where I'm not sure whether the >> code is ours or not (As in, imported source code for external libraries). >> Note that the existing checks in place for the size of the characters >> written still work, as the return value of snprintf works mostly the same as >> _snprintf. The only difference is the nu ll terminating character at the end and the returning of the number of written characters if the buffer was terminated early, as seen here: https://learn.microsoft.com/en-us/cpp/c-runtime-library/reference/snprintf-snprintf-snprintf-l-snwprintf-snwprintf-l?view=msvc-170 >> >> Reviews Required: 2 for HotSpot, jdk.hotspot.agent, and jdk.jdwp.agent, 1 >> for awt and jdk.accessibility, 1 for java.base, 1 for security, 1 for >> jdk.management(?) > > Julian Waters has updated the pull request incrementally with one additional > commit since the last revision: > > Revert Standard Integer type rewrite AWT + A11Y - Marked as reviewed by prr (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/20153#pullrequestreview-2214138343
Re: RFR: 8338768: Introduce runtime lookup to check for static builds [v2]
On Wed, 21 Aug 2024 22:14:40 GMT, Magnus Ihse Bursie wrote: >> As a preparation for Hermetic Java, we need to have a way to look up during >> runtime if we are using a statically linked library or not. >> >> This change will be the first step needed towards compiling the object files >> only once, and then link them into either dynamic or static libraries. (The >> only exception will be the linktype.c[pp] files, which needs to be compiled >> twice, once for the dynamic libraries and once for the static libraries.) >> Getting there will require further work though. >> >> This is part of the changes that make up the draft PR >> https://github.com/openjdk/jdk/pull/19478, which I have broken out. > > Magnus Ihse Bursie has updated the pull request incrementally with one > additional commit since the last revision: > > Also update build to link properly AWT changes look fine. - Marked as reviewed by prr (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/20666#pullrequestreview-2252384240
Re: RFR: 8341692: Implement JEP 490: ZGC: Remove the Non-Generational Mode [v2]
On Wed, 9 Oct 2024 12:57:36 GMT, Axel Boldt-Christmas wrote: >> This is the implementation task for `JEP 490: ZGC: Remove the >> Non-Generational Mode`. See the JEP for details. >> [JDK-8335850](https://bugs.openjdk.org/browse/JDK-8335850) > > Axel Boldt-Christmas has updated the pull request incrementally with six > additional commits since the last revision: > > - LargeWindowPaintTest.java fix id typo > - Fix problem-listed @requires typo > - Fix @requires !vm.gc.Z, must use vm.gc != "Z" > - Reorder z_globals options: product > diagnostic product > develop > - Consistent albite special code style > - Consistent order between ZArguments and GCArguments the 2D/AWT test changes are fine. I've not looked at anything else. - Marked as reviewed by prr (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/21401#pullrequestreview-2357882378
Re: RFR: 8338411: Implement JEP 486: Permanently Disable the Security Manager [v2]
On Fri, 18 Oct 2024 19:03:30 GMT, Sean Mullan wrote:
>> This is the implementation of JEP 486: Permanently Disable the Security
>> Manager. See [JEP 486](https://openjdk.org/jeps/486) for more details. The
>> [CSR](https://bugs.openjdk.org/browse/JDK-8338412) describes in detail the
>> main changes in the JEP and also includes an apidiff of the specification
>> changes.
>>
>> NOTE: the majority (~95%) of the changes in this PR are test updates
>> (removal/modifications) and API specification changes, the latter mostly to
>> remove `@throws SecurityException`. The remaining changes are primarily the
>> removal of the `SecurityManager`, `Policy`, `AccessController` and other
>> Security Manager API implementations. There is very little new code.
>>
>> The code changes can be broken down into roughly the following categories:
>>
>> 1. Degrading the behavior of Security Manager APIs to either throw
>> Exceptions by default or provide an execution environment that disallows
>> access to all resources by default.
>> 2. Changing hundreds of methods and constructors to no longer throw a
>> `SecurityException` if a Security Manager was enabled. They will operate as
>> they did in JDK 23 with no Security Manager enabled.
>> 3. Changing the `java` command to exit with a fatal error if a Security
>> Manager is enabled.
>> 4. Removing the hotspot native code for the privileged stack walk and the
>> inherited access control context. The remaining hotspot code and tests
>> related to the Security Manager will be removed immediately after
>> integration - see [JDK-8341916](https://bugs.openjdk.org/browse/JDK-8341916).
>> 5. Removing or modifying hundreds of tests. Many tests that tested Security
>> Manager behavior are no longer relevant and thus have been removed or
>> modified.
>>
>> There are a handful of Security Manager related tests that are failing and
>> are at the end of the `test/jdk/ProblemList.txt`,
>> `test/langtools/ProblemList.txt` and `test/hotspot/jtreg/ProblemList.txt`
>> files - these will be removed or separate bugs will be filed before
>> integrating this PR.
>>
>> Inside the JDK, we have retained calls to
>> `SecurityManager::getSecurityManager` and `AccessController::doPrivileged`
>> for now, as these methods have been degraded to behave the same as they did
>> in JDK 23 with no Security Manager enabled. After we integrate this JEP,
>> those calls will be removed in each area (client-libs, core-libs, security,
>> etc).
>>
>> I don't expect each reviewer to review all the code changes in this JEP.
>> Rather, I advise that you only focus on the changes for the area
>> (client-libs, core-libs, net, ...
>
> Sean Mullan has updated the pull request with a new target base due to a
> merge or a rebase. The pull request now contains 97 commits:
>
> - Merge remote-tracking branch 'jdk-sandbox/jep486' into JDK-8338411
> - Change apiNote to deprecated annotation on checkAccess methods. Change
> method dedescription to "Does nothing".
> - Sanitize the class descriptions of DelegationPermission and
> ServicePermission
>by removing text that refers to granting permissions, but avoid changes
> that
>affect the API specification, such as the description and format of input
>parameters.
> - Restored methods in RMIConnection to throw SecurityExceptions again but
>with adjusted text that avoids the word "permission".
> - Add text to class description of MBeanServer stating that implementations
>may throw SecurityException if authorization doesn't allow access to
> resource.
> - Restore text about needing permissions from the desktop environment in the
>getPixelColor and createScreenCapture methods.
> - Add api note to getClassContext to use StackWalker instead and
>add DROP_METHOD_INFO option to StackWalker.
> - Change checkAccess() methods to be no-ops, rather than throwing
>SecurityException.
> - Merge
> - Merge
> - ... and 87 more: https://git.openjdk.org/jdk/compare/f50bd0d9...f89d9d09
test/jdk/javax/imageio/CachePremissionsTest/CachePermissionsTest.java line 55:
> 53: * @run main/othervm -Djava.security.manager=allow
> CachePermissionsTest false w.policy
> 54: * @run main/othervm -Djava.security.manager=allow
> CachePermissionsTest false rw.policy
> 55: * @run main/othervm -Djava.security.manager=allow
> CachePermissionsTest true rwd.policy
Looks to me like we should delete these 3 policy files.
Also this test isn't about Permissions anymore. So should be renamed to
CacheUsedTest
Then we can get rid of the misspelt directory. in a follow up.Probably do this
test/jdk/javax/imageio/CachePremissionsTest/CachePermissionsTest.java line 76:
> 74: System.out.println("java.io.tmpdir is " +
> System.getProperty("java.io.tmpdir"));
> 75:
> 76: if (args.length > 1) {
The isFileCacheExpected logic does not make sense. The test sets set to use the
cache but then reads whether to expect it based o
Re: RFR: 8338411: Implement JEP 486: Permanently Disable the Security Manager [v2]
On Tue, 22 Oct 2024 15:22:08 GMT, Sean Mullan wrote: >> test/jdk/javax/swing/JComboBox/8080972/TestBasicComboBoxEditor.java line 26: >> >>> 24: import javax.swing.SwingUtilities; >>> 25: import javax.swing.plaf.basic.BasicComboBoxEditor; >>> 26: /* >> >> I think we have finally decided that jtreg tag will come after copyright and >> before imports...Applicable for all modified javax_swing tests in this PR... > > This should be addressed in a more general separate task, and not part of > this PR since it does not have anything to do with the changes in this JEP. Agreed. This is not a "clean up / update tests" task. If it is a change on some lines of code that are updated by the SM changes, then that's fair game, but otherwise only the SM behaviour is part of this task. Anything that is not needed to be changed for that purpose, can (and mostly should) be left alone. >> test/jdk/javax/swing/JOptionPane/8081019/bug8081019.java line 31: >> >>> 29: /** >>> 30: * @test >>> 31: * @key headful >> >> javadoc style /** at the beginning > > Not specific to JEP 486, this should be done as part of a different issue. agreed. No need to touch. >> test/jdk/javax/swing/UIDefaults/6622002/bug6622002.java line 29: >> >>> 27: * @test >>> 28: * @bug 6622002 >>> 29: * @author Alexander Potochkin >> >> remove @author tag > > Not specific to JEP 486, this should be done as part of a different issue. agreed - PR Review Comment: https://git.openjdk.org/jdk/pull/21498#discussion_r1811067982 PR Review Comment: https://git.openjdk.org/jdk/pull/21498#discussion_r1811068956 PR Review Comment: https://git.openjdk.org/jdk/pull/21498#discussion_r1811070418
Re: RFR: 8339783: Implement JEP 479: Remove the Windows 32-bit x86 Port [v17]
On Fri, 1 Nov 2024 16:04:55 GMT, Magnus Ihse Bursie wrote: >> This is the implementation of [JEP 479: _Remove the Windows 32-bit x86 >> Port_](https://openjdk.org/jeps/479). >> >> This is the summary of JEP 479: >>> Remove the source code and build support for the Windows 32-bit x86 port. >>> This port was [deprecated for removal in JDK >>> 21](https://openjdk.org/jeps/449) with the express intent to remove it in a >>> future release. > > Magnus Ihse Bursie has updated the pull request incrementally with two > additional commits since the last revision: > > - Remove superfluous check for 64-bit on Windows in > MacroAssembler::call_clobbered_xmm_registers > - Remove windows-32-bit code in CompilerConfig::ergo_initialize make/modules/jdk.accessibility/Lib.gmk line 57: > 55: TARGETS += $(BUILD_LIBJAVAACCESSBRIDGE) > 56: > 57: > ## Most of the desktop related changes are related to Assistive Technologies I don't think we currently provide a 32-bit windowsaccessbridge.dll in the 64 bit JDK, but I'd like to be sure I am not forgetting something. The point being windowsaccessbridge.dll is not loaded by the JDK, but by an AT, so traditionally we provided both 32 and 64 bit versions because we don't control that AT. So I would like Alex Zuev to review these changes. For whatever reason his git hub handle doesn't seem to be found. I think it is something like @azuev-java - PR Review Comment: https://git.openjdk.org/jdk/pull/21744#discussion_r1826177047
Re: RFR: 8338411: Implement JEP 486: Permanently Disable the Security Manager [v2]
On Wed, 23 Oct 2024 05:11:19 GMT, Prasanta Sadhukhan wrote: >> Sean Mullan has updated the pull request with a new target base due to a >> merge or a rebase. The pull request now contains 97 commits: >> >> - Merge remote-tracking branch 'jdk-sandbox/jep486' into JDK-8338411 >> - Change apiNote to deprecated annotation on checkAccess methods. Change >> method dedescription to "Does nothing". >> - Sanitize the class descriptions of DelegationPermission and >> ServicePermission >>by removing text that refers to granting permissions, but avoid changes >> that >>affect the API specification, such as the description and format of input >>parameters. >> - Restored methods in RMIConnection to throw SecurityExceptions again but >>with adjusted text that avoids the word "permission". >> - Add text to class description of MBeanServer stating that implementations >>may throw SecurityException if authorization doesn't allow access to >> resource. >> - Restore text about needing permissions from the desktop environment in the >>getPixelColor and createScreenCapture methods. >> - Add api note to getClassContext to use StackWalker instead and >>add DROP_METHOD_INFO option to StackWalker. >> - Change checkAccess() methods to be no-ops, rather than throwing >>SecurityException. >> - Merge >> - Merge >> - ... and 87 more: https://git.openjdk.org/jdk/compare/f50bd0d9...f89d9d09 > >> @prsadhuk Addressed review comments in the following jep486 branch commit: >> [openjdk/jdk-sandbox@d9ee496](https://github.com/openjdk/jdk-sandbox/commit/d9ee496f7349cb8beaf1e696fd430f8064baee8e) >> >> 1. test/jdk/javax/swing/JEditorPane/8080972/TestJEditor.java - Updated, >> removed redundant try-catch block >> >> 2. test/jdk/javax/swing/JPopupMenu/6691503/bug6691503.java - Repurposed >> and added back >> >> 3. test/jdk/javax/swing/JPopupMenu/6694823/bug6694823.java - Updated, >> added finally block >> >> 4. test/jdk/javax/swing/UIDefaults/6795356/TableTest.java - Added back >> >> >> Left out comments that fall into out-of-scope/clean-up for jep486. > > looks ok but awt import should have been before swing as decided, although > this again is not part of SM exercise but since you modified the tests I > thought I will say it aloud.. > @prsadhuk > > > looks ok but awt import should have been before swing as decided, although > > this again is not part of SM exercise but since you modified the tests I > > thought I will say it aloud > > Thanks! I didn't notice it happened again with the new changes. This was > probably due to IDE settings. Since this was a recent change I have updated > in the latest commit. I also didn't realise you had "changed" it. I'm finding it very painful to navigate to the related files in this huge PR. - PR Comment: https://git.openjdk.org/jdk/pull/21498#issuecomment-2433046255
Re: RFR: 8338411: Implement JEP 486: Permanently Disable the Security Manager [v4]
On Mon, 28 Oct 2024 12:29:07 GMT, Sean Mullan wrote: >> This is the implementation of JEP 486: Permanently Disable the Security >> Manager. See [JEP 486](https://openjdk.org/jeps/486) for more details. The >> [CSR](https://bugs.openjdk.org/browse/JDK-8338412) describes in detail the >> main changes in the JEP and also includes an apidiff of the specification >> changes. >> >> NOTE: the majority (~95%) of the changes in this PR are test updates >> (removal/modifications) and API specification changes, the latter mostly to >> remove `@throws SecurityException`. The remaining changes are primarily the >> removal of the `SecurityManager`, `Policy`, `AccessController` and other >> Security Manager API implementations. There is very little new code. >> >> The code changes can be broken down into roughly the following categories: >> >> 1. Degrading the behavior of Security Manager APIs to either throw >> Exceptions by default or provide an execution environment that disallows >> access to all resources by default. >> 2. Changing hundreds of methods and constructors to no longer throw a >> `SecurityException` if a Security Manager was enabled. They will operate as >> they did in JDK 23 with no Security Manager enabled. >> 3. Changing the `java` command to exit with a fatal error if a Security >> Manager is enabled. >> 4. Removing the hotspot native code for the privileged stack walk and the >> inherited access control context. The remaining hotspot code and tests >> related to the Security Manager will be removed immediately after >> integration - see [JDK-8341916](https://bugs.openjdk.org/browse/JDK-8341916). >> 5. Removing or modifying hundreds of tests. Many tests that tested Security >> Manager behavior are no longer relevant and thus have been removed or >> modified. >> >> There are a handful of Security Manager related tests that are failing and >> are at the end of the `test/jdk/ProblemList.txt`, >> `test/langtools/ProblemList.txt` and `test/hotspot/jtreg/ProblemList.txt` >> files - these will be removed or separate bugs will be filed before >> integrating this PR. >> >> Inside the JDK, we have retained calls to >> `SecurityManager::getSecurityManager` and `AccessController::doPrivileged` >> for now, as these methods have been degraded to behave the same as they did >> in JDK 23 with no Security Manager enabled. After we integrate this JEP, >> those calls will be removed in each area (client-libs, core-libs, security, >> etc). >> >> I don't expect each reviewer to review all the code changes in this JEP. >> Rather, I advise that you only focus on the changes for the area >> (client-libs, core-libs, net, ... > > Sean Mullan has updated the pull request with a new target base due to a > merge or a rebase. The pull request now contains 175 commits: > > - Merge remote-tracking branch 'jdk-sandbox/jep486' into JDK-8338411 > - Specify that params passed to getPermissions and implies are ignored and >implies always returns false. > - Change deprecated annotations to api notes on getPolicy and setPolicy. > - clientlibs: Updated Problemlist >Deleted javax/swing/JPopupMenu/6694823/bug6694823.java entry since it was > determined that it is not a JDK bug but expected behavior on windows and > linux platform. > - clientlibs: Deleted JPopupMenu tests >The following tests are deleted as they don't hold value without SM >test/jdk/javax/swing/JPopupMenu/6691503/bug6691503.java - It tests that > the popup are >always-on-top for apps which doesn't hold value after SM removal. > >test/jdk/javax/swing/JPopupMenu/6694823/bug6694823.java - Tests whether > popup can overlap taskbar. >Works differently on macOS and windows & linux but this is the expected > behavior. >Details in JDK-8342012. Not a functional issue. > - clientlibs: GetSoundBankSecurityException.java renamed to > EmptySoundBankTest.java > - clientlibs: GetSoundBankSecurityException.java renamed to > EmptySoundBankTest.java >test renamed, test summary updated > - Restore note for implementers in > src/java.prefs/share/classes/java/util/prefs/AbstractPreferences.java > - Change "SecurityManager" to "Security Manager". Add some missing code and > linkplain tags. > - Add api note to class description that permission checking is not > supported and remove text about getting permissions from system policy. In > getPermissions(), change "granted to the given codesource" to "for the > codesource". > - ... and 165 more: https://git.openjdk.org/jdk/compare/1476f6c4...e490f698 The client/desktop related src and test changes have all now been reviewed by at least one client/desktop engineer. - Marked as reviewed by prr (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/21498#pullrequestreview-2399915245
Re: RFR: 8338411: Implement JEP 486: Permanently Disable the Security Manager [v3]
On Fri, 1 Nov 2024 18:06:47 GMT, Alexey Ivanov wrote: > > I'd not looked at this test before but when I do the thing I noticed is > > that createPrivateValue is no longer used. But I don't see a problem with > > keeping the rest of the test. > > @prrace Do I understand correctly that _“`createPrivateValue` is no longer > used”_ means `MultiUIDefaults` is never used with `ProxyLazyValue`? > > The [`MultiUIDefaults` > class](https://github.com/openjdk/jdk/blob/master/src/java.desktop/share/classes/javax/swing/MultiUIDefaults.java) > is used in `UIManager`: > > https://github.com/openjdk/jdk/blob/c82ad845e101bf5d97c0744377d68002907d4a0e/src/java.desktop/share/classes/javax/swing/UIManager.java#L198 I think I was just saying there appeared to be dead code in the test. - PR Review Comment: https://git.openjdk.org/jdk/pull/21498#discussion_r1835053637
Re: RFR: 8338411: Implement JEP 486: Permanently Disable the Security Manager [v9]
On Fri, 8 Nov 2024 13:49:55 GMT, Sean Mullan wrote: >> This is the implementation of JEP 486: Permanently Disable the Security >> Manager. See [JEP 486](https://openjdk.org/jeps/486) for more details. The >> [CSR](https://bugs.openjdk.org/browse/JDK-8338412) describes in detail the >> main changes in the JEP and also includes an apidiff of the specification >> changes. >> >> NOTE: the majority (~95%) of the changes in this PR are test updates >> (removal/modifications) and API specification changes, the latter mostly to >> remove `@throws SecurityException`. The remaining changes are primarily the >> removal of the `SecurityManager`, `Policy`, `AccessController` and other >> Security Manager API implementations. There is very little new code. >> >> The code changes can be broken down into roughly the following categories: >> >> 1. Degrading the behavior of Security Manager APIs to either throw >> Exceptions by default or provide an execution environment that disallows >> access to all resources by default. >> 2. Changing hundreds of methods and constructors to no longer throw a >> `SecurityException` if a Security Manager was enabled. They will operate as >> they did in JDK 23 with no Security Manager enabled. >> 3. Changing the `java` command to exit with a fatal error if a Security >> Manager is enabled. >> 4. Removing the hotspot native code for the privileged stack walk and the >> inherited access control context. The remaining hotspot code and tests >> related to the Security Manager will be removed immediately after >> integration - see [JDK-8341916](https://bugs.openjdk.org/browse/JDK-8341916). >> 5. Removing or modifying hundreds of tests. Many tests that tested Security >> Manager behavior are no longer relevant and thus have been removed or >> modified. >> >> There are a handful of Security Manager related tests that are failing and >> are at the end of the `test/jdk/ProblemList.txt`, >> `test/langtools/ProblemList.txt` and `test/hotspot/jtreg/ProblemList.txt` >> files - these will be removed or separate bugs will be filed before >> integrating this PR. >> >> Inside the JDK, we have retained calls to >> `SecurityManager::getSecurityManager` and `AccessController::doPrivileged` >> for now, as these methods have been degraded to behave the same as they did >> in JDK 23 with no Security Manager enabled. After we integrate this JEP, >> those calls will be removed in each area (client-libs, core-libs, security, >> etc). >> >> I don't expect each reviewer to review all the code changes in this JEP. >> Rather, I advise that you only focus on the changes for the area >> (client-libs, core-libs, net, ... > > Sean Mullan has updated the pull request with a new target base due to a > merge or a rebase. The pull request now contains 224 commits: > > - Merge remote-tracking branch 'jdk-sandbox/jep486' into JDK-8338411 > - Merge branch 'master' into jep486 > - Move remaining JEP 486 failing tests into correct groups. > - Move JEP 486 failing tests into hotspot_runtime group. > - test/jdk/java/rmi/server/RMIClassLoader/spi/DefaultProperty.java failing > - Merge branch 'master' into jep486 > - Merge branch 'master' into jep486 > - Merge remote-tracking branch 'jdk-sandbox/jep486' into JDK-8338411 > - Merge branch 'master' into jep486 > - Merge branch 'master' into jep486 > - ... and 214 more: https://git.openjdk.org/jdk/compare/d0077eec...6ad9192e Marked as reviewed by prr (Reviewer). - PR Review: https://git.openjdk.org/jdk/pull/21498#pullrequestreview-2424915104
Re: RFR: 8338411: Implement JEP 486: Permanently Disable the Security Manager [v2]
On Fri, 18 Oct 2024 19:03:30 GMT, Sean Mullan wrote: >> This is the implementation of JEP 486: Permanently Disable the Security >> Manager. See [JEP 486](https://openjdk.org/jeps/486) for more details. The >> [CSR](https://bugs.openjdk.org/browse/JDK-8338412) describes in detail the >> main changes in the JEP and also includes an apidiff of the specification >> changes. >> >> NOTE: the majority (~95%) of the changes in this PR are test updates >> (removal/modifications) and API specification changes, the latter mostly to >> remove `@throws SecurityException`. The remaining changes are primarily the >> removal of the `SecurityManager`, `Policy`, `AccessController` and other >> Security Manager API implementations. There is very little new code. >> >> The code changes can be broken down into roughly the following categories: >> >> 1. Degrading the behavior of Security Manager APIs to either throw >> Exceptions by default or provide an execution environment that disallows >> access to all resources by default. >> 2. Changing hundreds of methods and constructors to no longer throw a >> `SecurityException` if a Security Manager was enabled. They will operate as >> they did in JDK 23 with no Security Manager enabled. >> 3. Changing the `java` command to exit with a fatal error if a Security >> Manager is enabled. >> 4. Removing the hotspot native code for the privileged stack walk and the >> inherited access control context. The remaining hotspot code and tests >> related to the Security Manager will be removed immediately after >> integration - see [JDK-8341916](https://bugs.openjdk.org/browse/JDK-8341916). >> 5. Removing or modifying hundreds of tests. Many tests that tested Security >> Manager behavior are no longer relevant and thus have been removed or >> modified. >> >> There are a handful of Security Manager related tests that are failing and >> are at the end of the `test/jdk/ProblemList.txt`, >> `test/langtools/ProblemList.txt` and `test/hotspot/jtreg/ProblemList.txt` >> files - these will be removed or separate bugs will be filed before >> integrating this PR. >> >> Inside the JDK, we have retained calls to >> `SecurityManager::getSecurityManager` and `AccessController::doPrivileged` >> for now, as these methods have been degraded to behave the same as they did >> in JDK 23 with no Security Manager enabled. After we integrate this JEP, >> those calls will be removed in each area (client-libs, core-libs, security, >> etc). >> >> I don't expect each reviewer to review all the code changes in this JEP. >> Rather, I advise that you only focus on the changes for the area >> (client-libs, core-libs, net, ... > > Sean Mullan has updated the pull request with a new target base due to a > merge or a rebase. The pull request now contains 97 commits: > > - Merge remote-tracking branch 'jdk-sandbox/jep486' into JDK-8338411 > - Change apiNote to deprecated annotation on checkAccess methods. Change > method dedescription to "Does nothing". > - Sanitize the class descriptions of DelegationPermission and > ServicePermission >by removing text that refers to granting permissions, but avoid changes > that >affect the API specification, such as the description and format of input >parameters. > - Restored methods in RMIConnection to throw SecurityExceptions again but >with adjusted text that avoids the word "permission". > - Add text to class description of MBeanServer stating that implementations >may throw SecurityException if authorization doesn't allow access to > resource. > - Restore text about needing permissions from the desktop environment in the >getPixelColor and createScreenCapture methods. > - Add api note to getClassContext to use StackWalker instead and >add DROP_METHOD_INFO option to StackWalker. > - Change checkAccess() methods to be no-ops, rather than throwing >SecurityException. > - Merge > - Merge > - ... and 87 more: https://git.openjdk.org/jdk/compare/f50bd0d9...f89d9d09 I've reviewed all jdk/java/awt and jdk/javax/imageio tests. I've either commented or proactively fixed in the jep sandbox branch. Or both. Next sync from there should resolve those. - PR Comment: https://git.openjdk.org/jdk/pull/21498#issuecomment-2433048411
Re: RFR: 8338411: Implement JEP 486: Permanently Disable the Security Manager [v3]
On Fri, 25 Oct 2024 21:23:26 GMT, Harshitha Onkar wrote: >> The tests with “Audit Core Reflection” in their summary fall into this >> category, we may consider removing them. > > @prrace Can you please advice on “Audit Core Reflection” category of tests. > I'm not 100% sure if these tests need to be deleted or retained (May be some > of them are required for code coverage purpose and/or testing code paths that > are not covered by existing tests). I'd not looked at this test before but when I do the thing I noticed is that createPrivateValue is no longer used. But I don't see a problem with keeping the rest of the test. - PR Review Comment: https://git.openjdk.org/jdk/pull/21498#discussion_r1817433603
Re: RFR: 8338411: Implement JEP 486: Permanently Disable the Security Manager [v3]
On Fri, 25 Oct 2024 21:06:28 GMT, Harshitha Onkar wrote:
>> test/jdk/javax/imageio/spi/AppletContextTest/IIOPluginTest.java line 42:
>>
>>> 40: } catch (ServiceConfigurationError sce) {
>>> 41: System.out.println("Expected ServiceConfigurationError \n"
>>> + sce);
>>> 42: System.out.println("Test Passed");
>>
>> Previously, `ServiceConfigurationError` wasn't caught and, as far as I
>> understand, wasn't expected; if it were thrown, the test would fail. Why is
>> the logic different now?
>
> When SM is removed, test fails with java.util.ServiceConfigurationError:
> javax.imageio.spi.ImageReaderSpi:Provider BadReaderPluginSpi not found. This
> is the expected behavior without SM.
>
> When SM is present this error is swallowed because otherwise a erroneous
> plugin could cause a VM-wide problem. Now that SM is removed, the test
> (IIOPluginTest.java)
> has been updated to expect ServiceConfigurationError.
>
> AFAIK, there isn't another test which checks this code path hence it has been
> repurposed and retained.
Yes, perhaps there's some more general test for this in java.base, but there's
nothing wrong with verifying it here, even though it is sort of backward from
what the test previously verified.
Probably the test could always have tested both cases.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/21498#discussion_r1817428928
Re: RFR: 8338411: Implement JEP 486: Permanently Disable the Security Manager
On Mon, 14 Oct 2024 13:52:24 GMT, Sean Mullan wrote:
> This is the implementation of JEP 486: Permanently Disable the Security
> Manager. See [JEP 486](https://openjdk.org/jeps/486) for more details. The
> [CSR](https://bugs.openjdk.org/browse/JDK-8338412) describes in detail the
> main changes in the JEP and also includes an apidiff of the specification
> changes.
>
> NOTE: the majority (~95%) of the changes in this PR are test updates
> (removal/modifications) and API specification changes, the latter mostly to
> remove `@throws SecurityException`. The remaining changes are primarily the
> removal of the `SecurityManager`, `Policy`, `AccessController` and other
> Security Manager API implementations. There is very little new code.
>
> The code changes can be broken down into roughly the following categories:
>
> 1. Degrading the behavior of Security Manager APIs to either throw Exceptions
> by default or provide an execution environment that disallows access to all
> resources by default.
> 2. Changing hundreds of methods and constructors to no longer throw a
> `SecurityException` if a Security Manager was enabled. They will operate as
> they did in JDK 23 with no Security Manager enabled.
> 3. Changing the `java` command to exit with a fatal error if a Security
> Manager is enabled.
> 4. Removing the hotspot native code for the privileged stack walk and the
> inherited access control context. The remaining hotspot code and tests
> related to the Security Manager will be removed immediately after integration
> - see [JDK-8341916](https://bugs.openjdk.org/browse/JDK-8341916).
> 5. Removing or modifying hundreds of tests. Many tests that tested Security
> Manager behavior are no longer relevant and thus have been removed or
> modified.
>
> There are a handful of Security Manager related tests that are failing and
> are at the end of the `test/jdk/ProblemList.txt`,
> `test/langtools/ProblemList.txt` and `test/hotspot/jtreg/ProblemList.txt`
> files - these will be removed or separate bugs will be filed before
> integrating this PR.
>
> Inside the JDK, we have retained calls to
> `SecurityManager::getSecurityManager` and `AccessController::doPrivileged`
> for now, as these methods have been degraded to behave the same as they did
> in JDK 23 with no Security Manager enabled. After we integrate this JEP,
> those calls will be removed in each area (client-libs, core-libs, security,
> etc).
>
> I don't expect each reviewer to review all the code changes in this JEP.
> Rather, I advise that you only focus on the changes for the area
> (client-libs, core-libs, net, security, etc) that you are most f...
I have looked at the source code changes in java.desktop
They are mostly OK.
I have noted text that was removed in two places in java.awt.Robot where the
removal should be reverted.
I have also "grepped" the sandbox repo to identify any errors of omission -
pertaining to the SE API specification, not internals - and found none.
I also noted a couple of Permission classes we should deprecate - and filed
bugs on them.
I have not yet examined any of the test updates. That looks like a big job.
src/java.desktop/share/classes/java/awt/AWTPermission.java line 39:
> 37: * @apiNote
> 38: * This permission cannot be used for controlling access to resources
> anymore
> 39: * as the Security Manager is no longer supported.
After this JEP is integrated, I expect to deprecate AWTPermission, probably for
removal
src/java.desktop/share/classes/java/awt/Robot.java line 433:
> 431: * then a {@code SecurityException} may be thrown,
> 432: * or the content of the returned {@code Color} is undefined.
> 433: *
This text should not have been removed. It pertains to the desktop permissions
as well as the Java SecurityManager.
src/java.desktop/share/classes/java/awt/Robot.java line 460:
> 458: * then a {@code SecurityException} may be thrown,
> 459: * or the contents of the returned {@code BufferedImage} are
> undefined.
> 460: *
This text should not have been removed. It pertains to the desktop permissions
as well as the Java SecurityManager.
src/java.desktop/share/classes/javax/sound/sampled/AudioPermission.java line 36:
> 34: * actions list; you either have the named permission or you don't.
> 35: *
> 36: * The target name is the name of the audio permission.
AudioPermission is another class we should deprecate
-
PR Review: https://git.openjdk.org/jdk/pull/21498#pullrequestreview-2370309133
PR Review Comment: https://git.openjdk.org/jdk/pull/21498#discussion_r1801765010
PR Review Comment: https://git.openjdk.org/jdk/pull/21498#discussion_r1802031119
PR Review Comment: https://git.openjdk.org/jdk/pull/21498#discussion_r1802031524
PR Review Comment: https://git.openjdk.org/jdk/pull/21498#discussion_r1802042388
Re: RFR: 8343490: Update copyright year for JDK-8341692
On Tue, 5 Nov 2024 01:41:00 GMT, SendaoYan wrote: > Hi all, > The copyright year of some files which has been changed by > [JDK-8341692](https://bugs.openjdk.org/browse/JDK-8341692) wasn't update > correctly. This PR update the copyright year of > [JDK-8341692](https://bugs.openjdk.org/browse/JDK-8341692). Trivial fix, no > risk. FWIW this whole PR seems like a waste of a bug id. Copyright year is implied anyway. - PR Comment: https://git.openjdk.org/jdk/pull/21891#issuecomment-2475394959
Re: RFR: 8345799: Update copyright year to 2024 for core-libs in files where it was missed [v3]
On Tue, 10 Dec 2024 09:31:21 GMT, Prasanta Sadhukhan wrote: > not sure why client label is added, no java.desktop/accessibility files are > in there I was puzzling over that too. - PR Comment: https://git.openjdk.org/jdk/pull/22640#issuecomment-2532895836
Re: RFR: 8347123: Add missing @serial tags to other modules
On Wed, 22 Jan 2025 09:23:10 GMT, Hannes Wallnöfer wrote:
>> src/java.datatransfer/share/classes/java/awt/datatransfer/DataFlavor.java
>> line 1288:
>>
>>> 1286:
>>> 1287: /**
>>> 1288: * Serializes this {@code DataFlavor}.
>>
>> This most definitely changes the serialisation spec. So a CSR is needed.
>> Also shouldn't readExternal be updated to correspond ?
>
> Only the `writeExternal` method is required to have a `@serialData` tag in
> order to keep javadoc running with `-serialwarn` option from complaining. I
> guess the thinking is that the `readExternal` logic can be derived from that.
>
> @prrace do you have any suggestions for the spec change, or are you ok with
> the proposed wording?
There's not a great number of "good" examples of this in the JDK, so probably
OK except it
seems like most cases will do it like a normal javadoc method so you'd want an
@param tag too.
note : this is a desktop class, so I'm looking at this one but someone else
will need to look at all the others
-
PR Review Comment: https://git.openjdk.org/jdk/pull/22980#discussion_r1925928563
Re: RFR: 8347123: Add missing @serial tags to other modules [v2]
On Fri, 24 Jan 2025 10:58:24 GMT, Hannes Wallnöfer wrote: >> Please review a doc-only change to mostly add missing `@serial` javadoc >> tags. This is a sub-task of [JDK-8286931] to allow us to re-enable the >> javadoc `-serialwarn` option in the JDK doc build, which has been disabled >> since JDK 19. >> >> [JDK-8286931]: https://bugs.openjdk.org/browse/JDK-8286931 >> >> For private and package-private serialized fields that already have a doc >> comment, the main description is converted to a block tag by prepending >> `@serial` since these fields do not require a main description. For >> protected and public serialized fields that require a main description, an >> empty `@serial` block tag is appended to the doc comment instead. The effect >> is the same, as the main description is used in `serial-form.html` if the >> `@serial` tag is missing or empty. For those fields that do not have a doc >> comment, a doc comment with an empty `@serial` tag is added. >> >> Apart from missing `@serial` tags, this PR also adds a `@serialData` tag to >> `java.awt.datatransfer.DataFlavor.writeExternal(ObjectOutput)` that the >> javadoc `-serialwarn` option complains about. This is the only change in >> this PR that adds documentation text and causes a change in the generated >> documentation. > > Hannes Wallnöfer has updated the pull request incrementally with one > additional commit since the last revision: > > Update @serialData text to CSR-approved version Approving the datatransfer portion .. at least ... - Marked as reviewed by prr (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/22980#pullrequestreview-2579320786 PR Comment: https://git.openjdk.org/jdk/pull/22980#issuecomment-2619967593
Re: RFR: 8347123: Add missing @serial tags to other modules
On Wed, 8 Jan 2025 20:13:50 GMT, Hannes Wallnöfer wrote:
> Please review a doc-only change to mostly add missing `@serial` javadoc tags.
> This is a sub-task of [JDK-8286931] to allow us to re-enable the javadoc
> `-serialwarn` option in the JDK doc build, which has been disabled since JDK
> 19.
>
> [JDK-8286931]: https://bugs.openjdk.org/browse/JDK-8286931
>
> For private and package-private serialized fields that already have a doc
> comment, the main description is converted to a block tag by prepending
> `@serial` since these fields do not require a main description. For protected
> and public serialized fields that require a main description, an empty
> `@serial` block tag is appended to the doc comment instead. The effect is the
> same, as the main description is used in `serial-form.html` if the `@serial`
> tag is missing or empty. For those fields that do not have a doc comment, a
> doc comment with an empty `@serial` tag is added.
>
> Apart from missing `@serial` tags, this PR also adds a `@serialData` tag to
> `java.awt.datatransfer.DataFlavor.writeExternal(ObjectOutput)` that the
> javadoc `-serialwarn` option complains about. This is the only change in this
> PR that adds documentation text and causes a change in the generated
> documentation.
src/java.datatransfer/share/classes/java/awt/datatransfer/DataFlavor.java line
1288:
> 1286:
> 1287: /**
> 1288: * Serializes this {@code DataFlavor}.
This most definitely changes the serialisation spec. So a CSR is needed.
Also shouldn't readExternal be updated to correspond ?
-
PR Review Comment: https://git.openjdk.org/jdk/pull/22980#discussion_r1924275958
Re: RFR: 8346829: Problem list com/sun/jdi/ReattachStressTest.java & ProcessAttachTest.java on Linux
On Tue, 24 Dec 2024 22:55:12 GMT, Phil Race wrote: > I'd like to problem list two tests which have started failing very frequently bit of a guess ... - PR Comment: https://git.openjdk.org/jdk/pull/22877#issuecomment-2561490332
RFR: 8346829: Problem list com/sun/jdi/ReattachStressTest.java & ProcessAttachTest.java on Linux
I'd like to problem list two tests which have started failing very frequently - Commit messages: - 8346829 Changes: https://git.openjdk.org/jdk/pull/22877/files Webrev: https://webrevs.openjdk.org/?repo=jdk&pr=22877&range=00 Issue: https://bugs.openjdk.org/browse/JDK-8346829 Stats: 2 lines in 1 file changed: 2 ins; 0 del; 0 mod Patch: https://git.openjdk.org/jdk/pull/22877.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/22877/head:pull/22877 PR: https://git.openjdk.org/jdk/pull/22877
Integrated: 8346829: Problem list com/sun/jdi/ReattachStressTest.java & ProcessAttachTest.java on Linux
On Tue, 24 Dec 2024 22:55:12 GMT, Phil Race wrote: > I'd like to problem list two tests which have started failing very frequently This pull request has now been integrated. Changeset: 4fc445d1 Author: Phil Race URL: https://git.openjdk.org/jdk/commit/4fc445d12b4dabd5ce3a6deb23ca6e4fea323620 Stats: 2 lines in 1 file changed: 2 ins; 0 del; 0 mod 8346829: Problem list com/sun/jdi/ReattachStressTest.java & ProcessAttachTest.java on Linux Reviewed-by: lmesnik - PR: https://git.openjdk.org/jdk/pull/22877
Re: RFR: 8346773: Fix unmatched brackets in source files
On Mon, 23 Dec 2024 19:42:34 GMT, Kim Barrett wrote: > > That makes it harder to review and to manage the review, because it is > > large and affects a wide range of areas, so may need many reviewers. > > And many of the appropriate reviewers might be unavailable for a while, since > it's right before Christmas. I think at the very least this needs multiple reviewers. Even 2 isn't enough considering it is so cross-area. It went to 7 different mailing lists, so that should tell you something ! - PR Comment: https://git.openjdk.org/jdk/pull/22861#issuecomment-2560263514
Re: RFR: 8346773: Fix unmatched brackets in source files
On Mon, 23 Dec 2024 19:30:56 GMT, Kim Barrett wrote: >> This patch fixes unmatched brackets in some source files. > > test/jdk/sanity/client/lib/SwingSet3/src/com/sun/swingset3/demos/table/resources/oscars.xml > line 2: > >> 1: >> 2: http://www.fatdog.com/oscar-results"; > > Does this change affect the behavior of the associated test(s)? I was also wondering how this was tested. > test/jdk/sanity/client/lib/SwingSet3/src/com/sun/swingset3/demos/table/resources/oscars.xml > line 2: > >> 1: >> 2: http://www.fatdog.com/oscar-results"; > > I can't tell whether anything was changed in this file other than the > modification of all the end of > line indicators. I've no idea whether changing those is appropriate or not, > but this file came from > a 3rd party, so might not be appropriate to change here. +1 to all of that. This change is un-reviewable. - PR Review Comment: https://git.openjdk.org/jdk/pull/22861#discussion_r1896100629 PR Review Comment: https://git.openjdk.org/jdk/pull/22861#discussion_r1896100936
Re: RFR: 8346773: Fix unmatched brackets in source files
On Mon, 23 Dec 2024 20:29:09 GMT, Phil Race wrote: >> test/jdk/sanity/client/lib/SwingSet3/src/com/sun/swingset3/demos/table/resources/oscars.xml >> line 2: >> >>> 1: >>> 2: http://www.fatdog.com/oscar-results"; >> >> Does this change affect the behavior of the associated test(s)? > > I was also wondering how this was tested. FYI, the way to test this is .. cd test/jdk jtreg sanity/client/SwingSet/src/TableDemoTest.java I tried converting all ^M to \n in this file another repo and comparing with your patch but every line was different so I just can't review this. - PR Review Comment: https://git.openjdk.org/jdk/pull/22861#discussion_r1896162028
Re: RFR: 8356171: Increase timeout for testcases as preparation for change of default timeout factor
On Fri, 9 May 2025 08:58:15 GMT, Leo Korinth wrote: > > test/jdk/java/awt/font/NumericShaper/MTTest.java > > ``` > > * * @run main/timeout=300/othervm MTTest > > > > > > * * @run main/timeout=1200/othervm MTTest > > ``` > > > > > > > > > > > > > > > > > > > > > > > > I'm puzzling over why you saw this test fail with timeout = 300 .. or > > perhaps you saw it fail with 0.7 ? Which would amount to 210 seconds .. > > that might just be enough to cause it to fail because if you look at the > > whole test you'll see it wants the core loops of the test to run for 180 > > seconds. > > https://openjdk.github.io/cr/?repo=jdk&pr=25122&range=00#new-144-test/jdk/java/awt/font/NumericShaper/MTTest.java > > So 300 was fine, and 1200 isn't needed. > > I started with a timeout factor less than `0.7` but I got hindered by > CODETOOLS-7903937. That is probably the reason. Maybe I should change the > timeout to 400? I think it is reasonable to handle a timeout factor somewhat > less than 1 to weed out tight test cases. But maybe 300 is good enough? I think 300 is correct for this test. Setting the timeout factor to < 1 is an interesting experiment but I don't think tests that timeout in such a case are automatic candidates to have an increased time out and this one shows why. - PR Comment: https://git.openjdk.org/jdk/pull/25122#issuecomment-2867676176
Re: RFR: 8356644: Update encoding declaration to UTF-8
On Fri, 9 May 2025 14:14:57 GMT, Magnus Ihse Bursie wrote: > A handful of html and xml files in the JDK source tree claims to have > encodings like `ISO-8859-1`, when they are in fact pure US-ASCII files. > > While perhaps technically correct, this is misleading, and goes contrary to > the efforts of turning the source code into UTF-8 proper. > > I chose between marking them as "ASCII" and "UTF-8", but chose the latter, > since otherwise if they ever were to be updated with a non-ASCII character, > the value would have been unspecified, and after JDK-8301971, all files in > the JDK repository will be interpreted as UTF-8. Marked as reviewed by prr (Reviewer). - PR Review: https://git.openjdk.org/jdk/pull/25148#pullrequestreview-2829627877
Re: RFR: 8356171: Increase timeout for testcases as preparation for change of default timeout factor
On Thu, 8 May 2025 14:51:24 GMT, Leo Korinth wrote: > This change tries to add timeout to individual testcases so that I am able to > run them with a timeout factor of 1 in the future (JDK-8260555). > > The first commit changes the timeout factor to 0.7, so that I can run tests > and test the change (it will finally be changed to 1.0 in JDK-8260555). The > next commit excludes some junit/testng tests where I can currently not change > the timeout factor (CODETOOLS-7903961). Both these commits will be reverted > before integrating the change. I will also apply copyright updates after the > review. > > In addition to changing the timeout factor, I am also using a library call to > parse the timeout factor from the java properties (I can not use the library > function everywhere as jtreg does not allow me to add @library notations to > non testcase files). > > My approach has been to run all test, and afterwards updating those that > fails due to a timeout factor. The amount of updated testcases is huge, and > my strategy has been to quadruple the timeout if I could not directly see > that less was needed (thus the timeout will be the same after JDK-8260555 is > implemented). In a few places I have added a bit more timeout so that it will > work with the 0.7 timeout factor. > > These fixes have been created when I have plown through testcases: > JDK-8352719: Add an equals sign to the modules statement > JDK-8352709: Remove bad timing annotations from WhileOpTest.java > JDK-8352074: Test MemoryLeak.java seems not to test what it is supposed to > test > CODETOOLS-7903937: JTREG uses timeout factor on socket timeout but not on > KEEPALIVE > CODETOOLS-7903961: Make default timeout configurable > > Sometime in the future I will also fix: > 8260555: Change the default TIMEOUT_FACTOR from 4 to 1 > > for which I am awaiting: > CODETOOLS-7903961 that is fixed in jtreg 7.6, but we are still running 7.5.1+1 > > *After the review I will revert the two first commits, and update the > copyrights* test/jdk/java/awt/font/NumericShaper/MTTest.java - * @run main/timeout=300/othervm MTTest + * @run main/timeout=1200/othervm MTTest I'm puzzling over why you saw this test fail with timeout = 300 .. or perhaps you saw it fail with 0.7 ? Which would amount to 210 seconds .. that might just be enough to cause it to fail because if you look at the whole test you'll see it wants the core loops of the test to run for 180 seconds. https://openjdk.github.io/cr/?repo=jdk&pr=25122&range=00#new-144-test/jdk/java/awt/font/NumericShaper/MTTest.java So 300 was fine, and 1200 isn't needed. - PR Comment: https://git.openjdk.org/jdk/pull/25122#issuecomment-2864133534
Re: RFR: 8356978: Convert unicode sequences in Java source code to UTF-8
On Wed, 14 May 2025 14:29:23 GMT, Magnus Ihse Bursie wrote: > After we converted the source base to be fully UTF-8, we do not need to use > unicode sequences (like \u0123) in string literals. Sometimes, that might > still make sense, as for control characters, non-breaking space, etc. But for > strings that is supposed to be a coherent text in a language that needs > non-ASCII parts of Unicode, this is not so. Instead, having the sequences > makes the text just harder to read and edit. We have already removed several > such sequences before, but some remains. Just speaking for the one client demo. Someone else will need to review the other 98% of this PR - Marked as reviewed by prr (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/25229#pullrequestreview-2862613813
Re: RFR: 8359761: JDK 25 RDP1 L10n resource files update [v2]
On Wed, 18 Jun 2025 14:45:32 GMT, Alexey Ivanov wrote: >> Alisen Chung has updated the pull request incrementally with one additional >> commit since the last revision: >> >> fix unicode escapes > > src/demo/share/jfc/SwingSet2/resources/swingset_de.properties line 460: > >> 458: SliderDemo.a_plain_slider=Ein einfacher Schieberegler >> 459: SliderDemo.majorticks=Grobteilungen >> 460: SliderDemo.majorticksdescription=Ein Schieberegler mit Hauptteilstrichen > > Should the translation of `SliderDemo.majorticks` also be updated? I agree. I'd expect consistency here. - PR Review Comment: https://git.openjdk.org/jdk/pull/25839#discussion_r2155451453
