[icedtea-web][rfc] Fix for PR588, storing java cookie jar cookies in browser

[Jiri Vanek]

On 07/26/2012 09:44 PM, Adam Domurad wrote

The fix looks more over good, still few issues remains:

First  -  can you please separate introducing  of new method "consume_plugin" from rest of changes? 
It  give me som epuzzling to find the real changes :) You can proceed with this refactoring and push 
without more reviews.
Second - i was not able to verify functionality :((

C - part -

parts = g_strsplit (message, " ", 6); - Shouldn be parts freed somewhere... else? I see just 
g_strfreev in condition and still parts are used later....


How does (set_cookie_info especially with decoded_url behave on urls with strange characters? I 
would recommend definitely to check and also include this to reproducer suite with other testcases 
of this fix.


Java - part  - see inline


...
>       {
> diff --git a/plugin/icedteanp/java/sun/applet/PluginCookieManager.java b/plugin/icedteanp/java/sun/applet/PluginCookieManager.java
> --- a/plugin/icedteanp/java/sun/applet/PluginCookieManager.java
> +++ b/plugin/icedteanp/java/sun/applet/PluginCookieManager.java
> @@ -45,7 +45,16 @@ import java.util.Collections;
>   import java.util.List;
>   import java.util.Map;
>
> +import com.sun.jndi.toolkit.url.UrlUtil;
> +
>   public class PluginCookieManager extends CookieManager {
> +    private PluginStreamHandler streamHandler;
> +
> +    public PluginCookieManager(PluginStreamHandler streamHandler) {
> +        this.streamHandler = streamHandler;
> +    }
> +
> +    @Override
>       public Map<String, List<String>> get(URI uri,
>               Map<String, List<String>> requestHeaders) throws IOException {
>           // pre-condition check
> @@ -84,4 +93,21 @@ public class PluginCookieManager extends
>
>           return false;
>       }
> +
> +    @Override
> +    public void put(URI uri,
> +            Map<String, List<String>> responseHeaders) throws IOException {
> +        super.put(uri, responseHeaders);
> +
> +        for (Map.Entry<String, List<String>> headerEntry : responseHeaders.entrySet()) {
> +            String type = headerEntry.getKey();
> +            if ("Set-Cookie".equalsIgnoreCase(type) || "Set-Cookie2".equalsIgnoreCase(type)) {

Are you sure that bot Set-Cookie types will have same processing?

> +                List<String> cookies = headerEntry.getValue();
> +                for (String cookie : cookies) {
> +                    streamHandler.write("plugin PluginSetCookie reference -1 " + UrlUtil.encode(uri.toString(), "UTF-8") + " " + cookie);

Please ensure in tests that this works fine with "strange" urls (same test as for C side)

Also probably will be worthy to test how rewriting of cookies is working with your implementation 
(another testcases... soryyyyy)
> +                }
> +            }
> +
> +        }
> +    }
>   }
> diff --git a/plugin/icedteanp/java/sun/applet/PluginMain.java b/plugin/icedteanp/java/sun/applet/PluginMain.java
> --- a/plugin/icedteanp/java/sun/applet/PluginMain.java
> +++ b/plugin/icedteanp/java/sun/applet/PluginMain.java
> @@ -114,7 +114,7 @@ public class PluginMain {
>               PluginAppletViewer.setStreamhandler(streamHandler);
>               PluginAppletViewer.setPluginCallRequestFactory(new PluginCallRequestFactory());
>
> -            init();
> +            init(streamHandler);
>
>               // Streams set. Start processing.
>               streamHandler.startProcessing();

I can see that you are sending streamHandler into init, but startProcessing() is executed later. Can 
there be some unlikely  problem, when something will be written to this handler before processing 
starts? Cant there be better solution?

eg return CookieManager from inti and then set(streamHandler) on t?. Also it will lower amount of 
changes in code (but decrees readability probably) . But here I'm just posting my thoughts so so 
take it just as it:)

> @@ -140,7 +140,7 @@ public class PluginMain {
>           return streamHandler;
>       }
>
> -    private static void init() {
> +    private static void init(PluginStreamHandler streamHandler) {
>           Properties avProps = new Properties();
>
>           // ADD OTHER RANDOM PROPERTIES
> @@ -200,7 +200,7 @@ public class PluginMain {
>           // override the proxy selector set by JNLPRuntime
>           ProxySelector.setDefault(new PluginProxySelector());
>
> -        CookieManager ckManager = new PluginCookieManager();
> +        CookieManager ckManager = new PluginCookieManager(streamHandler);
>           CookieHandler.setDefault(ckManager);
>       }
>   }
>


Good improvement at all!

J.