[RFC][icedtea-web]: Set plugin JVM options in itw-settings
Adam Domurad
adomurad at redhat.com
Wed Nov 21 13:14:19 PST 2012
On 11/21/2012 01:26 PM, Saad Mohammad wrote:
> Thanks Adam and Omair for your feedback.
>
> I improved the patch (mostly on the C++ side) and it now works well with
> multiple jvm arguments. I removed the 'clean up' part from my earlier patch
> since Omair pointed out it had been previously discussed and there was a
> disagreement towards that approach.
>
> Also to answer one of Omair's question, there are no user-visible performance
> differences using this approach of spawning a JVM to retrieve arguments.
>
> Bug Report:http://icedtea.classpath.org/bugzilla/show_bug.cgi?id=1217
>
> CHANGELOG
> ========================================================================
> 2012-11-21 Saad Mohammad<smohammad at redhat.com>
>
> Added new option in itw-settings which allows users to set JVM
> arguments when plugin is initialized.
> * netx/net/sourceforge/jnlp/config/Defaults.java (getDefaults):
> Added defaults for DeploymentConfiguration.KEY_PLUGIN_JVM_ARGUMENTS.
> * netx/net/sourceforge/jnlp/config/DeploymentConfiguration.java:
> Added new property (KEY_PLUGIN_JVM_ARGUMENTS) which stores the value of
> JVM plugin arguments.
> * netx/net/sourceforge/jnlp/controlpanel/ControlPanel.java:
> (createMainSettingsPanel): Added JVM settings to the list of tabs.
> (createJVMSettingsPanel): Returns a new JVMPanel object.
> * netx/net/sourceforge/jnlp/controlpanel/JVMPanel.java:
> JVM settings panel.
> * netx/net/sourceforge/jnlp/resources/Messages.properties:
> Added a new items (CPJVMPluginArguments, CPHeadJVMSettings,
> CPTabJVMSettings).
> * plugin/icedteanp/IcedTeaNPPlugin.cc:
> (plugin_start_appletviewer): Adds JVM arguments to the commands that
> initalize it.
> (get_jvm_args): Returns JVM arguments set in itw-settings.
> (tokenize_char_to_vector): Split string into vector<gchar*> separated by
> newlines and spaces.
> ========================================================================
>
Thanks for the update! This is great. However one issue I came across is
that when setting arbitrary invalid arguments for the plugin, firefox
hangs on load of the applet.
Comments inline.
> diff --git a/netx/net/sourceforge/jnlp/config/Defaults.java
> b/netx/net/sourceforge/jnlp/config/Defaults.java
> --- a/netx/net/sourceforge/jnlp/config/Defaults.java
> +++ b/netx/net/sourceforge/jnlp/config/Defaults.java
> @@ -378,6 +378,12 @@
> DeploymentConfiguration.KEY_UPDATE_TIMEOUT,
> BasicValueValidators.getRangedIntegerValidator(0, 10000),
> String.valueOf(500)
> + },
> + //JVM arguments for plugin
> + {
> + DeploymentConfiguration.KEY_PLUGIN_JVM_ARGUMENTS,
> + null,
> + null
> }
> };
This array is quite unwieldy, it would be nice if 2D array had a better
representation (just in general, not directly related to your patch).
>
> diff --git
> a/netx/net/sourceforge/jnlp/config/DeploymentConfiguration.java
> b/netx/net/sourceforge/jnlp/config/DeploymentConfiguration.java
> --- a/netx/net/sourceforge/jnlp/config/DeploymentConfiguration.java
> +++ b/netx/net/sourceforge/jnlp/config/DeploymentConfiguration.java
> @@ -158,6 +158,11 @@
> public static final String KEY_BROWSER_PATH =
> "deployment.browser.path";
> public static final String KEY_UPDATE_TIMEOUT =
> "deployment.javaws.update.timeout";
>
> + /*
> + * JVM arguments for plugin
> + */
> + public static final String KEY_PLUGIN_JVM_ARGUMENTS=
> "deployment.plugin.jvm.arguments";
> +
> public enum ConfigType {
> System, User
> }
> diff --git a/netx/net/sourceforge/jnlp/controlpanel/ControlPanel.java
> b/netx/net/sourceforge/jnlp/controlpanel/ControlPanel.java
> --- a/netx/net/sourceforge/jnlp/controlpanel/ControlPanel.java
> +++ b/netx/net/sourceforge/jnlp/controlpanel/ControlPanel.java
> @@ -226,6 +226,7 @@
> // new
> SettingsPanel(Translator.R("CPTabClassLoader"),
> createClassLoaderSettingsPanel()),
> new SettingsPanel(Translator.R("CPTabDebugging"),
> createDebugSettingsPanel()),
> new
> SettingsPanel(Translator.R("CPTabDesktopIntegration"),
> createDesktopSettingsPanel()),
> + new SettingsPanel(Translator.R("CPTabJVMSettings"),
> createJVMSettingsPanel()),
> new SettingsPanel(Translator.R("CPTabNetwork"),
> createNetworkSettingsPanel()),
> // TODO: This is commented out since this is not
> implemented yet
> // new SettingsPanel(Translator.R("CPTabRuntimes"),
> createRuntimesSettingsPanel()),
> @@ -319,6 +320,10 @@
> return new SecuritySettingsPanel(this.config);
> }
>
> + private JPanel createJVMSettingsPanel() {
> + return new JVMPanel(this.config);
> + }
> +
> /**
> * This is a placeholder panel.
> *
> diff --git a/netx/net/sourceforge/jnlp/controlpanel/JVMPanel.java
> b/netx/net/sourceforge/jnlp/controlpanel/JVMPanel.java
> new file mode 100644
> --- /dev/null
> +++ b/netx/net/sourceforge/jnlp/controlpanel/JVMPanel.java
> @@ -0,0 +1,85 @@
> +/* PluginPanel.java
> +Copyright (C) 2012, 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.
> +*/
> +package net.sourceforge.jnlp.controlpanel;
> +
> +import java.awt.Component;
> +import java.awt.Dimension;
> +import java.awt.GridBagConstraints;
> +import java.awt.GridBagLayout;
> +
> +import javax.swing.Box;
> +import javax.swing.JLabel;
> +import javax.swing.JTextField;
> +
> +import net.sourceforge.jnlp.config.DeploymentConfiguration;
> +import net.sourceforge.jnlp.runtime.Translator;
> +
> + at SuppressWarnings("serial")
> +public class JVMPanel extends NamedBorderPanel {
> + private DeploymentConfiguration config;
> +
> + JVMPanel(DeploymentConfiguration config) {
> + super(Translator.R("CPHeadJVMSettings"), new GridBagLayout());
> + this.config = config;
> + addComponents();
> + }
> +
> + private void addComponents() {
> + JLabel description = new JLabel("<html>" +
> Translator.R("CPJVMPluginArguments") + "<hr /></html>");
> + JTextField testFieldArguments = new JTextField(25);
> +
> + testFieldArguments.getDocument().addDocumentListener(new
> DocumentAdapter(config,
> DeploymentConfiguration.KEY_PLUGIN_JVM_ARGUMENTS));
> +
> testFieldArguments.setText(config.getProperty(DeploymentConfiguration.KEY_PLUGIN_JVM_ARGUMENTS));
> +
> + // Filler to pack the bottom of the panel.
> + GridBagConstraints c = new GridBagConstraints();
> + c.fill = GridBagConstraints.BOTH;
> + c.weightx = 1;
> + c.gridx = 0;
> + c.gridy = 0;
> +
> + this.add(description, c);
> + c.gridy++;
> + this.add(testFieldArguments, c);
> +
> + // This is to keep it from expanding vertically if resized.
> + Component filler = Box.createRigidArea(new Dimension(1, 1));
> + c.gridy++;
> + c.weighty++;
> + this.add(filler, c);
> + }
> +}
> diff --git a/netx/net/sourceforge/jnlp/resources/Messages.properties
> b/netx/net/sourceforge/jnlp/resources/Messages.properties
> --- a/netx/net/sourceforge/jnlp/resources/Messages.properties
> +++ b/netx/net/sourceforge/jnlp/resources/Messages.properties
> @@ -299,6 +299,7 @@
> CPSecurityDescription=Use this to configure security settings.
> CPDebuggingDescription=Enable options here to help with debugging
> CPDesktopIntegrationDescription=Set whether or not to allow creation
> of desktop shortcut.
> +CPJVMPluginArguments=Set JVM arguments for plugin.
Can we handle the Czech translation being out of sync yet ? (Jiri?) If
not Jiri or Pavel will have to translate these.
>
> # Control Panel - Buttons
> CPButAbout=About...
> @@ -317,6 +318,7 @@
> CPHeadDebugging=Debugging Settings
> CPHeadDesktopIntegration=Desktop Integrations
> CPHeadSecurity=Security Settings
> +CPHeadJVMSettings=JVM Settings
>
> # Control Panel - Tabs
> CPTabAbout=About IcedTea-Web
> @@ -328,6 +330,7 @@
> CPTabNetwork=Network
> CPTabRuntimes=Runtimes
> CPTabSecurity=Security
> +CPTabJVMSettings=JVM Settings
>
> # Control Panel - AboutPanel
> CPAboutInfo=This is the control panel for setting
> deployments.properties.<br/>Not all options will take effect until
> implemented.<br/>The use of multiple JREs is currently unsupported.<br/>
> diff --git a/plugin/icedteanp/IcedTeaNPPlugin.cc
> b/plugin/icedteanp/IcedTeaNPPlugin.cc
> --- a/plugin/icedteanp/IcedTeaNPPlugin.cc
> +++ b/plugin/icedteanp/IcedTeaNPPlugin.cc
> @@ -243,6 +243,11 @@
> void plugin_send_initialization_message(char* instance, gulong handle,
> int width, int height,
> char* url);
> +/* Returns JVM options set in itw-settings */
> +static std::vector<gchar*> get_jvm_args();
> +/* Removes newlines and extra spaces from char */
> +static std::vector<gchar*> tokenize_char_to_vector(const char*
> to_tokenize);
> +
>
> // Global instance counter.
> // Mutex to protect plugin_instance_counter.
> @@ -1617,48 +1622,62 @@
> PLUGIN_DEBUG ("plugin_start_appletviewer\n");
> NPError error = NPERR_NO_ERROR;
>
> - gchar** command_line;
> + std::vector<gchar*> command_line;
> + std::vector<gchar*> jvm_args = get_jvm_args();
vector<gchar*> is a good compromise, however I also like the idea of
using a vector<string> and at accumulating all the character pointers
(not copying the strings) into a vector<gchar*> at the last moment. A
utility method could do this and it'd avoid the need for manual strdup's
(the memory will always be copied into the resulting string).
> gchar** environment;
>
> - int cmd_num = 0;
> if (plugin_debug)
> {
> - command_line = (gchar**) malloc(sizeof(gchar*)*11);
> - command_line[cmd_num++] = g_strdup(appletviewer_executable);
> - command_line[cmd_num++] = g_strdup(PLUGIN_BOOTCLASSPATH);
> - // set the classpath to avoid using the default (cwd).
> - command_line[cmd_num++] = g_strdup("-classpath");
> - command_line[cmd_num++] = g_strdup_printf("%s/lib/rt.jar",
> ICEDTEA_WEB_JRE);
> - command_line[cmd_num++] = g_strdup("-Xdebug");
> - command_line[cmd_num++] = g_strdup("-Xnoagent");
> - if (plugin_debug_suspend)
> - {
> - command_line[cmd_num++] =
> g_strdup("-Xrunjdwp:transport=dt_socket,address=8787,server=y,suspend=y");
> - } else
> - {
> - command_line[cmd_num++] =
> g_strdup("-Xrunjdwp:transport=dt_socket,address=8787,server=y,suspend=n");
> - }
> - command_line[cmd_num++] = g_strdup("sun.applet.PluginMain");
> - command_line[cmd_num++] = g_strdup(out_pipe_name);
> - command_line[cmd_num++] = g_strdup(in_pipe_name);
> - command_line[cmd_num] = NULL;
> + command_line.push_back(g_strdup(appletviewer_executable));
> +
> + //Add JVM args to command_line
> + for (int i = 0; i < jvm_args.size(); i++)
> + {
> + command_line.push_back(jvm_args[i]);
> + }
> +
> + command_line.push_back(g_strdup(PLUGIN_BOOTCLASSPATH));
> + // set the classpath to avoid using the default (cwd).
> + command_line.push_back(g_strdup("-classpath"));
> + command_line.push_back(g_strdup_printf("%s/lib/rt.jar",
> ICEDTEA_WEB_JRE));
> + command_line.push_back(g_strdup("-Xdebug"));
> + command_line.push_back(g_strdup("-Xnoagent"));
> +
> + if (plugin_debug_suspend)
> + {
> +
> command_line.push_back(g_strdup("-Xrunjdwp:transport=dt_socket,address=8787,server=y,suspend=y"));
> + } else
> + {
> +
> command_line.push_back(g_strdup("-Xrunjdwp:transport=dt_socket,address=8787,server=y,suspend=n"));
> + }
> + command_line.push_back(g_strdup("sun.applet.PluginMain"));
> + command_line.push_back(g_strdup(out_pipe_name));
> + command_line.push_back(g_strdup(in_pipe_name));
> + command_line.push_back(g_strdup(NULL));
> } else
> {
> - command_line = (gchar**) malloc(sizeof(gchar*)*8);
> - command_line[cmd_num++] = g_strdup(appletviewer_executable);
> - command_line[cmd_num++] = g_strdup(PLUGIN_BOOTCLASSPATH);
> - command_line[cmd_num++] = g_strdup("-classpath");
> - command_line[cmd_num++] = g_strdup_printf("%s/lib/rt.jar",
> ICEDTEA_WEB_JRE);
> - command_line[cmd_num++] = g_strdup("sun.applet.PluginMain");
> - command_line[cmd_num++] = g_strdup(out_pipe_name);
> - command_line[cmd_num++] = g_strdup(in_pipe_name);
> - command_line[cmd_num] = NULL;
> + command_line.push_back(g_strdup(appletviewer_executable));
> +
> + //Add JVM args to command_line
> + for (int i = 0; i < jvm_args.size(); i++)
> + {
> + command_line.push_back(jvm_args[i]);
> + }
> +
> + command_line.push_back(g_strdup(PLUGIN_BOOTCLASSPATH));
> + command_line.push_back(g_strdup("-classpath"));
> + command_line.push_back(g_strdup_printf("%s/lib/rt.jar",
> ICEDTEA_WEB_JRE));
> + command_line.push_back(g_strdup("sun.applet.PluginMain"));
> + command_line.push_back(g_strdup(out_pipe_name));
> + command_line.push_back(g_strdup(in_pipe_name));
> + command_line.push_back(g_strdup(NULL));
> }
>
> environment = plugin_filter_environment();
> -
> - if (!g_spawn_async (NULL, command_line, environment,
> - (GSpawnFlags) G_SPAWN_DO_NOT_REAP_CHILD,
> + gchar** command_line_args = &command_line[0];
> +
> + if (!g_spawn_async (NULL, command_line_args, environment,
> + (GSpawnFlags) G_SPAWN_DO_NOT_REAP_CHILD,
> NULL, NULL, &appletviewer_pid, &channel_error))
> {
> if (channel_error)
> @@ -1673,15 +1692,15 @@
> error = NPERR_GENERIC_ERROR;
> }
>
> - g_strfreev (environment);
> -
> - for (int i = 0; i < cmd_num; i++) {
> - g_free (command_line[i]);
> + //Free memory
> + g_strfreev(environment);
> +
> + for (int i = 0; i < command_line.size(); i++)
> + {
> + g_free(command_line[i]);
> command_line[i] = NULL;
> }
> -
> - g_free(command_line);
> - command_line = NULL;
> + command_line_args = NULL;
>
> if (appletviewer_pid)
> {
> @@ -1689,12 +1708,85 @@
> appletviewer_watch_id = g_child_watch_add(appletviewer_pid,
> (GChildWatchFunc) appletviewer_monitor, (gpointer) appletviewer_pid);
> }
>
> -
> PLUGIN_DEBUG ("plugin_start_appletviewer return\n");
> return error;
> }
>
> /*
> + * Returns JVM options set in itw-settings
> + */
> +static std::vector<gchar*>
> +get_jvm_args()
> +{
> + gchar** commands;
> + gchar *output;
> + int cmd_num = 0;
> +
> + commands = (gchar**) g_malloc(sizeof(gchar*) * 8);
> + commands[cmd_num++] = g_strdup(appletviewer_executable);
> + commands[cmd_num++] = g_strdup(PLUGIN_BOOTCLASSPATH);
> + commands[cmd_num++] = g_strdup("-classpath");
> + commands[cmd_num++] = g_strdup_printf("%s/lib/rt.jar",
> ICEDTEA_WEB_JRE);
> + commands[cmd_num++] =
> g_strdup("net.sourceforge.jnlp.controlpanel.CommandLine");
> + commands[cmd_num++] = g_strdup("get");
> + commands[cmd_num++] = g_strdup("deployment.plugin.jvm.arguments");
> + commands[cmd_num] = NULL;
Please use a vector here as well.
> +
> + if (!g_spawn_sync(NULL, commands, NULL,
> + (GSpawnFlags) G_SPAWN_STDERR_TO_DEV_NULL, NULL, NULL, &output,
> NULL, NULL,
> + &channel_error))
> + {
> + PLUGIN_ERROR("Failed to get JVM arguments set for plugin.");
> + output = NULL;
> + }
> +
> + std::vector<gchar*> tokenizedOutput = tokenize_char_to_vector(output);
> +
> + //Free memory
> + for (int i = 0; i < cmd_num; i++)
> + {
> + g_free(commands[i]);
> + commands[i] = NULL;
> + }
> + g_free(commands);
> + commands = NULL;
> + g_free(output);
> +
> + return tokenizedOutput;
> +}
> +
> +/*
> + * Split string into vector<gchar*> separated by newlines and spaces
> + */
> +static std::vector<gchar*>
> +tokenize_char_to_vector(const char* to_tokenize)
This seems redundant with IcedTeaPluginUtilities::strSplit(str, " ").
> +{
> + //Tokenize
> + std::vector<gchar*> output_tokens;
> +
> + if (to_tokenize != NULL)
> + {
> + for (int i = 0; i < strlen(to_tokenize); i++)
> + {
> + std::string arg("");
> +
> + while (i < strlen(to_tokenize) && to_tokenize[i] != ' ' &&
> to_tokenize[i] != '\n')
Please don't use 'strlen' in loop conditions as it takes time linear
with string size.
> + {
> + arg.append(1, to_tokenize[i]);
> + i++;
> + }
> +
> + if (arg.length() > 0 && arg != "null")
> + {
> + output_tokens.push_back(g_strdup(arg.c_str()));
> + }
> + }
> + }
> +
> + return output_tokens;
> +}
> +
> +/*
> * Replaces certain characters (\r, \n, etc) with HTML escape
> equivalents.
> *
> * Return string is allocated on the heap. Caller assumes responsibility
Thanks again for the update, looks good overall. Maybe try out the new
unit testing system if you're writing small utility methods.
-Adam
More information about the distro-pkg-dev
mailing list