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

[Adam Domurad]

Updated ChangeLog:
2012-08-10  Adam Domurad  <adomurad at redhat.com>

	Fixes PR588, cookies set in the java cookie jar are now stored properly
	* plugin/icedteanp/IcedTeaNPPlugin.cc
	(set_cookie_info): New, uses setvalueforurl
	(consume_plugin_message): Additional message added allowing 
	set_cookie_info to be used from the java side.
	* plugin/icedteanp/java/sun/applet/PluginCookieManager.java: Now
	overrides put method, results in set_cookie_info calls in C++
	* plugin/icedteanp/java/sun/applet/PluginMain.java: Passes
	PluginStreamHandler to PluginCookieManager to allow C++ side
	communication


> 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.
Done
> Second - i was not able to verify functionality :((
Resolved
> 
> 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....
I have tried to make the code clearer in intent here
> 
> 
> 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.
Since decoded_url has been used in many frequently used code paths already (and admittedly mostly due to laziness) I have not included such a reproducer :)
Feel free to disagree with this logic/laziness :)

> 
> 
> 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?
Yes, the default cookie manager treats them identically
> 
> > +                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)
The rewriting of cookies is tested in some respects by the 'clear cookie' aspect of my reproducer. 
If you feel this needs additional testing let me know.

> > +                }
> > +            }
> > +
> > +        }
> > +    }
> >   }
> > 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:)
I have created a setCookieManager function in the new patch. The constructor stays as-is. I dislike the idea of a setter here.
> 
> > @@ -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.



-------------- next part --------------
A non-text attachment was scrubbed...
Name: cookies3.patch
Type: text/x-patch
Size: 5347 bytes
Desc: not available
Url : http://mail.openjdk.java.net/pipermail/distro-pkg-dev/attachments/20120810/9f431bd3/cookies3.patch