[rfc][icedtea-web] headless dialogues -tech preview
Jiri Vanek
jvanek at redhat.com
Mon Jun 15 15:21:30 UTC 2015
On 06/15/2015 04:59 PM, Andrew Azores wrote:
> Hi,
>
> On 14/06/15 02:04 PM, Jiri Vanek wrote:
>> Hi!
>>
>> the dialog subsytem in itw is wrong.
>> In last few and in a lot of future patches I'm trying to replace it.
>> When I was doing the remeberable solution, I noted that headless uspport is completly wrong, and
>> no worlaround may fix it.
>> Th eonly correct reaction is to print dialog to stdou, and request user to type answer. +1 from
>> me, her eis implementation :)
>> It consist 9again) from little bit more work, soI will push it in parts.
>> This patch add the headless logic, and have few advantages:
>> - no aditional text hanling. The dialogs can prints themselves based on current ui!
>> - no aditional input chekcing. With strong types, each can read itslef perfectly as it is already
>> done!
>>
>> - it encapsualtes a lot of suplicated code - eg the if (!sahouldAskUser)...if (trustAll) ..
>> If9trustNone).,... and more. So there is much more removed code then added :)
>>
>>
>> So the patch itself is quite good, isnt it? :)
>>
>> Few drawbacks
>> - stdin get broken if underlying application creates new VM. Currently I dont know how to
>> workaround it. But it may be surviable.
>> - it do nto work without X....
>>
>>
>> yah the last sentence is pitfall. It is casued by EVERYTHING in itw expects SecurityDialogue to be
>> existing. And new Window() is on the blacklist in headless mode.
>> But I have some ideas how to ix it....
>>
>>
>> J.
>
> Very cool ideas here! I think I like the approach in general and some of the refactoring is very
> nice. I do have some issues with the patch though:
>
>> + OutputController.getLogger().printOutLn(dialog.getText().replace("<html>",
>> "").replace("</html>", "")); //see htmlize.. but eg a href is ok to keep
>
> I think the HTML-cleaning needs to be more rigorous than this. Sample below, but to me this really
> isn't sufficient and the output isn't clean looking.
>
was improved in v2 i sent few minuts ago.
>> + OutputController.getLogger().printOutLn("Type 'exit' to terminate ITW one f below values:");
>
> The print message here isn't really clear... perhaps "Type one of the below values, or 'exit' to
> cancel loading the [applet|application]" ? And should this message and others like it be in
> Messages.properties?
alrready moved to properties in v2. Ans sure, will be reworded.
>
>> + boolean keepGoing = true;
>> + do {
>> + try {
>> + OutputController.getLogger().printOutLn(dialog.getText().replace("<html>",
>> "").replace("</html>", "")); //see htmlize.. but eg a href is ok to keep
>> + OutputController.getLogger().printOutLn("Type 'exit' to terminate ITW one f below values:");
>> + OutputController.getLogger().printOutLn(dialog.helpToStdIn());
>> + String s = br.readLine();
>> + if (s==null){
>> + break;
>> + }
>> + if (s.trim().toLowerCase().equals("exit")) {
>> + JNLPRuntime.exit(0);
>> + }
>> + message.userResponse = dialog.readFromStdIn(s);
>> + keepGoing = false;
>> + //br.close(); we cant close, it closes underlyig stream
>> + } catch (IOException eex) {
>> + OutputController.getLogger().log(eex);
>> + keepGoing = false;
>> + //br.close(); we cant close, it closes underlyig stream
>> + } catch (Exception ex) {
>> + OutputController.getLogger().log(OutputController.Level.MESSAGE_ALL, ex.toString());
>> + OutputController.getLogger().log(OutputController.Level.ERROR_ALL, ex);
>> + }
>> + } while (keepGoing);
>
> Perhaps rather than looping the whole thing and reprinting the "dialog", can you just reprint the
> prompt for input? And related, as it is now, it's not very obvious when I input an invalid response
> - I get a stack trace that makes perfect sense to me but I'd expect to make zero sense to the
> average end user:
I had it like this in early stage of this patch. But becasuse whole exception is printed in case
wrong output, it may be hard to recall what the I';m answering.
Conisdering loops of
message, help+suggested values, pass/exception
help+suggested values, pass/exception
message, help+suggested values, pass/better message
help+suggested values, pass/better message
I choose used "message, help+suggested values, pass/exception"
My reasons are - smallest amount of work now. Straightforward and working.
and - most important - headless ITW will not be run by average user :)
>
>> Type 'exit' to terminate ITW one f below values:
>> [YES, NO]
>> NotARealResponse
>> java.lang.IllegalArgumentException: No enum constant
>> net.sourceforge.jnlp.security.dialogresults.BasicDialogValue.Primitive.NotARealResponse
>> java.lang.IllegalArgumentException: No enum constant
>> net.sourceforge.jnlp.security.dialogresults.BasicDialogValue.Primitive.NotARealResponse
>> at java.lang.Enum.valueOf(Enum.java:238)
>> at
>> net.sourceforge.jnlp.security.dialogresults.BasicDialogValue$Primitive.valueOf(BasicDialogValue.java:43)
>>
>> at net.sourceforge.jnlp.security.dialogresults.YesNo.readValue(YesNo.java:52)
>> at
>> net.sourceforge.jnlp.security.dialogs.MissingPermissionsAttributePanel.readFromStdIn(MissingPermissionsAttributePanel.java:197)
>>
>> at net.sourceforge.jnlp.security.SecurityDialog.readFromStdIn(SecurityDialog.java:430)
>> at
>> net.sourceforge.jnlp.security.SecurityDialogMessageHandler.handleMessage(SecurityDialogMessageHandler.java:161)
>>
>> at
>> net.sourceforge.jnlp.security.SecurityDialogMessageHandler.run(SecurityDialogMessageHandler.java:95)
>> at java.lang.Thread.run(Thread.java:745)
>
> This is also printed above the reprinted dialog, so it would be easy to miss.
>
>> + return "You can type YES/NO or komplex answe parsable by
>> AccessWarningPaneComplexReturn.readValue.";
>
> This really needs to be rewritten. I don't think anything after "YES/NO" is going to make sense to
> end users - how are they supposed to know what that readValue method is able to parse?
>
>> + //this is default SecurityDialog "toString". All extending panels are recommanded to override
>> this.
>
> Maybe this would be better as a Javadoc comment?
sure
>
>> + //But what the hhell, it is working awesom!
>
> And maybe this one is better removed...
Rather just typo free (as it is in v2) I would like to keep similar comment anyway...
>
>> + public String getText() {
>> + return traverse(this);
>> + }
>
>> + @Override
>> + public DialogResult getDefaultNegativeAnswer() {
>> + return YesNoSandboxLimited.no();
>> + }
>
> There are a lot of instances of weird indentation or lack of inner whitespace in this patch, like
> the override annotation here being two spaces over. Can you have your IDE apply autoformatting to
> each touched file please?
I usually do :( Exactly this ">> + @Override" +-2sapces is slipping pretty often. When I will
post this part, I will double check.
>
>> diff -r 0afcc93fb7fb
>> tests/netx/unit/net/sourceforge/jnlp/security/dialogresults/NamePasswordTest.java
>> --- /dev/null Thu Jan 01 00:00:00 1970 +0000
>> +++ b/tests/netx/unit/net/sourceforge/jnlp/security/dialogresults/NamePasswordTest.java Sun Jun
>> 14 19:52:22 2015 +0200
>> @@ -0,0 +1,57 @@
>> +/*
>> + Copyright (C) 2009 Red Hat, Inc.
>
> Is it? :)
I hate those headers:( So just close, but yes.. :(you are right...
>
>
> Here's the sample "dialog" on stdout that I'm looking at right now:
>
>> $ ./bin/javaws -headless ~/Downloads/1d_fast_fourier_transform_java_jnlp.jnlp
>> This application does not specify a Codebase in its manifest. Please verify with the applet's
>> vendor. Continuing. See:
>> http://docs.oracle.com/javase/7/docs/technotes/guides/jweb/security/no_redeploy.html for details.
>> This application does not specify a Codebase in its manifest. Please verify with the applet's
>> vendor. Continuing. See:
>> http://docs.oracle.com/javase/7/docs/technotes/guides/jweb/security/no_redeploy.html for details.
>> Application title was not found in manifest. Check with application vendor
>> Application title was not found in manifest. Check with application vendor
>> Application <span color='red'> 1D Fast Fourier Transform </span> from <span color='red'>
>> http://www.cs.brown.edu/exploratories/freeSoftware/repository/ </span> is missing the permissions
>> attribute. Applications without this attribute should not be trusted. Do you wish to allow this
>> application to run?Application <span color='red'> 1D Fast Fourier Transform </span> from <span
>> color='red'> http://www.cs.brown.edu/exploratories/freeSoftware/repository/ </span> is missing the
>> permissions attribute. Applications without this attribute should not be trusted. Do you wish to
>> allow this application to run?
>>
>> <head>
>>
>> </head>
>> <body>
>> For more information you can visit:<br><a
>> href="http://docs.oracle.com/javase/7/docs/technotes/guides/jweb/security/manifest.html#permissions">JAR
>>
>> File Manifest Attributes</a><br>and<br><a
>> href="http://docs.oracle.com/javase/7/docs/technotes/guides/jweb/security/no_redeploy.html">Preventing
>>
>> the repurposing of Applications</a>
>> </body>
>>
>>
>> <head>
>>
>> </head>
>> <body>
>> For more information you can visit:<br><a
>> href="http://docs.oracle.com/javase/7/docs/technotes/guides/jweb/security/manifest.html#permissions">JAR
>>
>> File Manifest Attributes</a><br>and<br><a
>> href="http://docs.oracle.com/javase/7/docs/technotes/guides/jweb/security/no_redeploy.html">Preventing
>>
>> the repurposing of Applications</a>
>> </body>
>>
>>
>> YesYes
>> NoNo
>> <b>Remember this option?</b><b>Remember this option?</b>
>> For appletFor applet
>> For site <u>http://www.cs.brown.edu/exploratories/freeSoftware/repository/</u>For site
>> <u>http://www.cs.brown.edu/exploratories/freeSoftware/repository/</u>
>>
>> Type 'exit' to terminate ITW one f below values:
>> [YES, NO]
>
> The HTML tags don't really look very good, and most things that are printed are printed twice. I
> don't know what's causing that and how easy it is to fix or work around, but I think that's a
> blocker for this patch.
This is not bug, its an feature!-)
You can see how this text is composed? Also do you realize target audience of this patch? Also have
you seen big comment " //this is default SecurityDialog "toString". All extending panels are
recommanded to override " ?
So this method is providing some *good* default text. (in v2 the removal of html tags was much improved.
The providing of texts for exactly those messages will be work for another changesets. MAybe for
another person...
Well - the headless dialogues are now completly broken, so *anything* is better then current state.
>
> Also, here's another bug for you:
>
>> Type 'exit' to terminate ITW one f below values:
>> [YES, NO]
>> netx: Initialization Error: Could not initialize applet. (Your Extended applets security is at
yes I'm aware of it. I fixed it in v2. Bad typo on last second before posting :(
>> 'high' and this application is missing the 'permissions' attribute in manifest. And you have
>> refused to run it.)
>> netx: Initialization Error: Could not initialize applet. (Your Extended applets security is at
>> 'high' and this application is missing the 'permissions' attribute in manifest. And you have
>> refused to run it.)
>> net.sourceforge.jnlp.LaunchException: Fatal: Initialization Error: Could not initialize applet.
>> For more information click "more information button".
>> at net.sourceforge.jnlp.Launcher.createApplet(Launcher.java:764)
>> at net.sourceforge.jnlp.Launcher.launchApplet(Launcher.java:653)
>> at net.sourceforge.jnlp.Launcher$TgThread.run(Launcher.java:939)
>> Caused by: net.sourceforge.jnlp.LaunchException: Your Extended applets security is at 'high' and
>> this application is missing the 'permissions' attribute in manifest. And you have refused to run it.
>> at
>> net.sourceforge.jnlp.runtime.ManifestAttributesChecker.checkPermissionsAttribute(ManifestAttributesChecker.java:279)
>>
>> at
>> net.sourceforge.jnlp.runtime.ManifestAttributesChecker.checkAll(ManifestAttributesChecker.java:114)
>> at net.sourceforge.jnlp.runtime.JNLPClassLoader.initializeResources(JNLPClassLoader.java:777)
>> at net.sourceforge.jnlp.runtime.JNLPClassLoader.<init>(JNLPClassLoader.java:285)
>> at net.sourceforge.jnlp.runtime.JNLPClassLoader.createInstance(JNLPClassLoader.java:357)
>> at net.sourceforge.jnlp.runtime.JNLPClassLoader.getInstance(JNLPClassLoader.java:429)
>> at net.sourceforge.jnlp.runtime.JNLPClassLoader.getInstance(JNLPClassLoader.java:403)
>> at net.sourceforge.jnlp.Launcher.createApplet(Launcher.java:729)
>> ... 2 more
>
> I typed "Control-D"/EOF character for this. At this point it seems ITW is hanging.
>
Atatched v2 again.
Thanx!
J.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: headlessDialogues2.patch
Type: text/x-patch
Size: 60236 bytes
Desc: not available
URL: <http://mail.openjdk.java.net/pipermail/distro-pkg-dev/attachments/20150615/6db1f625/headlessDialogues2-0001.patch>
More information about the distro-pkg-dev
mailing list