[rfc][icedtea-web] jnlp_href extensions inside HTML applets - PR974
Andrew Azores
aazores at redhat.com
Thu Aug 1 12:41:50 PDT 2013
On 07/30/2013 01:46 AM, Jiri Vanek wrote:
> 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)
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?
>> +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,
Okay, they're gone now.
>
>
> 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.
This makes sense, I'll keep this in mind for the future (and it has been
changed now too).
>
>> + 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.
>
Changelog:
* netx/net/sourceforge/jnlp/runtime/Boot.java (main): Pass command line
arguments to ParserSettings and retrieve parser settings from there
* netx/net/sourceforge/jnlp/PluginBridge.java (PluginBridge,
getResources): Constructor stores list of JNLP extensions, getResources
returns this list. ParserSettings field instantiated from ParserSettings
new method getCommandLineParserSettings
* netx/net/sourceforge/jnlp/ParserSettings.java (cmdArgs,
setCommandLineArgs, getCommandLineParserSettings, getOption,
getOptions): new field to keep track of command line args. New methods
to return parser settings according to command line args.
*
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
Andrew A
-------------- next part --------------
A non-text attachment was scrubbed...
Name: fix2.patch
Type: text/x-patch
Size: 6754 bytes
Desc: not available
Url : http://mail.openjdk.java.net/pipermail/distro-pkg-dev/attachments/20130801/de54aed6/fix2.patch
-------------- next part --------------
A non-text attachment was scrubbed...
Name: reproducer2.patch
Type: text/x-patch
Size: 17133 bytes
Desc: not available
Url : http://mail.openjdk.java.net/pipermail/distro-pkg-dev/attachments/20130801/de54aed6/reproducer2.patch
More information about the distro-pkg-dev
mailing list