Quantcast

[9] Review request for 8163889: [macosx] Can't print from browser on Mac OS X

classic Classic list List threaded Threaded
15 messages Options
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

[9] Review request for 8163889: [macosx] Can't print from browser on Mac OS X

Dmitry Markov
Hello,

Could you review a fix for jdk9, please?

        bug: https://bugs.openjdk.java.net/browse/JDK-8163889
        webrev: http://cr.openjdk.java.net/~dmarkov/8163889/webrev.00/

Problem description:
Several functions inside ImageSurfaceData.m, (e.g. customPixelsFromJava(), customPixelsToJava(), etc.) invoke getJNIEnv() from ThreadUtilities instead of usage corresponding input parameter. According to the design - getJNIEnv() should be executed on AppKit thread, but all code related to printing should NOT run on AppKit thread. So we get the following assert here - ‘CocoaAWT: Not running on AppKit thread 0 when expected.’

Also customPixelsFromJava() and customPixelsToJava() call OSXOffScreenSurfaceData.syncFromCustom() and OSXOffScreenSurfaceData.syncToCustom(), but these methods are absent at Java layer. So when we try to perform such JNI invocation we experience the crash.

Fix:
It is necessary to eliminate negative effects such as crashes, native exceptions and assert errors during printing.

Change summary:
 - Replaced invocation of getJNIEnv() with usage of corresponding input parameter.
 - Added method stubs to OSXOffScreenSurfaceData.

Thanks,
Dmitry
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: [9] Review request for 8163889: [macosx] Can't print from browser on Mac OS X

prasanta sadhukhan
Hi Dmitri,

Don't you need to implement syncFromCustom() and syncToCustom()?
If not, then it seems you do not need to call this functions from
customPixelsFromJava() and customPixelsToJava() in which case, then env
change/fix will not be needed.
Also, is it not possible to create a regression testcase?

Regards
Prasanta
On 1/14/2017 8:22 PM, Dmitry Markov wrote:

> Hello,
>
> Could you review a fix for jdk9, please?
>
> bug: https://bugs.openjdk.java.net/browse/JDK-8163889
> webrev: http://cr.openjdk.java.net/~dmarkov/8163889/webrev.00/
>
> Problem description:
> Several functions inside ImageSurfaceData.m, (e.g. customPixelsFromJava(), customPixelsToJava(), etc.) invoke getJNIEnv() from ThreadUtilities instead of usage corresponding input parameter. According to the design - getJNIEnv() should be executed on AppKit thread, but all code related to printing should NOT run on AppKit thread. So we get the following assert here - ‘CocoaAWT: Not running on AppKit thread 0 when expected.’
>
> Also customPixelsFromJava() and customPixelsToJava() call OSXOffScreenSurfaceData.syncFromCustom() and OSXOffScreenSurfaceData.syncToCustom(), but these methods are absent at Java layer. So when we try to perform such JNI invocation we experience the crash.
>
> Fix:
> It is necessary to eliminate negative effects such as crashes, native exceptions and assert errors during printing.
>
> Change summary:
>   - Replaced invocation of getJNIEnv() with usage of corresponding input parameter.
>   - Added method stubs to OSXOffScreenSurfaceData.
>
> Thanks,
> Dmitry

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: [9] Review request for 8163889: [macosx] Can't print from browser on Mac OS X

Dmitry Markov
Hi Prasanta,
Thank you for your feedback.

Actually there are two problems here: JDK-8163889 is devoted to the crash/assert error and JDK-8173050 which is about possible lack of support for the images with 'custom' type.
Currently I am trying to address the first problem, (i.e. eliminate crash and assert errors which take place during printing). So possible implementations of syncFromCustom(), syncToCustom(), etc.  are out of scope for this fix. They will be investigated under JDK-8173050.

It is possible to create manual regression test for this problem. Also the test will require some additional set up steps such as printer installation and so on. It seems to me that is overhead for person who runs it. However if you insist on test creation, I will add it.

Thanks,
Dmitry
On 16/01/2017 09:14, Prasanta Sadhukhan wrote:
Hi Dmitri,

Don't you need to implement syncFromCustom() and syncToCustom()?
If not, then it seems you do not need to call this functions from customPixelsFromJava() and customPixelsToJava() in which case, then env change/fix will not be needed.
Also, is it not possible to create a regression testcase?

Regards
Prasanta
On 1/14/2017 8:22 PM, Dmitry Markov wrote:
Hello,

Could you review a fix for jdk9, please?

    bug: https://bugs.openjdk.java.net/browse/JDK-8163889
    webrev: http://cr.openjdk.java.net/~dmarkov/8163889/webrev.00/

Problem description:
Several functions inside ImageSurfaceData.m, (e.g. customPixelsFromJava(), customPixelsToJava(), etc.) invoke getJNIEnv() from ThreadUtilities instead of usage corresponding input parameter. According to the design - getJNIEnv() should be executed on AppKit thread, but all code related to printing should NOT run on AppKit thread. So we get the following assert here - ‘CocoaAWT: Not running on AppKit thread 0 when expected.’

Also customPixelsFromJava() and customPixelsToJava() call OSXOffScreenSurfaceData.syncFromCustom() and OSXOffScreenSurfaceData.syncToCustom(), but these methods are absent at Java layer. So when we try to perform such JNI invocation we experience the crash.

Fix:
It is necessary to eliminate negative effects such as crashes, native exceptions and assert errors during printing.

Change summary:
  - Replaced invocation of getJNIEnv() with usage of corresponding input parameter.
  - Added method stubs to OSXOffScreenSurfaceData.

Thanks,
Dmitry


Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: [9] Review request for 8163889: [macosx] Can't print from browser on Mac OS X

prasanta sadhukhan

Hi Dmitri,

Ok.
I think it will be good to add a testcase (since bug already has a reproducible test) and since its a print problem, anyone who runs it has to install printer to verify so no excuse there :-)
or if you can find any other existing testcase which showcase this crash problem, you can append this bugid to that testcase.

Regards
Prasanta
On 1/19/2017 5:58 PM, dmitry markov wrote:
Hi Prasanta,
Thank you for your feedback.

Actually there are two problems here: JDK-8163889 is devoted to the crash/assert error and JDK-8173050 which is about possible lack of support for the images with 'custom' type.
Currently I am trying to address the first problem, (i.e. eliminate crash and assert errors which take place during printing). So possible implementations of syncFromCustom(), syncToCustom(), etc.  are out of scope for this fix. They will be investigated under JDK-8173050.

It is possible to create manual regression test for this problem. Also the test will require some additional set up steps such as printer installation and so on. It seems to me that is overhead for person who runs it. However if you insist on test creation, I will add it.

Thanks,
Dmitry
On 16/01/2017 09:14, Prasanta Sadhukhan wrote:
Hi Dmitri,

Don't you need to implement syncFromCustom() and syncToCustom()?
If not, then it seems you do not need to call this functions from customPixelsFromJava() and customPixelsToJava() in which case, then env change/fix will not be needed.
Also, is it not possible to create a regression testcase?

Regards
Prasanta
On 1/14/2017 8:22 PM, Dmitry Markov wrote:
Hello,

Could you review a fix for jdk9, please?

    bug: https://bugs.openjdk.java.net/browse/JDK-8163889
    webrev: http://cr.openjdk.java.net/~dmarkov/8163889/webrev.00/

Problem description:
Several functions inside ImageSurfaceData.m, (e.g. customPixelsFromJava(), customPixelsToJava(), etc.) invoke getJNIEnv() from ThreadUtilities instead of usage corresponding input parameter. According to the design - getJNIEnv() should be executed on AppKit thread, but all code related to printing should NOT run on AppKit thread. So we get the following assert here - ‘CocoaAWT: Not running on AppKit thread 0 when expected.’

Also customPixelsFromJava() and customPixelsToJava() call OSXOffScreenSurfaceData.syncFromCustom() and OSXOffScreenSurfaceData.syncToCustom(), but these methods are absent at Java layer. So when we try to perform such JNI invocation we experience the crash.

Fix:
It is necessary to eliminate negative effects such as crashes, native exceptions and assert errors during printing.

Change summary:
  - Replaced invocation of getJNIEnv() with usage of corresponding input parameter.
  - Added method stubs to OSXOffScreenSurfaceData.

Thanks,
Dmitry



Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: [9] Review request for 8163889: [macosx] Can't print from browser on Mac OS X

Philip Race
I haven't looked at the fix (yet) but I definitely agree that a manual
regression test
for this  is better than none. What else should we do ? Just not test
printing ?

In my view which I've expressed to SQE for a really long time, if you
aren't testing with
printers installed you aren't testing the whole platform. Whilst it may
be convenient
that tests (silently) don't complain when there are no printers, it is a
slippery slope ..

-phil.

On 01/20/2017 04:04 AM, Prasanta Sadhukhan wrote:
>
> It is possible to create manual regression test for this problem. Also
> the test will require some additional set up steps such as printer
> installation and so on. It seems to me that is overhead for person who
> runs it. However if you insist on test creation, I will add it.

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: [9] Review request for 8163889: [macosx] Can't print from browser on Mac OS X

Dmitry Markov
Hi Phil, Prasanta,

I have updated the fix as you suggested, (i.e. added the regression test). The new webrev is located at http://cr.openjdk.java.net/~dmarkov/8163889/webrev.01/
Could you review the new version, please?

Thanks,
Dmitry

> On 20 Jan 2017, at 19:50, Phil Race <[hidden email]> wrote:
>
> I haven't looked at the fix (yet) but I definitely agree that a manual regression test
> for this  is better than none. What else should we do ? Just not test printing ?
>
> In my view which I've expressed to SQE for a really long time, if you aren't testing with
> printers installed you aren't testing the whole platform. Whilst it may be convenient
> that tests (silently) don't complain when there are no printers, it is a slippery slope ..
>
> -phil.
>
> On 01/20/2017 04:04 AM, Prasanta Sadhukhan wrote:
>>
>> It is possible to create manual regression test for this problem. Also the test will require some additional set up steps such as printer installation and so on. It seems to me that is overhead for person who runs it. However if you insist on test creation, I will add it.
>

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: [9] Review request for 8163889: [macosx] Can't print from browser on Mac OS X

Philip Race
Hi Dmitry,
> 29 * @run main/othervm PrintCrashTest
why othervm ?

I don't think that is strictly necessary just because you are using deleteOnExit.
And FWIW I think the test could "more promptly" delete the file anyway after print returns.

-phil.


On 1/20/17, 9:36 AM, Dmitry Markov wrote:
Hi Phil, Prasanta,

I have updated the fix as you suggested, (i.e. added the regression test). The new webrev is located at http://cr.openjdk.java.net/~dmarkov/8163889/webrev.01/
Could you review the new version, please?

Thanks,
Dmitry
On 20 Jan 2017, at 19:50, Phil Race [hidden email] wrote:

I haven't looked at the fix (yet) but I definitely agree that a manual regression test
for this  is better than none. What else should we do ? Just not test printing ?

In my view which I've expressed to SQE for a really long time, if you aren't testing with
printers installed you aren't testing the whole platform. Whilst it may be convenient
that tests (silently) don't complain when there are no printers, it is a slippery slope ..

-phil.

On 01/20/2017 04:04 AM, Prasanta Sadhukhan wrote:
It is possible to create manual regression test for this problem. Also the test will require some additional set up steps such as printer installation and so on. It seems to me that is overhead for person who runs it. However if you insist on test creation, I will add it.

      

    
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: [9] Review request for 8163889: [macosx] Can't print from browser on Mac OS X

Dmitry Markov
Hi Phil,

I agree ‘othervm’ is not necessary here. That is ‘copy-paste’ error. 
Also I updated the part related to the file deletion based on your suggestion.

Thanks,
Dmitry 
On 21 Jan 2017, at 00:24, Philip Race <[hidden email]> wrote:

Hi Dmitry,
> 29 * @run main/othervm PrintCrashTest
why othervm ?

I don't think that is strictly necessary just because you are using deleteOnExit.
And FWIW I think the test could "more promptly" delete the file anyway after print returns.

-phil.


On 1/20/17, 9:36 AM, Dmitry Markov wrote:
Hi Phil, Prasanta,

I have updated the fix as you suggested, (i.e. added the regression test). The new webrev is located at http://cr.openjdk.java.net/~dmarkov/8163889/webrev.01/
Could you review the new version, please?

Thanks,
Dmitry
On 20 Jan 2017, at 19:50, Phil Race [hidden email] wrote:

I haven't looked at the fix (yet) but I definitely agree that a manual regression test
for this  is better than none. What else should we do ? Just not test printing ?

In my view which I've expressed to SQE for a really long time, if you aren't testing with
printers installed you aren't testing the whole platform. Whilst it may be convenient
that tests (silently) don't complain when there are no printers, it is a slippery slope ..

-phil.

On 01/20/2017 04:04 AM, Prasanta Sadhukhan wrote:
It is possible to create manual regression test for this problem. Also the test will require some additional set up steps such as printer installation and so on. It seems to me that is overhead for person who runs it. However if you insist on test creation, I will add it.

      

    

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: [9] Review request for 8163889: [macosx] Can't print from browser on Mac OS X

Philip Race
+1

-phil

On 1/21/17, 11:05 AM, Dmitry Markov wrote:
Hi Phil,

I agree ‘othervm’ is not necessary here. That is ‘copy-paste’ error. 
Also I updated the part related to the file deletion based on your suggestion.

Thanks,
Dmitry 
On 21 Jan 2017, at 00:24, Philip Race <[hidden email]> wrote:

Hi Dmitry,
> 29 * @run main/othervm PrintCrashTest
why othervm ?

I don't think that is strictly necessary just because you are using deleteOnExit.
And FWIW I think the test could "more promptly" delete the file anyway after print returns.

-phil.


On 1/20/17, 9:36 AM, Dmitry Markov wrote:
Hi Phil, Prasanta,

I have updated the fix as you suggested, (i.e. added the regression test). The new webrev is located at http://cr.openjdk.java.net/~dmarkov/8163889/webrev.01/
Could you review the new version, please?

Thanks,
Dmitry
On 20 Jan 2017, at 19:50, Phil Race [hidden email] wrote:

I haven't looked at the fix (yet) but I definitely agree that a manual regression test
for this  is better than none. What else should we do ? Just not test printing ?

In my view which I've expressed to SQE for a really long time, if you aren't testing with
printers installed you aren't testing the whole platform. Whilst it may be convenient
that tests (silently) don't complain when there are no printers, it is a slippery slope ..

-phil.

On 01/20/2017 04:04 AM, Prasanta Sadhukhan wrote:
It is possible to create manual regression test for this problem. Also the test will require some additional set up steps such as printer installation and so on. It seems to me that is overhead for person who runs it. However if you insist on test creation, I will add it.

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: [9] Review request for 8163889: [macosx] Can't print from browser on Mac OS X

Dmitry Markov
Thank you very much, Phil!
Looking for the second +1 from someone else.

Dmitry
On 21 Jan 2017, at 22:39, Philip Race <[hidden email]> wrote:

+1

-phil

On 1/21/17, 11:05 AM, Dmitry Markov wrote:
Hi Phil,

I agree ‘othervm’ is not necessary here. That is ‘copy-paste’ error. 
Also I updated the part related to the file deletion based on your suggestion.

Thanks,
Dmitry 
On 21 Jan 2017, at 00:24, Philip Race <[hidden email]> wrote:

Hi Dmitry,
> 29 * @run main/othervm PrintCrashTest
why othervm ?

I don't think that is strictly necessary just because you are using deleteOnExit.
And FWIW I think the test could "more promptly" delete the file anyway after print returns.

-phil.


On 1/20/17, 9:36 AM, Dmitry Markov wrote:
Hi Phil, Prasanta,

I have updated the fix as you suggested, (i.e. added the regression test). The new webrev is located at http://cr.openjdk.java.net/~dmarkov/8163889/webrev.01/
Could you review the new version, please?

Thanks,
Dmitry
On 20 Jan 2017, at 19:50, Phil Race [hidden email] wrote:

I haven't looked at the fix (yet) but I definitely agree that a manual regression test
for this  is better than none. What else should we do ? Just not test printing ?

In my view which I've expressed to SQE for a really long time, if you aren't testing with
printers installed you aren't testing the whole platform. Whilst it may be convenient
that tests (silently) don't complain when there are no printers, it is a slippery slope ..

-phil.

On 01/20/2017 04:04 AM, Prasanta Sadhukhan wrote:
It is possible to create manual regression test for this problem. Also the test will require some additional set up steps such as printer installation and so on. It seems to me that is overhead for person who runs it. However if you insist on test creation, I will add it.


Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: [9] Review request for 8163889: [macosx] Can't print from browser on Mac OS X

prasanta sadhukhan
In reply to this post by Dmitry Markov

Hi Dmitry,

Any particular reason why in imageDataProvider_UnholdJavaPixels(), we are not doing

isdo->pixels = null; which we do in unholdJavaPixels()

since in holdJavaPixels() , we are doing
967     else if (isdo->pixels == NULL)
 968     {
 969         isdo->pixels = (Pixel8bit*)((*env)->GetDirectBufferAddress(env, isdo->array));
 970     }

Also, in the test, I guess there's no point catching Exception and rethrowing RuntimeException since we are not doing any processing in catch block. We can just add throws Exception in main() without this catch block and have try-finally.
Also, I guess we do not follow adding "author" in the new test anymore.

Regards
Prasanta
On 1/22/2017 12:35 AM, Dmitry Markov wrote:
Hi Phil,

I agree ‘othervm’ is not necessary here. That is ‘copy-paste’ error. 
Also I updated the part related to the file deletion based on your suggestion.

Thanks,
Dmitry 
On 21 Jan 2017, at 00:24, Philip Race <[hidden email]> wrote:

Hi Dmitry,
> 29 * @run main/othervm PrintCrashTest
why othervm ?

I don't think that is strictly necessary just because you are using deleteOnExit.
And FWIW I think the test could "more promptly" delete the file anyway after print returns.

-phil.


On 1/20/17, 9:36 AM, Dmitry Markov wrote:
Hi Phil, Prasanta,

I have updated the fix as you suggested, (i.e. added the regression test). The new webrev is located at http://cr.openjdk.java.net/~dmarkov/8163889/webrev.01/
Could you review the new version, please?

Thanks,
Dmitry
On 20 Jan 2017, at 19:50, Phil Race [hidden email] wrote:

I haven't looked at the fix (yet) but I definitely agree that a manual regression test
for this  is better than none. What else should we do ? Just not test printing ?

In my view which I've expressed to SQE for a really long time, if you aren't testing with
printers installed you aren't testing the whole platform. Whilst it may be convenient
that tests (silently) don't complain when there are no printers, it is a slippery slope ..

-phil.

On 01/20/2017 04:04 AM, Prasanta Sadhukhan wrote:
It is possible to create manual regression test for this problem. Also the test will require some additional set up steps such as printer installation and so on. It seems to me that is overhead for person who runs it. However if you insist on test creation, I will add it.


Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: [9] Review request for 8163889: [macosx] Can't print from browser on Mac OS X

Dmitry Markov
Hi Prasanta,

As far as I know imageDataProvider_UnholdJavaPixels() is only invoked for images with ‘custom’ type. We read the data directly from Java memory for such images. 
At the same time for other image types we use Get/ReleasePrimitiveArrayCritical() and copy the data from Java to native memory.
I guess that’s the main reason why we don’t set isdo->pixels to null inside imageDataProvider_UnholdJavaPixels().

Also I updated the test based on your suggestions. Please find the new version here: http://cr.openjdk.java.net/~dmarkov/8163889/webrev.03/

Thanks,
Dmitry 

On 23 Jan 2017, at 09:17, Prasanta Sadhukhan <[hidden email]> wrote:

Hi Dmitry,

Any particular reason why in imageDataProvider_UnholdJavaPixels(), we are not doing

isdo->pixels = null; which we do in unholdJavaPixels()

since in holdJavaPixels() , we are doing
967     else if (isdo->pixels == NULL)
 968     {
 969         isdo->pixels = (Pixel8bit*)((*env)->GetDirectBufferAddress(env, isdo->array));
 970     }

Also, in the test, I guess there's no point catching Exception and rethrowing RuntimeException since we are not doing any processing in catch block. We can just add throws Exception in main() without this catch block and have try-finally.
Also, I guess we do not follow adding "author" in the new test anymore.

Regards
Prasanta
On 1/22/2017 12:35 AM, Dmitry Markov wrote:
Hi Phil,

I agree ‘othervm’ is not necessary here. That is ‘copy-paste’ error. 
Also I updated the part related to the file deletion based on your suggestion.

Thanks,
Dmitry 
On 21 Jan 2017, at 00:24, Philip Race <[hidden email]> wrote:

Hi Dmitry,
> 29 * @run main/othervm PrintCrashTest
why othervm ?

I don't think that is strictly necessary just because you are using deleteOnExit.
And FWIW I think the test could "more promptly" delete the file anyway after print returns.

-phil.


On 1/20/17, 9:36 AM, Dmitry Markov wrote:
Hi Phil, Prasanta,

I have updated the fix as you suggested, (i.e. added the regression test). The new webrev is located at http://cr.openjdk.java.net/~dmarkov/8163889/webrev.01/
Could you review the new version, please?

Thanks,
Dmitry
On 20 Jan 2017, at 19:50, Phil Race [hidden email] wrote:

I haven't looked at the fix (yet) but I definitely agree that a manual regression test
for this  is better than none. What else should we do ? Just not test printing ?

In my view which I've expressed to SQE for a really long time, if you aren't testing with
printers installed you aren't testing the whole platform. Whilst it may be convenient
that tests (silently) don't complain when there are no printers, it is a slippery slope ..

-phil.

On 01/20/2017 04:04 AM, Prasanta Sadhukhan wrote:
It is possible to create manual regression test for this problem. Also the test will require some additional set up steps such as printer installation and so on. It seems to me that is overhead for person who runs it. However if you insist on test creation, I will add it.



Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: [9] Review request for 8163889: [macosx] Can't print from browser on Mac OS X

prasanta sadhukhan

Hi Dmitri,


On 1/23/2017 11:52 PM, Dmitry Markov wrote:
Hi Prasanta,

As far as I know imageDataProvider_UnholdJavaPixels() is only invoked for images with ‘custom’ type. We read the data directly from Java memory for such images.
I understand that but my question was coming from the fact that, if we are going to print more than 1 "custom" image, then isdo->pixels will point to the same "direct buffer address" (which we obtained for the first custom image) since isdo->pixels will not be NULL (so line 969 will not be executed), whereas ideally, isdo->pixels should obtain the data again. Am I misinterpreting something?
At the same time for other image types we use Get/ReleasePrimitiveArrayCritical() and copy the data from Java to native memory.
I guess that’s the main reason why we don’t set isdo->pixels to null inside imageDataProvider_UnholdJavaPixels().

Also I updated the test based on your suggestions. Please find the new version here: http://cr.openjdk.java.net/~dmarkov/8163889/webrev.03/
test looks good.

Regards
Prasanta

Thanks,
Dmitry 

On 23 Jan 2017, at 09:17, Prasanta Sadhukhan <[hidden email]> wrote:

Hi Dmitry,

Any particular reason why in imageDataProvider_UnholdJavaPixels(), we are not doing

isdo->pixels = null; which we do in unholdJavaPixels()

since in holdJavaPixels() , we are doing
967     else if (isdo->pixels == NULL)
 968     {
 969         isdo->pixels = (Pixel8bit*)((*env)->GetDirectBufferAddress(env, isdo->array));
 970     }

Also, in the test, I guess there's no point catching Exception and rethrowing RuntimeException since we are not doing any processing in catch block. We can just add throws Exception in main() without this catch block and have try-finally.
Also, I guess we do not follow adding "author" in the new test anymore.

Regards
Prasanta
On 1/22/2017 12:35 AM, Dmitry Markov wrote:
Hi Phil,

I agree ‘othervm’ is not necessary here. That is ‘copy-paste’ error. 
Also I updated the part related to the file deletion based on your suggestion.

Thanks,
Dmitry 
On 21 Jan 2017, at 00:24, Philip Race <[hidden email]> wrote:

Hi Dmitry,
> 29 * @run main/othervm PrintCrashTest
why othervm ?

I don't think that is strictly necessary just because you are using deleteOnExit.
And FWIW I think the test could "more promptly" delete the file anyway after print returns.

-phil.


On 1/20/17, 9:36 AM, Dmitry Markov wrote:
Hi Phil, Prasanta,

I have updated the fix as you suggested, (i.e. added the regression test). The new webrev is located at http://cr.openjdk.java.net/~dmarkov/8163889/webrev.01/
Could you review the new version, please?

Thanks,
Dmitry
On 20 Jan 2017, at 19:50, Phil Race [hidden email] wrote:

I haven't looked at the fix (yet) but I definitely agree that a manual regression test
for this  is better than none. What else should we do ? Just not test printing ?

In my view which I've expressed to SQE for a really long time, if you aren't testing with
printers installed you aren't testing the whole platform. Whilst it may be convenient
that tests (silently) don't complain when there are no printers, it is a slippery slope ..

-phil.

On 01/20/2017 04:04 AM, Prasanta Sadhukhan wrote:
It is possible to create manual regression test for this problem. Also the test will require some additional set up steps such as printer installation and so on. It seems to me that is overhead for person who runs it. However if you insist on test creation, I will add it.




Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: [9] Review request for 8163889: [macosx] Can't print from browser on Mac OS X

Dmitry Markov
Hi Prasanta,

Please find my answer inline.

Thanks,
Dmitry
On 24/01/2017 08:29, Prasanta Sadhukhan wrote:

Hi Dmitri,


On 1/23/2017 11:52 PM, Dmitry Markov wrote:
Hi Prasanta,

As far as I know imageDataProvider_UnholdJavaPixels() is only invoked for images with ‘custom’ type. We read the data directly from Java memory for such images.
I understand that but my question was coming from the fact that, if we are going to print more than 1 "custom" image, then isdo->pixels will point to the same "direct buffer address" (which we obtained for the first custom image) since isdo->pixels will not be NULL (so line 969 will not be executed), whereas ideally, isdo->pixels should obtain the data again. Am I misinterpreting something?
Actually we initialize isdo structure based on data from Java layer anytime when an image is printed, see LockImage() function. So isdo->pixels will be correctly initialized inside holdJavaPixels() for every "custom" image. I am sorry, but the situation you described above is not possible here.
At the same time for other image types we use Get/ReleasePrimitiveArrayCritical() and copy the data from Java to native memory.
I guess that’s the main reason why we don’t set isdo->pixels to null inside imageDataProvider_UnholdJavaPixels().

Also I updated the test based on your suggestions. Please find the new version here: http://cr.openjdk.java.net/~dmarkov/8163889/webrev.03/
test looks good.

Regards
Prasanta

Thanks,
Dmitry 

On 23 Jan 2017, at 09:17, Prasanta Sadhukhan <[hidden email]> wrote:

Hi Dmitry,

Any particular reason why in imageDataProvider_UnholdJavaPixels(), we are not doing

isdo->pixels = null; which we do in unholdJavaPixels()

since in holdJavaPixels() , we are doing
967     else if (isdo->pixels == NULL)
 968     {
 969         isdo->pixels = (Pixel8bit*)((*env)->GetDirectBufferAddress(env, isdo->array));
 970     }

Also, in the test, I guess there's no point catching Exception and rethrowing RuntimeException since we are not doing any processing in catch block. We can just add throws Exception in main() without this catch block and have try-finally.
Also, I guess we do not follow adding "author" in the new test anymore.

Regards
Prasanta
On 1/22/2017 12:35 AM, Dmitry Markov wrote:
Hi Phil,

I agree ‘othervm’ is not necessary here. That is ‘copy-paste’ error. 
Also I updated the part related to the file deletion based on your suggestion.

Thanks,
Dmitry 
On 21 Jan 2017, at 00:24, Philip Race <[hidden email]> wrote:

Hi Dmitry,
> 29 * @run main/othervm PrintCrashTest
why othervm ?

I don't think that is strictly necessary just because you are using deleteOnExit.
And FWIW I think the test could "more promptly" delete the file anyway after print returns.

-phil.


On 1/20/17, 9:36 AM, Dmitry Markov wrote:
Hi Phil, Prasanta,

I have updated the fix as you suggested, (i.e. added the regression test). The new webrev is located at http://cr.openjdk.java.net/~dmarkov/8163889/webrev.01/
Could you review the new version, please?

Thanks,
Dmitry
On 20 Jan 2017, at 19:50, Phil Race [hidden email] wrote:

I haven't looked at the fix (yet) but I definitely agree that a manual regression test
for this  is better than none. What else should we do ? Just not test printing ?

In my view which I've expressed to SQE for a really long time, if you aren't testing with
printers installed you aren't testing the whole platform. Whilst it may be convenient
that tests (silently) don't complain when there are no printers, it is a slippery slope ..

-phil.

On 01/20/2017 04:04 AM, Prasanta Sadhukhan wrote:
It is possible to create manual regression test for this problem. Also the test will require some additional set up steps such as printer installation and so on. It seems to me that is overhead for person who runs it. However if you insist on test creation, I will add it.





Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: [9] Review request for 8163889: [macosx] Can't print from browser on Mac OS X

prasanta sadhukhan

ok. +1

Regards
Prasanta
On 1/24/2017 12:04 PM, dmitry markov wrote:
Hi Prasanta,

Please find my answer inline.

Thanks,
Dmitry
On 24/01/2017 08:29, Prasanta Sadhukhan wrote:

Hi Dmitri,


On 1/23/2017 11:52 PM, Dmitry Markov wrote:
Hi Prasanta,

As far as I know imageDataProvider_UnholdJavaPixels() is only invoked for images with ‘custom’ type. We read the data directly from Java memory for such images.
I understand that but my question was coming from the fact that, if we are going to print more than 1 "custom" image, then isdo->pixels will point to the same "direct buffer address" (which we obtained for the first custom image) since isdo->pixels will not be NULL (so line 969 will not be executed), whereas ideally, isdo->pixels should obtain the data again. Am I misinterpreting something?
Actually we initialize isdo structure based on data from Java layer anytime when an image is printed, see LockImage() function. So isdo->pixels will be correctly initialized inside holdJavaPixels() for every "custom" image. I am sorry, but the situation you described above is not possible here.
At the same time for other image types we use Get/ReleasePrimitiveArrayCritical() and copy the data from Java to native memory.
I guess that’s the main reason why we don’t set isdo->pixels to null inside imageDataProvider_UnholdJavaPixels().

Also I updated the test based on your suggestions. Please find the new version here: http://cr.openjdk.java.net/~dmarkov/8163889/webrev.03/
test looks good.

Regards
Prasanta

Thanks,
Dmitry 

On 23 Jan 2017, at 09:17, Prasanta Sadhukhan <[hidden email]> wrote:

Hi Dmitry,

Any particular reason why in imageDataProvider_UnholdJavaPixels(), we are not doing

isdo->pixels = null; which we do in unholdJavaPixels()

since in holdJavaPixels() , we are doing
967     else if (isdo->pixels == NULL)
 968     {
 969         isdo->pixels = (Pixel8bit*)((*env)->GetDirectBufferAddress(env, isdo->array));
 970     }

Also, in the test, I guess there's no point catching Exception and rethrowing RuntimeException since we are not doing any processing in catch block. We can just add throws Exception in main() without this catch block and have try-finally.
Also, I guess we do not follow adding "author" in the new test anymore.

Regards
Prasanta
On 1/22/2017 12:35 AM, Dmitry Markov wrote:
Hi Phil,

I agree ‘othervm’ is not necessary here. That is ‘copy-paste’ error. 
Also I updated the part related to the file deletion based on your suggestion.

Thanks,
Dmitry 
On 21 Jan 2017, at 00:24, Philip Race <[hidden email]> wrote:

Hi Dmitry,
> 29 * @run main/othervm PrintCrashTest
why othervm ?

I don't think that is strictly necessary just because you are using deleteOnExit.
And FWIW I think the test could "more promptly" delete the file anyway after print returns.

-phil.


On 1/20/17, 9:36 AM, Dmitry Markov wrote:
Hi Phil, Prasanta,

I have updated the fix as you suggested, (i.e. added the regression test). The new webrev is located at http://cr.openjdk.java.net/~dmarkov/8163889/webrev.01/
Could you review the new version, please?

Thanks,
Dmitry
On 20 Jan 2017, at 19:50, Phil Race [hidden email] wrote:

I haven't looked at the fix (yet) but I definitely agree that a manual regression test
for this  is better than none. What else should we do ? Just not test printing ?

In my view which I've expressed to SQE for a really long time, if you aren't testing with
printers installed you aren't testing the whole platform. Whilst it may be convenient
that tests (silently) don't complain when there are no printers, it is a slippery slope ..

-phil.

On 01/20/2017 04:04 AM, Prasanta Sadhukhan wrote:
It is possible to create manual regression test for this problem. Also the test will require some additional set up steps such as printer installation and so on. It seems to me that is overhead for person who runs it. However if you insist on test creation, I will add it.






Loading...