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

[Jiri Vanek]

On 06/25/2013 09:47 PM, Andrew Azores wrote:
> Changelog:
>
> * netx/net/sourceforge/jnlp/PluginBridge.java (PluginBridge, getResources): Constructor stores list of JNLP extensions, getResources returns this list
>
> * tests/reproducers/custom/ExtensionJnlpsInApplet/testcases/ExtensionJnlpsInAppletTest.java: tests browser launch of HTML file with embedded JNLP applet referencing extension JNLP
> * tests/reproducers/custom/ExtensionJnlpsInApplet/resources/ExtensionJnlpTest.html: same
> * tests/reproducers/custom/ExtensionJnlpsInApplet/resources/ExtensionJnlpHelper.jnlp: same
> * tests/reproducers/custom/ExtensionJnlpsInApplet/resources/ExtensionJnlpTestApplet.jnlp: same
> * tests/reproducers/custom/ExtensionJnlpsInApplet/srcs/ExtensionJnlpHelper.java: same
> * tests/reproducers/custom/ExtensionJnlpsInApplet/srcs/ExtensionJnlpTestApplet.java: same
> * tests/reproducers/custom/ExtensionJnlpsInApplet/srcs/Makefile: same
>
> PluginBridge had null parserSettings, which caused a NPE in JNLPFile parse() method later on. I wasn't sure if there was a particular reason why the PluginBridge constructor never called super(), so instead it instantiates a new ParserSettings on its own, with default values. The constructor also stores a list of extension JNLP files, which are then returned later when getResources is called with the param ExtensionDesc.class. The reproducer tests set up a scenario as described in the original bug report; one HTML file which uses jnlp_href tag. This JNLP file has another JNLP file as an extension. Each of these JNLP files references one JAR. One of these JARs contains a helper class which is referenced by a
> class in the other JAR. Previously this scenario would result in a NoClassDefFoundError for the helper class when a browser was used to load the HTML, although using javaws to launch the JNLP directly worked fine.
>
> Thanks,
>
> Andrew A
>
>
> 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)

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.
> +clean-reproducer:
> +	echo NOTHING TO CLEAN FOR $(TESTNAME)
> diff --git a/tests/reproducers/custom/ExtensionJnlpsInApplet/testcases/ExtensionJnlpsInAppletTest.java b/tests/reproducers/custom/ExtensionJnlpsInApplet/testcases/ExtensionJnlpsInAppletTest.java
> new file mode 100644
> --- /dev/null
> +++ b/tests/reproducers/custom/ExtensionJnlpsInApplet/testcases/ExtensionJnlpsInAppletTest.java
> @@ -0,0 +1,74 @@
> +/* ExtensionJnlpsInAppletTest.java
> +Copyright (C) 2013 Red Hat, Inc.
> +
> +This file is part of IcedTea.
> +
> +IcedTea is free software; you can redistribute it and/or
> +modify it under the terms of the GNU General Public License as published by
> +the Free Software Foundation, version 2.
> +
> +IcedTea is distributed in the hope that it will be useful,
> +but WITHOUT ANY WARRANTY; without even the implied warranty of
> +MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
> +General Public License for more details.
> +
> +You should have received a copy of the GNU General Public License
> +along with IcedTea; see the file COPYING.  If not, write to
> +the Free Software Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA
> +02110-1301 USA.
> +
> +Linking this library statically or dynamically with other modules is
> +making a combined work based on this library.  Thus, the terms and
> +conditions of the GNU General Public License cover the whole
> +combination.
> +
> +As a special exception, the copyright holders of this library give you
> +permission to link this library with independent modules to produce an
> +executable, regardless of the license terms of these independent
> +modules, and to copy and distribute the resulting executable under
> +terms of your choice, provided that you also meet, for each linked
> +independent module, the terms and conditions of the license of that
> +module.  An independent module is a module which is not derived from
> +or based on this library.  If you modify this library, you may extend
> +this exception to your version of the library, but you are not
> +obligated to do so.  If you do not wish to do so, delete this
> +exception statement from your version.
> + */
> +
> +import net.sourceforge.jnlp.ProcessResult;
> +import net.sourceforge.jnlp.ServerAccess.AutoClose;
> +import net.sourceforge.jnlp.annotations.Bug;
> +import net.sourceforge.jnlp.annotations.KnownToFail;
> +import net.sourceforge.jnlp.annotations.NeedsDisplay;
> +import net.sourceforge.jnlp.annotations.TestInBrowsers;
> +import net.sourceforge.jnlp.browsertesting.BrowserTest;
> +import net.sourceforge.jnlp.browsertesting.Browsers;
> +import net.sourceforge.jnlp.closinglisteners.AutoOkClosingListener;
> +
> +import org.junit.Assert;
> +import org.junit.Test;
> +
> +public class ExtensionJnlpsInAppletTest extends BrowserTest {
> +
> +    private static final String appletCloseString = AutoOkClosingListener.MAGICAL_OK_CLOSING_STRING;
> +
> +    @Test
> +    @NeedsDisplay
> +    @TestInBrowsers(testIn={Browsers.one})
> +    public void testExtensionJnlpsInAppletLaunch() throws Exception {
> +        ProcessResult pr = server.executeBrowser("/ExtensionJnlpTest.html", AutoClose.CLOSE_ON_BOTH);
Those  slashes before resources are not necessary. I'm in favour to remove them,


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.

As you have already put an eye on reproducers engine, try to consider it.

> +        Assert.assertTrue("stdout should contain \"" + appletCloseString + "\" but did not", pr.stdout.contains(appletCloseString));
> +        Assert.assertTrue("stdout should contain \"Applet test running\" but did not", pr.stdout.contains("Applet test running"));
> +    }
> +
> +    @Test
> +    @NeedsDisplay
> +    @TestInBrowsers(testIn={Browsers.one})
> +    @Bug(id="PR974")
> +    public void testExtensionJnlpsInAppletHelper() throws Exception {
> +        ProcessResult pr = server.executeBrowser("/ExtensionJnlpTest.html", AutoClose.CLOSE_ON_BOTH);
> +        Assert.assertTrue("stdout should contain \"" + appletCloseString + "\" but did not", pr.stdout.contains(appletCloseString));
> +        Assert.assertTrue("stdout should contain \"Helper!\" but did not", pr.stdout.contains("Helper!"));
> +    }
> +
> +}

Otherwise nice job.
  J.