[rfc][icedtea-web] jnlp_href extensions inside HTML applets - PR974

Adam Domurad adomurad at redhat.com
Wed Aug 14 06:28:14 PDT 2013


On 08/01/2013 03:41 PM, Andrew Azores wrote:
> On 07/30/2013 01:46 AM, Jiri Vanek wrote:

[.. snipped ..]

>>> fix.patch
>>>
>>>
>>> diff --git a/netx/net/sourceforge/jnlp/PluginBridge.java
>>> b/netx/net/sourceforge/jnlp/PluginBridge.java
>>> --- a/netx/net/sourceforge/jnlp/PluginBridge.java
>>> +++ b/netx/net/sourceforge/jnlp/PluginBridge.java
>>> @@ -26,19 +26,19 @@ import java.io.ByteArrayInputStream;
>>>   import java.io.File;
>>>   import java.io.IOException;
>>>   import java.io.InputStream;
>>> +import java.net.MalformedURLException;
>>>   import java.net.URL;
>>> -import java.net.MalformedURLException;
>>> +import java.util.ArrayList;
>>> +import java.util.Arrays;
>>>   import java.util.HashSet;
>>> +import java.util.List;
>>>   import java.util.Locale;
>>> -import java.util.List;
>>> -import java.util.ArrayList;
>>>   import java.util.Map;
>>>   import java.util.Set;
>>>
>>> +import net.sourceforge.jnlp.runtime.JNLPRuntime;
>>>   import sun.misc.BASE64Decoder;
>>>
>>> -import net.sourceforge.jnlp.runtime.JNLPRuntime;
>>> -
>>>   /**
>>>    * Allows reuse of code that expects a JNLPFile object,
>>>    * while overriding behaviour specific to applets.
>>> @@ -47,6 +47,7 @@ public class PluginBridge extends JNLPFi
>>>
>>>       private PluginParameters params;
>>>       private Set<String>  jars = new HashSet<String>();
>>> +    private List<ExtensionDesc>  extensionJars = new
>>> ArrayList<ExtensionDesc>();
>>>       //Folders can be added to the code-base through the archive tag
>>>       private List<String>  codeBaseFolders = new ArrayList<String>();
>>>       private String[] cacheJars = new String[0];
>>> @@ -90,6 +91,7 @@ public class PluginBridge extends JNLPFi
>>>           this.codeBase = codebase;
>>>           this.sourceLocation = documentBase;
>>>           this.params = params;
>>> +        this.parserSettings = new ParserSettings();
>>
>> I believe this is wrong. Parser settings are depending on commandline
>> (plugin) parameters.
>> So you need to crete correct parser. If you check the Main method of
>> javaws, you will se it. Maybe extraction of the code from Boot to some
>> factory method in ParserSettings is most correct, here, but the
>> refactoring can e done as separate changeset (as you wish, but you
>> will need this here anyway)
>
> Okay, this is taken into account now in the new patch set.
>
>>
>> If you have just copy and paste the " this.parserSettings = new
>> ParserSettings();" then probably also source was wrong. May you point
>> it out?
>>>
>>>           if (params.getJNLPHref() != null) {
>>>               useJNLPHref = true;
>>> @@ -122,6 +124,9 @@ public class PluginBridge extends JNLPFi
>>>                        String fileName =
>>> jarDesc.getLocation().toExternalForm();
>>>                        this.jars.add(fileName);
>>>                    }
>>> +
>>> +                // Store any extensions listed in the JNLP file to
>>> be returned later on, namely in getResources()
>>> +                extensionJars =
>>> Arrays.asList(jnlpFile.getResources().getExtensions());
>>>               } catch (MalformedURLException e) {
>>>                   // Don't fail because we cannot get the jnlp file.
>>> Parameters are optional not required.
>>>                   // it is the site developer who should ensure that
>>> file exist.
>>> @@ -308,6 +313,8 @@ public class PluginBridge extends JNLPFi
>>>                           return result;
>>>                       } catch (MalformedURLException ex) { /* Ignored */
>>>                       }
>>> +                } else if (launchType.equals(ExtensionDesc.class)) {
>>> +                    return (List<T>) extensionJars; // this list is
>>> populated when the PluginBridge is first constructed
>>>                   }
>>>                   return sharedResources.getResources(launchType);
>>>               }
>>>
>>>
>>> reproducer.patch
>>>
>>>
>>> diff --git
>>> a/tests/reproducers/custom/ExtensionJnlpsInApplet/resources/ExtensionJnlpHelper.jnlp
>>> b/tests/reproducers/custom/ExtensionJnlpsInApplet/resources/ExtensionJnlpHelper.jnlp
>>>
>>
>> I do not think this is custom reproducer. See below.
>>
>>> new file mode 100644
>>> --- /dev/null
>>> +++
>>> b/tests/reproducers/custom/ExtensionJnlpsInApplet/resources/ExtensionJnlpHelper.jnlp
>>>
>>> @@ -0,0 +1,53 @@
>>
>> ..snip (=ok:))
>>
>>> diff --git
>>> a/tests/reproducers/custom/ExtensionJnlpsInApplet/srcs/Makefile
>>> b/tests/reproducers/custom/ExtensionJnlpsInApplet/srcs/Makefile
>>> new file mode 100644
>>> --- /dev/null
>>> +++ b/tests/reproducers/custom/ExtensionJnlpsInApplet/srcs/Makefile
>>> @@ -0,0 +1,34 @@
>>> +TESTNAME=ExtensionJnlpsInApplet
>>> +
>>> +SRC_FILES=ExtensionJnlpHelper.java ExtensionJnlpTestApplet.java
>>> +RESOURCE_FILES=ExtensionJnlpTest.html ExtensionJnlpTestApplet.jnlp
>>> ExtensionJnlpHelper.jnlp
>>> +ENTRYPOINT_CLASSES=ExtensionJnlpHelper ExtensionJnlpTestApplet
>>> +
>>> +JAVAC_CLASSPATH=$(TEST_EXTENSIONS_DIR):$(NETX_DIR)/lib/classes.jar
>>> +JAVAC=$(BOOT_DIR)/bin/javac
>>> +JAR=$(BOOT_DIR)/bin/jar
>>> +
>>> +TMPDIR:=$(shell mktemp -d)
>>> +
>>> +prepare-reproducer:
>>> +    echo PREPARING REPRODUCER $(TESTNAME)
>>> +
>>> +    $(JAVAC) -d $(TMPDIR) -classpath $(JAVAC_CLASSPATH) $(SRC_FILES)
>>> +
>>> +    cd ../resources; \
>>> +    cp $(RESOURCE_FILES) $(REPRODUCERS_TESTS_SERVER_DEPLOYDIR); \
>>> +    cd -; \
>>> +    ls; \
>>> +    for CLASS in $(ENTRYPOINT_CLASSES); \
>>> +    do \
>>> +        cd $(TMPDIR); \
>>> +        $(JAR) cfe "$$CLASS.jar" "$$CLASS" "$$CLASS.class"; \
>>> +        cd -;\
>>> +        mv $(TMPDIR)/"$$CLASS.jar"
>>> $(REPRODUCERS_TESTS_SERVER_DEPLOYDIR); \
>>> +    done; \
>>> +
>>> +    echo PREPARED REPRODUCER $(TESTNAME), removing $(TMPDIR)
>>> +    rm -rf $(TMPDIR)
>>> +
>>
>> As far as I can read this just compile classes and jar them. Then copy
>> ajr and resources. This is exactly what engine do for you for free.
>> Unless I misread, please move this reproducer to simple ones.
>
> The simple reproducers get JAR'd up into a single JAR, but this test
> specifically requires the srcs to end up in their own separate JARs. Is
> there some way to make this happen for a simple reproducer?

Not really. You can do this with two 'reproducers' (one without a test), 
but no need to change what you have.

[.. snip ..]

>>
>>
>> Also TBH I'm not fan of CLOSE_ON_BOTH. I woul like  to encourage you
>> to use only CloseOnOk. AThe reason is that some completely unrelated
>> exception can be fired, and it will cause this reproducer to fail.
>>
>> Although we (Me, Omair and Adam) have long ago decided that it is a
>> correct behaviour, the instability of reproducers is proving the
>> opposite.

Agreed here.

[.. snip ..]

Comments:

First off, the tests look great, no changes needed from me.

Fix comments:

> diff --git a/netx/net/sourceforge/jnlp/ParserSettings.java b/netx/net/sourceforge/jnlp/ParserSettings.java
> --- a/netx/net/sourceforge/jnlp/ParserSettings.java
> +++ b/netx/net/sourceforge/jnlp/ParserSettings.java
> @@ -37,6 +37,9 @@ exception statement from your version.
>
>  package net.sourceforge.jnlp;
>
> +import java.util.ArrayList;
> +import java.util.List;
> +
>  /**
>   * Contains settings to be used by the Parser while parsing JNLP files.
>   *
> @@ -44,9 +47,12 @@ package net.sourceforge.jnlp;
>   */
>  public class ParserSettings {
>
> +    private static final String doubleArgs = "-basedir -jnlp -arg -param -property -update";
> +

This seems clearer as an array. As well, consider using the all-caps and 
underscore convention for constants.

>      private final boolean isStrict;
>      private final boolean extensionAllowed;
>      private final boolean malformedXmlAllowed;
> +    private static String[] cmdArgs;

I prefer storing 'global parser settings' here. Setting some global 
parser settings is more obvious in intent than 
ParserSettings.setCommandLineArgs(...).

>
>      /** Create a new ParserSettings with the defautl parser settings */
>      public ParserSettings() {
> @@ -75,4 +81,66 @@ public class ParserSettings {
>          return isStrict;
>      }
>
> -}
> \ No newline at end of file
> +    public static void setCommandLineArgs(String[] args) {
> +        cmdArgs = args;
> +    }
> +
> +    public static ParserSettings getCommandLineParserSettings() {

Consider making this take a string parameter (combined with the 
recommended change above).

> +        if (cmdArgs == null || cmdArgs.length == 0) {
> +            return new ParserSettings();
> +        }
> +
> +        boolean strict = false;
> +        boolean malformedXmlAllowed = true;
> +
> +        if (null != getOption("-strict")) {
> +            strict = true;
> +        }
> +
> +        if (null != getOption("-xml")) {
> +            malformedXmlAllowed = false;
> +        }
> +
> +        return new ParserSettings(strict, true, malformedXmlAllowed);
> +    }
> +
> +    /**
> +     * Return value of the first occurence of the specified
> +     * option, or null if the option is not present.  If the
> +     * option is a flag (0-parameter) and is present then the
> +     * option name is returned.
> +     */
> +    private static String getOption(String option) {
> +        String result[] = getOptions(option);
> +
> +        if (result.length == 0)
> +            return null;
> +        else
> +            return result[0];
> +    }
> +
> +    /**
> +     * Return all the values of the specified option, or an empty
> +     * array if the option is not present.  If the option is a
> +     * flag (0-parameter) and is present then the option name is
> +     * returned once for each occurrence.
> +     */
> +    private static String[] getOptions(String option) {
> +        List<String> result = new ArrayList<String>();
> +
> +        for (int i = 0; i < cmdArgs.length; i++) {
> +            if (option.equals(cmdArgs[i])) {
> +                if (-1 == doubleArgs.indexOf(option))
> +                    result.add(option);
> +                else if (i + 1 < cmdArgs.length)
> +                    result.add(cmdArgs[i + 1]);
> +            }
> +
> +            if (cmdArgs[i].startsWith("-") && -1 != doubleArgs.indexOf(cmdArgs[i]))
> +                i++;
> +        }
> +
> +        return result.toArray(new String[result.size()]);
> +    }
> +
> +}
> diff --git a/netx/net/sourceforge/jnlp/PluginBridge.java b/netx/net/sourceforge/jnlp/PluginBridge.java
> --- a/netx/net/sourceforge/jnlp/PluginBridge.java
> +++ b/netx/net/sourceforge/jnlp/PluginBridge.java
> @@ -26,19 +26,19 @@ import java.io.ByteArrayInputStream;
>  import java.io.File;
>  import java.io.IOException;
>  import java.io.InputStream;
> +import java.net.MalformedURLException;
>  import java.net.URL;
> -import java.net.MalformedURLException;
> +import java.util.ArrayList;
> +import java.util.Arrays;
>  import java.util.HashSet;
> +import java.util.List;
>  import java.util.Locale;
> -import java.util.List;
> -import java.util.ArrayList;
>  import java.util.Map;
>  import java.util.Set;
>
> +import net.sourceforge.jnlp.runtime.JNLPRuntime;
>  import sun.misc.BASE64Decoder;
>
> -import net.sourceforge.jnlp.runtime.JNLPRuntime;
> -
>  /**
>   * Allows reuse of code that expects a JNLPFile object,
>   * while overriding behaviour specific to applets.
> @@ -47,6 +47,7 @@ public class PluginBridge extends JNLPFi
>
>      private PluginParameters params;
>      private Set<String> jars = new HashSet<String>();
> +    private List<ExtensionDesc> extensionJars = new ArrayList<ExtensionDesc>();
>      //Folders can be added to the code-base through the archive tag
>      private List<String> codeBaseFolders = new ArrayList<String>();
>      private String[] cacheJars = new String[0];
> @@ -90,6 +91,7 @@ public class PluginBridge extends JNLPFi
>          this.codeBase = codebase;
>          this.sourceLocation = documentBase;
>          this.params = params;
> +        this.parserSettings = ParserSettings.getCommandLineParserSettings();

I prefer to have some reference to 'global settings' versus 'command 
line settings' - eg some 'getGlobalParserSettings' method. I am not 
really fully convinced this should be anything except 'new 
ParserSettings()' as you had before.

>
>          if (params.getJNLPHref() != null) {
>              useJNLPHref = true;
> @@ -122,6 +124,9 @@ public class PluginBridge extends JNLPFi
>                       String fileName = jarDesc.getLocation().toExternalForm();
>                       this.jars.add(fileName);
>                   }
> +
> +                // Store any extensions listed in the JNLP file to be returned later on, namely in getResources()
> +                extensionJars = Arrays.asList(jnlpFile.getResources().getExtensions());
>              } catch (MalformedURLException e) {
>                  // Don't fail because we cannot get the jnlp file. Parameters are optional not required.
>                  // it is the site developer who should ensure that file exist.
> @@ -308,6 +313,8 @@ public class PluginBridge extends JNLPFi
>                          return result;
>                      } catch (MalformedURLException ex) { /* Ignored */
>                      }
> +                } else if (launchType.equals(ExtensionDesc.class)) {
> +                    return (List<T>) extensionJars; // this list is populated when the PluginBridge is first constructed
>                  }
>                  return sharedResources.getResources(launchType);
>              }
> diff --git a/netx/net/sourceforge/jnlp/runtime/Boot.java b/netx/net/sourceforge/jnlp/runtime/Boot.java
> --- a/netx/net/sourceforge/jnlp/runtime/Boot.java
> +++ b/netx/net/sourceforge/jnlp/runtime/Boot.java
> @@ -214,18 +214,9 @@ public final class Boot implements Privi
>          extra.put("arguments", getOptions("-arg"));
>          extra.put("parameters", getOptions("-param"));
>          extra.put("properties", getOptions("-property"));
> -        boolean strict = false;
> -        boolean malformedXmlAllowed = true;
>
> -        if (null != getOption("-strict")) {
> -            strict = true;
> -        }
> -
> -        if (null != getOption("-xml")) {
> -            malformedXmlAllowed = false;
> -        }
> -
> -        ParserSettings settings = new ParserSettings(strict, true, malformedXmlAllowed);
> +        ParserSettings.setCommandLineArgs(args);
> +        ParserSettings settings = ParserSettings.getCommandLineParserSettings();
>
>          try {
>              Launcher launcher = new Launcher(false);

Looks good, though.

Thanks,
-Adam



More information about the distro-pkg-dev mailing list