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

[David Holmes]

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.

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
>>>>
>>