[RFC][icedtea-web] Fix for PR855: AppletStub getDocumentBase() doesn't return full URL

[Danesh Dadachanji]

On 08/06/12 12:23 PM, Danesh Dadachanji wrote:
> On 07/06/12 04:32 PM, Danesh Dadachanji wrote:
>> On 05/06/12 08:15 AM, Jiri Vanek wrote:
>>> On 06/04/2012 11:53 PM, Danesh Dadachanji wrote:
>>>> On 18/05/12 05:15 AM, Jiri Vanek wrote:
>>>>> On 05/17/2012 11:16 PM, Danesh Dadachanji wrote:
>>>>>>
>>>>>> On 16/04/12 01:18 PM, Deepak Bhole wrote:
>>>>>>> * Danesh Dadachanji<ddadacha at redhat.com> [2012-02-03 15:40]:
>>>>>>>> Hi,
>>>>>>>>
>>>>>>>> This patch is a fix for PR855[1]. The return of getDocumentBase()
>>>>>>>> currently doesn't include the HTML file. According to the docs, it
>>>>>>>> should return the entire path of the HTML, including the filename.
>>>>>>>>
>>>>>>>> Attached is a patch that adds this. I've checked all the traces of
>>>>>>>> the bytes grabbed from the pipe over on the java side. I believe I
>>>>>>>> covered every read and assignment of the buffer/vars setup from the
>>>>>>>> buffer and so on. We fortunately only ever used getDocumentBase()'s
>>>>>>>> return to grab the host IP or use it as a base URL. The latter code
>>>>>>>> grabbed the return and cut off everything after the last '/', which
>>>>>>>> back before patch is the entire URL, post patch is the URL excluding
>>>>>>>> the file name.
>>>>>>>>
>>>>>>>> Okay for HEAD? Thoughts on backporting to release branches? It was
>>>>>>>> reported against 1.1.3 but this is a pretty big change that
>>>>>>>> potentially affects the way users' applications behave.
>>>>>>>>
>>>>>>>
>>>>>>> Looking at the surrounding code, we are taking a URL, splitting it on
>>>>>>> "/" and then combining the first length-1 components. This was done to
>>>>>>> purposely remove the file name itself.
>>>>>>>
>>>>>>> However if we want to add the file name back, we should just remove all
>>>>>>> of the splitting/combing code rather than appending the last part.
>>>>>>>
>>>>>>> Have you tried using NPVARIANT_TO_STRING(href).utf8characters directly?
>>>>>>>
>>>>>>
>>>>>> Ah yes I wasn't paying attention to the surrounding code :S
>>>>>>
>>>>>> How's this look? I'd like to backport this to 1.2 as well. The reporter uses 1.1.3 though so perhaps
>>>>>> it should go into 1.1 too? Thoughts?
>>>>>>
>>>>>> Cheers,
>>>>>> Danesh
>>>>> Can we get also test for this? :(
>>>>
>>>> Here we go. =) Since there was a long delay in this (sorry!), the original file has changed quite a bit in the last few days, I have
>>>> been waiting for those to go in so I could update mine accordingly. Please note the new changes!
>>>>
>>>> I've added a test for this and a test for checking what codebase should be. I also threw in an extra jnlp_href test because we don't
>>>> have one! 3 tests with one stone. ;)
>>>>
>>>> I also took the opportunity to make some minor typos/mistakes in AppletTest's asserts.
>>>>
>>>> ChangeLog:
>>>> +2012-06-04 Danesh Dadachanji <ddadacha at redhat.com>
>>>> +
>>>> + PR855: AppletStub getDocumentBase() doesn't return full URL
>>>> + * plugin/icedteanp/IcedTeaNPPlugin.cc (plugin_get_documentbase):
>>>> + Assign documentbase_copy directly to href's value instead of iterating
>>>> + through the segments to remove the file from the path.
>>>> + * tests/jnlp_tests/simple/AppletTest/srcs/AppletTest.java:
>>>> + Added prints of codebase and document base when initializing.
>>>> + * tests/jnlp_tests/simple/AppletTest/testcases/AppletTestTests.java:
>>>> + (evaluateApplet): New param baseName, represents the HTML file name
>>>> + when calling applet.getDocumentBase().
>>>> + * tests/jnlp_tests/simple/AppletTest/resources/appletJNLPHrefTest.html:
>>>> + New test that runs AppletTest via jnlp_href.
>>>> +
>>>>
>>>> I would still like to backport this to 1.2 but it may require me backporting the fixes for PR518 and PR863. I'm unsure if it needs to
>>>> go to 1.1, the reporter said they used 1.1 though. Thoughts?
>>> I think there should be no issue with backporting and also cc part of code looks ok.
>>
>> PR518 and PR863 are being backported as we speak, I will backport the attached patch (if it's approved =) ) shortly.
>>
>>> But I have one bigger issue with test - do you think it can be separated reproducer? I know they will be very similar but I would like
>>> have each reproducer to simulate as little as possible.
>>
>> Fair enough, AppletTest should only test the bare minimum to get an applet up and running. The 2 methods are additional on top of that,
>> I have created AppletBaseURLTest to do what is required.
>>
>>> Also do not forget to add @Bug(id="PR855") to the testcase class and test method;)
>>
>> Done
>>
>>> Also please be sure to use new logging functions (I guess you have reviewed them;) instead of all stdout/err in testcaes (not
>>> reproducer itself) and do no reprint stdout of processes. (I doubt the code you have used as stub exist anymore...)
>>
>> Done, they must have snuck back in when I patched via an old diff I was using, sorry!
>>
>>> Also there is no need to change timeout in your new reproducer.
>>
>> Ah okay, done!
>>
>>>
>>> Thanx again for patch and test!
>>
>> Thanks for reviewing!
>>
>
>
> Realized I had not added a NEWS entry, patch updated.
>
> Updated ChangeLog:
> +2012-06-07 Danesh Dadachanji <ddadacha at redhat.com>
> +
> + PR855: AppletStub getDocumentBase() doesn't return full URL
> + * NEWS: Added PR855 entry.
> + * plugin/icedteanp/IcedTeaNPPlugin.cc (plugin_get_documentbase):
> + Assign documentbase_copy directly to href's value instead of iterating
> + through the segments to remove the file from the path.
> + * tests/jnlp_tests/simple/AppletBaseURLTest/srcs/AppletBaseURL.java:
> + * tests/jnlp_tests/simple/AppletBaseURLTest/testcases/AppletBaseURLTest.java:
> + * tests/jnlp_tests/simple/AppletBaseURLTest/resources/AppletBaseURLTest.html:
> + * tests/jnlp_tests/simple/AppletBaseURLTest/resources/AppletBaseURLTest.jnlp:
> + * tests/jnlp_tests/simple/AppletBaseURLTest/resources/AppletJNLPHrefBaseURLTest.html:
> + New reproducer that checks the URLS that document and codebase
> + point are correct.
> +
>

Noticed some mistakes/inconsistencies I made while writing another test:
  - Both browser tests used the jnlp_href HTML file. Changed the first one to use the applet-tag-only HTML file instead.
  - Moved @Bug annotation to the top of the class (instead of 3 times for each test).
  - JNLP file had an extra "</applet>" line at the end. This was copied over from AppletTest's JNLP, is this intentional Jiri?

Same ChangeLog as before!

Cheers,
Danesh
-------------- next part --------------
A non-text attachment was scrubbed...
Name: get-document-base-06.patch
Type: text/x-patch
Size: 15164 bytes
Desc: not available
Url : http://mail.openjdk.java.net/pipermail/distro-pkg-dev/attachments/20120611/0d6f8fbf/get-document-base-06.patch