JDK-8159614 : Can't get file size with javascript
  • Type: Bug
  • Component: javafx
  • Sub-Component: web
  • Affected Version: 8u112,9
  • Priority: P3
  • Status: Closed
  • Resolution: Fixed
  • OS: generic
  • CPU: generic
  • Submitted: 2016-06-15
  • Updated: 2017-09-11
  • Resolved: 2016-07-01
The Version table provides details related to the release that this issue/RFE will be addressed.

Unresolved : Release in which this issue/RFE will be addressed.
Resolved: Release in which this issue/RFE has been resolved.
Fixed : Release in which this issue/RFE has been fixed. The release containing this fix may be available for download as an Early Access Release or a General Availability Release.

To download the current JDK release, click here.
JDK 8 JDK 9
8u112Fixed 9 masterFixed
Description
Testsuite name: FXWebNodeManualTests 
Test name(s): com/sun/fx/webnode/tests/html5/input/file/FileChooser.java#simpleSingle.html
Product(s) tested: JDK8u112b01 (64bit) 
OS/architecture: ubuntu16.04-x64 
Reproducible: Always 
Is it a Regression: yes (Last checked working version 8u102)
Is it a platform specific issue: No
The same jdk on Win10-x64: Failed
The same jdk on Mac10.11-x64: Failed

Instructions: 
1. Execute sample, click "Open file" and choose any file.
Expected: filename and file size displayed on the page.
Actual result: filename is displayed, but filesize is always 0.
Comments
There's the same issue happened on Ubuntu16.04-x64 with jdk9b125-64bit
01-07-2016
There's the same issue happened on Ubuntu16.04-x64 with jdk9b125-64bit RULE "com/sun/fx/webnode/tests/html5/input/file/FileChooser.java#simpleMultiple" any any RULE "com/sun/fx/webnode/tests/html5/input/file/FileChooser.java#simpleSingle" any any
01-07-2016
changeset: faaa627074f2 user: asrivastava date: Fri Jul 01 11:52:16 2016 +0530 Reviewed-by: arajkumar, kcr URL: http://hg.openjdk.java.net/openjfx/9-dev/rt/rev/faaa627074f2
01-07-2016
As the above mentioned change is a minor one, I have included that in the change set patch itself.
01-07-2016
+1 Minor Comment: is it good to declare JLocalRef<jlongArray> lArray(env->NewLongArray(3)); after the ASSERT(mid); statement? Though lArray is LocalRef and no harm, i think it is better to declare after ASSERT (in debug build) as we need not call NewLongArray(3) in case ASSERT executes. JNIEnv* env = WebCore_GetJavaEnv(); JLocalRef<jlongArray> lArray(env->NewLongArray(3)); static jmethodID mid = env->GetStaticMethodID( GetFileSystemClass(env), "fwkGetFileMetadata", "(Ljava/lang/String;[J)Z"); ASSERT(mid);
30-06-2016
lgtm +1
30-06-2016
+1
30-06-2016
To get a manual test case, I tried the following : 1) Changing file name, once in file chooser I have highlighted the file. 2) Deleting the file, once in file chooser I have highlighted the file. In both of the cases I couldn't hit the specified code. So I have removed that piece of code. http://cr.openjdk.java.net/~asrivastava/8159614/webrev.06/
29-06-2016
modules/web/src/main/native/Source/WebCore/platform/java/FileSystemJava.cpp: + if (metadataResults[0] == invalidFileTime() || (metadataResults[1] < 0)) { + return false; + } Do you have the manual test case to hit the above code? Also ReleaseLongArrayElements is not called on this failure path. modules/web/src/main/java/com/sun/webkit/FileSystem.java: + metadataArray[0] = file.lastModified() / TO_SECONDS; Don't converting to seconds and assign to integer type variable. You will lose precision. Don't it on native.
29-06-2016
Please review: http://cr.openjdk.java.net/~asrivastava/8159614/webrev.05/ Now returning modified time in seconds to WebCore. Added code to take care of leak issues.
29-06-2016
+ jlong* metadataResults = env->GetLongArrayElements(lArray, 0); ReleaseLongArrayElements is missing. You could also use GetPrimitiveArrayCritical/ReleasePrimitiveArrayCritical functions. + metadata.modificationTime = metadataResults[0]; + metadata.length = metadataResults[1]; modificationTime should be in seconds. Don't directly assign the result of File::lastModified(), it is in milliseconds. + FileMetadata metadata ; + if (getFileMetadata(path, metadata)) { + result = metadata.modificationTime / 1000; + return true; You can remove the second conversion once you are done with above mentioned change.
28-06-2016
Please review: http://cr.openjdk.java.net/~asrivastava/8159614/webrev.04/ Incorporated Arun's and murali's comments
28-06-2016
In FileSystemJava.cpp, function getFileModificationTime() : There is a extra space before return false; if (fileModificationTime > 0) { 92 result = fileModificationTime / 1000; 93 return true; 94 } else { 95 return false; 96 }
28-06-2016
+ jlongArray lArray = (env)->NewLongArray(3); You are leaking lArray object. Make use of smart point JLocalRef from JavaRef.h. Refer ImageSourceJava::createFrameAtIndex method for more details. -bool getFileModificationTime(const String&, time_t& result) +bool getFileModificationTime(const String& path, time_t& result) I suggest to implement getFileModificationTime using getFileMetadata. It will remove fwkGetFileModificationTime implementation from java and redundant code to get modification time. GTK WebKit does similar to that.
28-06-2016
+1
24-06-2016
Please review: http://cr.openjdk.java.net/~asrivastava/8159614/webrev.03/ Incorporated Kevin's review comment.
24-06-2016
Other than a couple formatting issues, it looks good now. FileSystem.java 1. Formatting + scoping: public final static int TYPE_* The style guidelines are to put "static" before "final". Also, these constants can and should be private. So: private static final int TYPE_* 2. Minor spacing issues: Remove space between 'long' and '[]' in: long [] metadataArray Remove extra space (there are two) in: "return false;" FileSystemJava.cpp 3. Minor spacing issue: Remove extra space (there are two) before equals sign in: jlong* metadataResults = env->GetLongArrayElements(lArray, 0); +1 pending the above fixes
23-06-2016
Please review : http://cr.openjdk.java.net/~asrivastava/8159614/webrev.02/ Incorporated Kevin's review comments.
22-06-2016
Overall looks like a good approach. I haven't tested it yet, but here are my comments: 1. Since none of the three fields returned by the fwkGetMetaData call are floating point it would be cleaner to use a long[] array (jlong on the native side) rather than double. This would avoid the call to ceil (which shouldn't be needed anyway). 2. Where are the enum types for file type defined? You might want to declare them as "private static final" constants at the top of FileSystem.java with a comment that the values must match the native definitions. 3. Minor suggestion: In fwkGetFileMetadata you could factor the "return true;" out of the three places in the if..else if...else block and move it below the block. 4. Minor formatting issue: you are missing a space after the comma to separate the arguments in a couple places in the native code. For example: 171 (jstring)path.toJavaString(env),dArray);
21-06-2016
Please review : http://cr.openjdk.java.net/~asrivastava/8159614/webrev.01/ Incorporated Arun's review comment
21-06-2016
+ metadata.length = fileSize; + metadata.modificationTime = fileModificationTime; FileMetaData type has 3 attributes: size, modificationTime, and type. Could you fill type value as well? Looks at FileSystemGtk.cpp for reference. Also instead of doing 3 different JNI upcalls, add a method named fwkGetMetaData to class FileSystem.java and return all 3 attributes.
17-06-2016
Review Comments: Few Minor Issues: FileSystem.java Please introduce empty line before declaration of private static long fwkGetFileModificationTime(String path) There is an extra space before the function declaration: private static long fwkGetFileModificationTime(String path) FileSystemJava.cpp Please introduce empty line after the statement CheckAndClearException(env);
17-06-2016
http://cr.openjdk.java.net/~asrivastava/8159614/webrev.00/ Solution: Implemented metadata functionality to get the correct file size in bytes. Additional changes: Found that File lastModifiedDate attributes is also getting only current time value. Hence implemented functions to get file last modified date. Tested on 9dev on all three platforms, works fine.
17-06-2016
There's the same issue happened on Win7-x64 with jdk8u112b01-64bit RULE "com/sun/fx/webnode/tests/html5/input/file/FileChooser.java#simpleMultiple" any any RULE "com/sun/fx/webnode/tests/html5/input/file/FileChooser.java#simpleSingle" any any
16-06-2016
Metadata implementation is missing for Java specific code
16-06-2016
Confirmed.
15-06-2016
There's the same issue happened on Mac10.11-x64 with jdk8u112b01-64bit RULE "com/sun/fx/webnode/tests/html5/input/file/FileChooser.java#simpleMultiple" any any RULE "com/sun/fx/webnode/tests/html5/input/file/FileChooser.java#simpleSingle" any any
15-06-2016
RULE "com/sun/fx/webnode/tests/html5/input/file/FileChooser.java#simpleSingle" any any RULE "com/sun/fx/webnode/tests/html5/input/file/FileChooser.java#simpleMultiple" any any
15-06-2016