[12] RFR: 8214827: Incorrect call ClassLoaders.toFileURL("jrt:/java.compiler")

[Jiangli Zhou]

Hi Ioi and David,

Thanks for the comments!


On 1/9/19 5:13 PM, David Holmes wrote:
> On 10/01/2019 10:47 am, Ioi Lam wrote:
>>
>> On 1/9/19 4:32 PM, Jiangli Zhou wrote:
>>> Hi Calvin,
>>>
>>> Thanks for the review!
>>>
>>>
>>> On 1/9/19 3:01 PM, Calvin Cheung wrote:
>>>> Hi Jiangli,
>>>>
>>>> 1) systemDictionaryShared.cpp:
>>>>
>>>> Should all those CHECK_(protection_domain) be simply CHECK_NH?
>>>> In get_shared_jar_manifest() of the same file, CHECK_NH is used. 
>>>> But in other functions, CHECK_(something) is being used.
>>>> Not sure which one is the preferred way?
>>>
>>> I'm not sure which is the preferred way either. I'm leaving it as is 
>>> and also checking with Coleen...
>>
>> CHECK_(protection_domain)  depends on the fact that protection_domain 
>> is NULL at the point of the CHECK_. This could lead to maintenance 
>> issues down the road if someone decides to initialize 
>> protection_domain earlier.
>
> I think this is fairly common practice though. We often define the 
> "result" variable at the start of a method with an "error" value and 
> return the result on error.

I agree with David and it seems to be a common practice. However, I 
don't have a strong preference on the usage of the CHECK_ macros. Based 
on the feedback from Coleen, Calvin and Ioi, I updated the webrev to use 
the CHECK_NH in SystemDictionaryShared::get_shared_protection_domain(). 
The rest of the usages in the file are not changed in this changeset to 
minimize risks.

Here is the updated webrev: 
http://cr.openjdk.java.net/~jiangli/8214827/webrev.03/src/hotspot/share/classfile/systemDictionaryShared.cpp.frames.html

Full webrev: http://cr.openjdk.java.net/~jiangli/8214827/webrev.03/

Thanks!
Jiangli
>
> David
> -----
>
>>
>> I think CHECK_NH is better. Easier to read and less dependencies.
>>
>> The rest of the changes and Calvin's suggestions look good to me.
>>
>> Thanks
>>
>> - Ioi
>>
>>
>>
>>>>
>>>> 2) ProtectionDomain.java:
>>>>
>>>> 58     TestCommon.run("-cp", appJar, "ProtDomain").assertNormalExit();
>>>>
>>>>     You can also check the output by doing:
>>>>     TestCommon.run("-cp", appJar, 
>>>> "ProtDomain").assertNormalExit("Protection Domains match");
>>>>     Similarly for line 61.
>>>>
>>>> 65     TestCommon.run("-cp", appJar, 
>>>> "JimageClassProtDomain").assertNormalExit();
>>>>
>>>>      You can check the output "shouldNotContain" by doing:
>>>>      TestCommon.run("-cp", appJar, "JimageClassProtDomain")
>>>>          .assertNormalExit(output -> 
>>>> output.shouldNotContain("Failed: Protection Domains do not match");
>>>
>>> I've updated the webrev and incorporated with your suggestions: 
>>> http://cr.openjdk.java.net/~jiangli/8214827/webrev.02/.
>>>>
>>>> The remaining 3 files look good.
>>>
>>> Thanks!
>>> Jiangli
>>>>
>>>> thanks,
>>>> Calvin
>>>>
>>>> On 1/9/19, 2:15 PM, Jiangli Zhou wrote:
>>>>> Please review the bug fix for 
>>>>> SystemDictionaryShared::get_shared_protection_domain(). When 
>>>>> jrt:/<module> location is given, a null URL (due to the error in 
>>>>> ClassLoaders.toFileURL) is used for the CodeSource, which results 
>>>>> a wrong ProtectionDomain being used for the archived app/platform 
>>>>> class from the runtime image. The granted permission is minimum in 
>>>>> that case.
>>>>>
>>>>> This turns out to be an issue that's hidden for a long time. The 
>>>>> JimageClassProtDomain.java test has been outdated and fails to 
>>>>> detect the issue (I'm the guilt one). Please see the comments in 
>>>>> the bug for more details. Thanks Martin for finding the issue, and 
>>>>> David, Alan and Mandy for discussions.
>>>>>
>>>>> webrev: http://cr.openjdk.java.net/~jiangli/8214827/webrev.01/
>>>>>
>>>>> bug: https://bugs.openjdk.java.net/browse/JDK-8214827
>>>>>
>>>>> Tested with appcds tests locally. Tested tier1-2. Tier3 is still 
>>>>> in progress.
>>>>>
>>>>> Thanks,
>>>>>
>>>>> Jiangli
>>>>>
>>>