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

[Calvin Cheung]

Looks good.

thanks,
Calvin

On 1/9/19, 5:50 PM, Jiangli Zhou wrote:
> 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
>>>>>>
>>>>
>