[RFC][icedtea-web]: Set plugin JVM options in itw-settings
Adam Domurad
adomurad at redhat.com
Tue Dec 4 11:25:40 PST 2012
On 12/03/2012 04:58 PM, Saad Mohammad wrote:
> Hi Adam,
>
> Thanks for the feedback! I decided to go with your recommendation to remove
> manual memory allocation/deallocation and took advantage of std::string!
>
> Updated patch is attached.
>
> CHANGELOG
> ========================================================================
> 2012-12-03 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 line list.
> (get_jvm_args): Returns JVM arguments set in itw-settings.
> * plugin/icedteanp/IcedTeaPluginUtils.cc:
> (IcedTeaPluginUtilities::vectorstring_to_vectorgchar): New helper method
> which returns a vector of gchar* from the vector of strings passed.
> * plugin/icedteanp/IcedTeaPluginUtils.h:
> Declaration of IcedTeaPluginUtilities::vectorstring_to_vectorgchar.
>
> ========================================================================
>
Thanks for sticking with it :)
Comments below
> 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
> }
> };
>
> 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.
>
> # 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,8 @@
> 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<std::string*>* get_jvm_args();
>
> // Global instance counter.
> // Mutex to protect plugin_instance_counter.
> @@ -1617,48 +1619,61 @@
> PLUGIN_DEBUG ("plugin_start_appletviewer\n");
> NPError error = NPERR_NO_ERROR;
>
> - gchar** command_line;
> - gchar** environment;
> -
> - int cmd_num = 0;
> + std::vector<std::string> command_line;
> + gchar** environment = NULL;
> + std::vector<std::string*>* jvm_args = get_jvm_args();
> + int jvm_args_count = jvm_args->size();
If 'jvm_args_count' is only used in the loop below, I think its clearer
and more idiomatic to just have jvm_args->size() in the loop condition.
> +
> 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(std::string(appletviewer_executable));
std::string(..) is not needed here, push_back(appletviewer_executable)
suffices.
> +
> + //Add JVM args to command_line
> + for (int i = 0; i < jvm_args_count; i++)
> + {
> + command_line.push_back(jvm_args->at(i)->c_str());
You are taking a C++ string, returning the C string it holds, and then
copying it into a C++ string here :)
Simpler:
command_line.push_back(jvm_args->at(i));
> + }
> +
> + command_line.push_back(PLUGIN_BOOTCLASSPATH);
> + // set the classpath to avoid using the default (cwd).
> + command_line.push_back("-classpath");
> + command_line.push_back(std::string(ICEDTEA_WEB_JRE "/lib/rt.jar"));
std::string(..) is not needed here (and inconsistent with the other
pushbacks)
> + command_line.push_back("-Xdebug");
> + command_line.push_back("-Xnoagent");
> +
> + if (plugin_debug_suspend)
> + {
> + command_line.push_back("-Xrunjdwp:transport=dt_socket,address=8787,server=y,suspend=y");
> + } else
> + {
> + command_line.push_back("-Xrunjdwp:transport=dt_socket,address=8787,server=y,suspend=n");
> + }
Consider doing:
std::string debug_flags =
"-Xrunjdwp:transport=dt_socket,address=8787,server=y,";
debug_flags += plugin_debug_suspend ? "suspend=y" : "suspend=n";
command_line.push_back(debug_flags);
It is easier to tell what effect the argument has this way.
> + command_line.push_back("sun.applet.PluginMain");
> + command_line.push_back(out_pipe_name);
> + command_line.push_back(in_pipe_name);
> } 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(std::string(appletviewer_executable));
> +
> + //Add JVM args to command_line
> + for (int i = 0; i < jvm_args_count; i++)
> + {
> + command_line.push_back(jvm_args->at(i)->c_str());
Simpler to not unbox the string (ie no c_str()).
> + }
> +
> + command_line.push_back(PLUGIN_BOOTCLASSPATH);
> + command_line.push_back("-classpath");
> + command_line.push_back(std::string(ICEDTEA_WEB_JRE "/lib/rt.jar"));
std::string(..) should be dropped IMO.
> + command_line.push_back("sun.applet.PluginMain");
> + command_line.push_back(out_pipe_name);
> + command_line.push_back(in_pipe_name);
> }
>
> environment = plugin_filter_environment();
> -
> - if (!g_spawn_async (NULL, command_line, environment,
> - (GSpawnFlags) G_SPAWN_DO_NOT_REAP_CHILD,
> + gchar **command_line_args = &IcedTeaPluginUtilities::vectorstring_to_vectorgchar(&command_line)[0];
This is partially why I was wary of recommending this solution. This
causes memory corruption! Do not point into temporary objects.
In C++, a vector should be understood like this ...
Say we have this expression:
vector<int>();
This is like saying:
int* temporary = (int*) malloc (..);
free(temporary);
And say we have something like this ...
int* p = &vector<int>()[0];
It would be:
int* temporary = (int*) malloc (..);
int* p = temporary;
free(temporary);
'p' is now a 'stale pointer'. It points to a memory region that at any
time can be reclaimed because it has been freed.
(Note that I used malloc and free because I thought they were a little
clearer, to be exact 'new int[..]' and 'delete[]' would be used)
> +
> + 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 +1688,16 @@
> error = NPERR_GENERIC_ERROR;
> }
>
> - g_strfreev (environment);
> -
> - for (int i = 0; i < cmd_num; i++) {
> - g_free (command_line[i]);
> - command_line[i] = NULL;
> + //Free memory
> + g_strfreev(environment);
> + command_line_args = NULL;
> +
> + for (int i = 0; i < jvm_args_count; i++)
> + {
> + free(jvm_args->at(i));
Not quite! This leaks memory in a subtle way. An std::string internally
has a pointer to string data.
This is a bit like doing:
char** str = malloc(..);
*str = malloc(..);
free(str);
Correct here is:
+ delete jvm_args->at(i);
This does what is intended, allowing an std::string to manage its memory
via its destructor (Makes you appreciate GC more I'm sure :)).
However ITW has a method IcedTeaPluginUtilities::freeStringPtrVector for
this usage (will also cover the 'delete jvm_args' at the end.)
> }
> -
> - g_free(command_line);
> - command_line = NULL;
> + delete jvm_args;
> + jvm_args = NULL;
>
> if (appletviewer_pid)
> {
> @@ -1689,12 +1705,56 @@
> 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<std::string*>*
> +get_jvm_args()
> +{
> + std::vector < std::string> commands;
> + gchar *output = NULL;
> + std::vector<std::string*>* tokenOutput = NULL;
> +
> + commands.push_back(appletviewer_executable);
> + commands.push_back(PLUGIN_BOOTCLASSPATH);
> + commands.push_back("-classpath");
> + commands.push_back(std::string(ICEDTEA_WEB_JRE "/lib/rt.jar"));
explicit std::string call a bit out of place here
> + commands.push_back("net.sourceforge.jnlp.controlpanel.CommandLine");
> + commands.push_back("get");
> + commands.push_back("deployment.plugin.jvm.arguments");
> +
> + gchar **command_line_args = &IcedTeaPluginUtilities::vectorstring_to_vectorgchar(&commands)[0];
See my note above about pointing into temporaries.
> +
> + if (!g_spawn_sync(NULL, command_line_args, 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;
> + }
> +
> + tokenOutput = IcedTeaPluginUtilities::strSplit(output, " \n");
> +
> + if (tokenOutput->size() > 0
> + && strcmp(tokenOutput->at(0)->c_str(), "null") == 0)
> + {
More idiomatic is:
if (!tokenOutput->empty() && *tokenOutput->at(0) == "null")
(On another note, I really wish these were just returning
vectors-of-strings consistently, these
vector-pointers-of-string-pointers are annoying and make it hard to
write clean code.)
> + free(tokenOutput->at(0));
delete here, see my note above.
> + tokenOutput->erase(tokenOutput->begin());
> + }
> +
> + //Free memory
> + g_free(output);
> + output = NULL;
> + command_line_args = NULL;
> +
> + return tokenOutput;
> +}
> +
> +/*
> * Replaces certain characters (\r, \n, etc) with HTML escape equivalents.
> *
> * Return string is allocated on the heap. Caller assumes responsibility
> diff --git a/plugin/icedteanp/IcedTeaPluginUtils.cc b/plugin/icedteanp/IcedTeaPluginUtils.cc
> --- a/plugin/icedteanp/IcedTeaPluginUtils.cc
> +++ b/plugin/icedteanp/IcedTeaPluginUtils.cc
> @@ -1005,6 +1005,24 @@
> }
>
> /**
> + * Returns a vector of gchar* from the vector string reference
> + * @param stringVec The vector of strings reference.
> + */
> +std::vector<gchar*>
> +IcedTeaPluginUtilities::vectorstring_to_vectorgchar(const std::vector<std::string>* stringVec)
> +{
> + std::vector<gchar*> charVec;
> +
> + for (int i = 0; i < stringVec->size(); i++)
> + {
> + gchar* character = (gchar*) stringVec->at(i).c_str(); //cast from const char
'character' isn't a good name here. &stringVec->at(i)[0] would not
require the cast -- although, isn't brilliant looking though, the
const-dropping cast is probably better here.
> + charVec.push_back(character);
> + }
> + charVec.push_back(NULL);
> + return charVec;
> +}
> +
> +/**
> * Runs through the async call wait queue and executes all calls
> *
> * @param param Ignored -- required to conform to NPN_PluginThreadAsynCall API
> diff --git a/plugin/icedteanp/IcedTeaPluginUtils.h b/plugin/icedteanp/IcedTeaPluginUtils.h
> --- a/plugin/icedteanp/IcedTeaPluginUtils.h
> +++ b/plugin/icedteanp/IcedTeaPluginUtils.h
> @@ -274,6 +274,9 @@
>
> static void decodeURL(const char* url, char** decoded_url);
>
> + /* Returns a vector of gchar* from the vector string reference*/
> + static std::vector<gchar*> vectorstring_to_vectorgchar(const std::vector<std::string>* stringVec);
> +
Should comment here somehow indicating that the gchar* pointers point to
the internal std::string data. Specifically whenever returning pointers
it should be clear whether they need to be freed. The would use
camelCase here too (eg stringVectorAsGcharPointers could be a good name
that implies no copying, but it's a little hard to name because it has
some nuances to its usage.)
> /* Posts call in async queue and waits till execution completes */
> static void callAndWaitForResult(NPP instance, void (*func) (void *), AsyncCallThreadData* data);
> };
Good work! Don't get discouraged by the amounts of nits :) It is almost
there, and you did get quite a few tricky details correct. Learning
different languages is good for the programming-soul :)
Happy hacking,
-Adam
-------------- next part --------------
An HTML attachment was scrubbed...
URL: http://mail.openjdk.java.net/pipermail/distro-pkg-dev/attachments/20121204/b46ceffb/attachment.html
More information about the distro-pkg-dev
mailing list