8210765: Remove finalize method in CStrike.java #26397
Conversation
|
👋 Welcome back prr! A progress list of the required criteria for merging this PR into |
|
@prrace 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: 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 206 new commits pushed to the
As there are no conflicts, your changes will automatically be rebased on top of these commits when integrating. If you prefer to avoid this automatic rebasing, please check the documentation for the /integrate command for further details. ➡️ To integrate this PR with the above commit message to the |
Webrevs
|
|
/label remove rfr |
|
@prrace
|
| if (nativeStrikePtr != 0) { | ||
| return nativeStrikePtr; | ||
| } | ||
| return nativeStrikePtr; |
There was a problem hiding this comment.
Any reason this check was removed?
I see that initNativeStrikePtr will call createNativeStrikePtr which does this where there is no null check and it just returns what it gets so shouldn't we check it here?
awtStrike = [AWTStrike awtStrikeForFont:awtFont tx:glyphTx invDevTx:invDevTx style:style aaStyle:aaStyle]; // autoreleased
if (awtStrike)
{
CFRetain(awtStrike); // GC
}
JNI_COCOA_EXIT(env);
return ptr_to_jlong(awtStrike);
There was a problem hiding this comment.
And if it is 0, what would you do ?
Previously 0 meant it hadn't yet been initialized. Now it is always initialized.
You'd need to return it anyway even it was zero.
There was a problem hiding this comment.
Then I guess whoever is using the nativeStrikePtr should account for that it can receive null or 0..
I haven't seen all cases but one case I saw getFontMetrics calls getFontMetrics(getNativeStrikePtr())
and there it does this where it dereference awtStrikePtr...Shouldn't it cause SIGSEGV if it ever becomes 0?
JNIEXPORT jobject JNICALL
Java_sun_font_CStrike_getFontMetrics
(JNIEnv *env, jclass clazz, jlong awtStrikePtr)
{
jobject metrics = NULL;
JNI_COCOA_ENTER(env);
AWT_FONT_CLEANUP_SETUP;
AWTFont *awtfont = ((AWTStrike *)jlong_to_ptr(awtStrikePtr))->fAWTFont;
There was a problem hiding this comment.
Nothing is different than before.
We return the result of calling createNativeStrikePtr() both before and after this fix.
So this is a pre-existing issue and one that I am not sure is very likely.
It would require the native alloc for the Objective C object to fail.
And it probably would require all callers to be ready to handle it.
Dealing with that pre-existing theoretical problem is well outside the scope of this.
I'm not going looking for clean up here. |
|
/integrate |
|
Going to push as commit 0d0d93e.
Your commit was automatically rebased without conflicts. |
openjdk
bot
removed
ready
Remove finalize method from CStrike.java
This one is a bit odd in that there's already a Disposer used - and in fact it involves two classes
CStrikeDisposer.java and its subclass - the nested class CStrike.GlyphInfoCache
CStrike.GlyphInfoCache is tracking all the glyph image references.
CStrikeDisposer has the full set of constructors of its superclass - FontStrikeDisposer including support
for the native context
And if supplied, CStrikeDisposer will call the native method freeNativeScalerContext(long) to free the native context
but that native method does not exist !
And there's no CStrike.GlyphInfoCache constructor which allows it to be specified
So the fix is to add that and call the disposeNativeStrikePtr method instead.
I also rejigged things a little so nativeStrikePtr could be final which is supposed to help with the thread visibility.
Progress
Issue
Reviewers
Reviewing
Using
gitCheckout this PR locally:
$ git fetch https://git.openjdk.org/jdk.git pull/26397/head:pull/26397$ git checkout pull/26397Update a local copy of the PR:
$ git checkout pull/26397$ git pull https://git.openjdk.org/jdk.git pull/26397/headUsing Skara CLI tools
Checkout this PR locally:
$ git pr checkout 26397View PR using the GUI difftool:
$ git pr show -t 26397Using diff file
Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/26397.diff
Using Webrev
Link to Webrev Comment