Code review: 8010464: Evolve java networking same origin policy
Michael McMahon
michael.x.mcmahon at oracle.com
Mon May 13 06:45:01 PDT 2013
On 13/05/13 14:12, Vitaly Davidovich wrote:
>
> I get what you're saying about before/after, but the difference would
> be that if it's called during then you get an exception purely due to
> missing synchronization; in the before/after case, caller may observe
> "stale" entries but that's fine, as you say.
>
How would that be? The only effect of synchronization is to ensure that
the other call
occurs before or after (so to speak).
> Maybe headers aren't reset in practice, but this code looks suspect to
> someone reading it. :)
>
Right, we shouldn't be depending on caller behavior, but I still don't
see a problem to be fixed.
Michael
> Sent from my phone
>
> On May 13, 2013 9:05 AM, "Michael McMahon"
> <michael.x.mcmahon at oracle.com <mailto:michael.x.mcmahon at oracle.com>>
> wrote:
>
> Hi Vitali,
>
> I was going to switch to use Arrays.asList() as you and Alan
> suggested. So getHeaderNames() would look like:
>
> public List<String> getHeaderNames() {
> return Arrays.asList(keys);
> }
>
> So, it turns out synchronizing nkeys and keys is no longer necessary.
> It's true that reset() could be called during the call. But, it
> could (in theory) be called
> before or after either. In practice that won't happen, since the
> request headers
> aren't ever reset.
>
> Michael
>
>
>
> On 13/05/13 13:36, Vitaly Davidovich wrote:
>>
>> Actually, local may not work since getHeaders uses nkeys as well
>> - can run into AIOBE. Probably best to just synchronize given
>> current implementation.
>>
>> Sent from my phone
>>
>> On May 13, 2013 8:30 AM, "Vitaly Davidovich" <vitalyd at gmail.com
>> <mailto:vitalyd at gmail.com>> wrote:
>>
>> Hi Michael,
>>
>> On the synchronized issue, I think you do need it; if
>> someone, e.g., calls reset() while this method is running,
>> you'll get NPE. Maybe pull the keys array into a local then
>> and iterate over the local instead?
>>
>> Also, why LinkedList instead of ArrayList(or Arrays.asList,
>> as Alan mentioned, although maybe caller is expected to
>> modify the returned list)?
>>
>> Thanks
>>
>> Sent from my phone
>>
>> On May 13, 2013 6:42 AM, "Michael McMahon"
>> <michael.x.mcmahon at oracle.com
>> <mailto:michael.x.mcmahon at oracle.com>> wrote:
>>
>> Thanks for the review. On the javadoc comments, there are
>> a couple
>> of small spec changes that will probably happen after
>> feature freeze anyway.
>> So, that might be the best time to address the other
>> javadoc issues.
>>
>> I agree with your other comments. On the synchronized
>> method in MessageHeader,
>> I don't believe it needs to be synchronized since the
>> method is not relying on
>> consistency between object fields, and the returned
>> object can be
>> modified before, during or after the method is called anyway.
>>
>> Michael
>>
>> On 12/05/13 08:13, Alan Bateman wrote:
>>
>> On 10/05/2013 12:34, Michael McMahon wrote:
>>
>> Hi,
>>
>> This is the webrev for the HttpURLPermission
>> addition.
>> As well as the new permission class, the change
>> includes the use of the permission in
>> java.net.HttpURLConnection.
>>
>> The code basically checks for a HttpURLPermission
>> in plainConnect(),
>> getInputStream() and getOutputStream() for the
>> request and if
>> the caller has permission the request is executed
>> in a doPrivileged()
>> block. When the limited doPrivileged feature is
>> integrated, I will
>> change the doPrivileged() call to limit the
>> privilege elevation to a single
>> SocketPermission (as shown in the code comments).
>>
>> The webrev is at
>> http://cr.openjdk.java.net/~michaelm/8010464/webrev.1/
>> <http://cr.openjdk.java.net/%7Emichaelm/8010464/webrev.1/>
>>
>> A partial review, focusing mostly on the spec as
>> we've been through a few rounds on that part already.
>> Overall I think the javadoc looks quite good. I
>> realize someone suggested using lowercase "url" in
>> the javadoc but as the usage is as an acronym then it
>> might be clearer if it were in uppercase, maybe "URL
>> string" to avoid any confusion with java.net.URL.
>>
>> I assume you'll add a copyright header to
>> HttpURLPermission before pushing this.
>>
>> A minor comment on the javadoc tags is that you
>> probably should use @throws instead of @exception.
>>
>> At a high-level it would be nice if the fields were
>> final but I guess the parsing of actions and being
>> serialized complicates this.
>>
>> setURI - this parses the URI rather than "sets" it so
>> maybe it should be renamed. If you use URI.create
>> then it would avoid needing to catch the
>> URISyntaxException.
>>
>> normalizeMethods/normalizeHeaders- I assume these
>> could use an ArrayList.
>>
>> HttpURLConnection - "if a security manager is
>> installed", should this be "set"?
>>
>> MessageHeader - some of the methods are synchronized,
>> some are not. I can't quite tell if getHeaderNames
>> needs to be synchronized. Also is there any reason
>> why this can't use Arrays.asList?
>>
>> HttpURLConnection.setRequestMethod - "connection
>> being open" -> "connect in progress"?
>>
>> That's all I have for now but I think there is
>> further review work required on HttpURLConnection as
>> some of that is tricky.
>>
>> -Alan.
>>
>>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: http://mail.openjdk.java.net/pipermail/net-dev/attachments/20130513/e75eeb42/attachment.html
More information about the net-dev
mailing list