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

[Jiangli Zhou]

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