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

[Ioi Lam]

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