Issue 29424615: Issue 4231 - Fix unstable FilterEngineTest.testSetRemoveFilterChangeCallback (Closed)
|
Created:
April 28, 2017, 8:24 a.m. by anton Modified:
Oct. 29, 2018, 5:39 p.m. CC:
René Jeschke Visibility:
Public. |
DescriptionIssue 4231 - Fix unstable FilterEngineTest.testSetRemoveFilterChangeCallback
This is a new code review instead of old one (https://codereview.adblockplus.org/29347315/) - 9 months passed
Patch Set 1 #Patch Set 2 : fixed typo #
Total comments: 1
Patch Set 3 : using method from c++ utils #
Total comments: 33
Patch Set 4 : now using smart_ptr, changed impl for reading file as string, minor changes #
Total comments: 7
Patch Set 5 : changed impl for reading file as bytes array, modified test. AndroidFileSystem now does not resolve… #
Total comments: 8
MessagesTotal messages: 17
I would recommend to firstly finish with updating of libadblockplus. It will eliminate some changes made here what will make this codereview a bit simpler.
On 2017/04/28 09:12:09, sergei wrote: > I would recommend to firstly finish with updating of libadblockplus. It will > eliminate some changes made here what will make this codereview a bit simpler. Oops, sorry, those changes what I expected in libadblockplus are not done yet.
I have not taken a look on the rest yet. https://codereview.adblockplus.org/29424615/diff/29424631/libadblockplus-andr... File libadblockplus-android/jni/Utils.cpp (right): https://codereview.adblockplus.org/29424615/diff/29424631/libadblockplus-andr... libadblockplus-android/jni/Utils.cpp:103: std::string JniStdStreamToStdString(std::istream *in) I think it would be better to call the function from libadblockplus instead of having own implementation. https://github.com/adblockplus/libadblockplus/blob/53211850e1dba0d3ba6613dfe9... So far, you can simply declare it in the header as namespace Utils { std::string Slurp(std::istream& stream); } and call if you statically link with libadblockplus.
On 2017/04/28 09:24:49, sergei wrote: > I have not taken a look on the rest yet. > > https://codereview.adblockplus.org/29424615/diff/29424631/libadblockplus-andr... > File libadblockplus-android/jni/Utils.cpp (right): > > https://codereview.adblockplus.org/29424615/diff/29424631/libadblockplus-andr... > libadblockplus-android/jni/Utils.cpp:103: std::string > JniStdStreamToStdString(std::istream *in) > I think it would be better to call the function from libadblockplus instead of > having own implementation. > https://github.com/adblockplus/libadblockplus/blob/53211850e1dba0d3ba6613dfe9... > > So far, you can simply declare it in the header as > namespace Utils { > std::string Slurp(std::istream& stream); > } > and call if you statically link with libadblockplus. no problem, see updated patch set
https://codereview.adblockplus.org/29424615/diff/29424666/libadblockplus-andr... File libadblockplus-android/src/org/adblockplus/libadblockplus/FileSystem.java (right): https://codereview.adblockplus.org/29424615/diff/29424666/libadblockplus-andr... libadblockplus-android/src/org/adblockplus/libadblockplus/FileSystem.java:123: private final static class DisposeWrapper implements Disposable By our code style, the modifier order should be 'static final' https://codereview.adblockplus.org/29424615/diff/29424666/libadblockplus-andr... libadblockplus-android/src/org/adblockplus/libadblockplus/FileSystem.java:139: private final static native void registerNatives(); Same here, 'static final' https://codereview.adblockplus.org/29424615/diff/29424666/libadblockplus-andr... libadblockplus-android/src/org/adblockplus/libadblockplus/FileSystem.java:141: private final static native long ctor(Object callbackObject); Same here, 'static final' https://codereview.adblockplus.org/29424615/diff/29424666/libadblockplus-andr... libadblockplus-android/src/org/adblockplus/libadblockplus/FileSystem.java:143: private final static native void dtor(long ptr); Same here, 'static final' https://codereview.adblockplus.org/29424615/diff/29424666/libadblockplus-andr... File libadblockplus-android/src/org/adblockplus/libadblockplus/JsEngine.java (right): https://codereview.adblockplus.org/29424615/diff/29424666/libadblockplus-andr... libadblockplus-android/src/org/adblockplus/libadblockplus/JsEngine.java:160: private final static native void setFileSystem(long ptr, long fileSystemPtr); By our code style, the modifier order should be 'static final'. We'll also need a follow up issue for the other ocurrences https://codereview.adblockplus.org/29424615/diff/29424666/libadblockplus-andr... File libadblockplus-android/src/org/adblockplus/libadblockplus/android/AndroidFileSystem.java (right): https://codereview.adblockplus.org/29424615/diff/29424666/libadblockplus-andr... libadblockplus-android/src/org/adblockplus/libadblockplus/android/AndroidFileSystem.java:56: this(); Recapping our discussion here from the old code review: I didn't get your point here. If we omit 'this()' call, by default it will implicitly call 'super()', which is the empty ctor of FileSystem. Shouldn't that be the desired behaviour? Right now, it just calls the empty ctor of AndroidFileSystem, and this one implicitly calls super(), which produces the same result.
https://codereview.adblockplus.org/29424615/diff/29424666/libadblockplus-andr... File libadblockplus-android/src/org/adblockplus/libadblockplus/FileSystem.java (right): https://codereview.adblockplus.org/29424615/diff/29424666/libadblockplus-andr... libadblockplus-android/src/org/adblockplus/libadblockplus/FileSystem.java:123: private final static class DisposeWrapper implements Disposable On 2017/04/28 13:23:53, diegocarloslima wrote: > By our code style, the modifier order should be 'static final' currently this is made consistent with existing code style. to be changed for the whole repo for new separate task. https://codereview.adblockplus.org/29424615/diff/29424666/libadblockplus-andr... libadblockplus-android/src/org/adblockplus/libadblockplus/FileSystem.java:139: private final static native void registerNatives(); On 2017/04/28 13:23:53, diegocarloslima wrote: > Same here, 'static final' currently this is made consistent with existing code style. to be changed for the whole repo for new separate task. https://codereview.adblockplus.org/29424615/diff/29424666/libadblockplus-andr... libadblockplus-android/src/org/adblockplus/libadblockplus/FileSystem.java:141: private final static native long ctor(Object callbackObject); On 2017/04/28 13:23:53, diegocarloslima wrote: > Same here, 'static final' currently this is made consistent with existing code style. to be changed for the whole repo for new separate task. https://codereview.adblockplus.org/29424615/diff/29424666/libadblockplus-andr... libadblockplus-android/src/org/adblockplus/libadblockplus/FileSystem.java:143: private final static native void dtor(long ptr); On 2017/04/28 13:23:53, diegocarloslima wrote: > Same here, 'static final' currently this is made consistent with existing code style. to be changed for the whole repo for new separate task.
https://codereview.adblockplus.org/29424615/diff/29424666/libadblockplus-andr... File libadblockplus-android-tests/src/org/adblockplus/libadblockplus/tests/AndroidFileSystemTest.java (right): https://codereview.adblockplus.org/29424615/diff/29424666/libadblockplus-andr... libadblockplus-android-tests/src/org/adblockplus/libadblockplus/tests/AndroidFileSystemTest.java:107: final String singleLineData = DATA.replace("\n", ""); DATA is 12345qwerty, so this replace has no effect. https://codereview.adblockplus.org/29424615/diff/29424666/libadblockplus-andr... File libadblockplus-android-tests/src/org/adblockplus/libadblockplus/tests/FilterEngineGenericTest.java (right): https://codereview.adblockplus.org/29424615/diff/29424666/libadblockplus-andr... libadblockplus-android-tests/src/org/adblockplus/libadblockplus/tests/FilterEngineGenericTest.java:101: jsEngine.setFileSystem(new PatternsIniStubFileSystem()); Just for reference, I would expect that the proposed above implementation breaks some tests. Previously there were no patterns.ini nor prefs.json files but now there are and they are bare minimal what changes the behavior of core. https://codereview.adblockplus.org/29424615/diff/29424666/libadblockplus-andr... File libadblockplus-android/jni/JniFileSystem.cpp (right): https://codereview.adblockplus.org/29424615/diff/29424666/libadblockplus-andr... libadblockplus-android/jni/JniFileSystem.cpp:64: AdblockPlus::FileSystem() The call of AdblockPlus::FileSystem() is not required. https://codereview.adblockplus.org/29424615/diff/29424666/libadblockplus-andr... File libadblockplus-android/jni/JniJsEngine.cpp (right): https://codereview.adblockplus.org/29424615/diff/29424666/libadblockplus-andr... libadblockplus-android/jni/JniJsEngine.cpp:145: AdblockPlus::FileSystemPtr fileSystem(JniLongToTypePtr<JniFileSystemCallback>(fileSystemPtr)); This will result in double deletion because JniFileSystemCallback will be destroyed by JsEngine and by JniDtor. I think we should use a pointer to the smart pointer, the same as for WebRequest, for example. https://codereview.adblockplus.org/29424615/diff/29424666/libadblockplus-andr... File libadblockplus-android/jni/Utils.h (right): https://codereview.adblockplus.org/29424615/diff/29424666/libadblockplus-andr... libadblockplus-android/jni/Utils.h:165: This line seems unrelated. https://codereview.adblockplus.org/29424615/diff/29424666/libadblockplus-andr... File libadblockplus-android/src/org/adblockplus/libadblockplus/FileSystemUtils.java (right): https://codereview.adblockplus.org/29424615/diff/29424666/libadblockplus-andr... libadblockplus-android/src/org/adblockplus/libadblockplus/FileSystemUtils.java:43: return scanner.useDelimiter("\\A").next(); I'm not sure that it's the best way to read files in java at least I don't see any reason to parse it here and use any delimiter. What if \a will be a valid byte in the file? I would recommend to rather not use strings but use just bytes when we are writing and reading from a file. BTW, C++ interface is operating bytes for reading and writing. https://codereview.adblockplus.org/29424615/diff/29424666/libadblockplus-andr... libadblockplus-android/src/org/adblockplus/libadblockplus/FileSystemUtils.java:72: fos.write(data.getBytes()); If we still use String then I think we should pass UTF-8 into getBytes method. https://codereview.adblockplus.org/29424615/diff/29424666/libadblockplus-andr... File libadblockplus-android/src/org/adblockplus/libadblockplus/android/AndroidFileSystem.java (right): https://codereview.adblockplus.org/29424615/diff/29424666/libadblockplus-andr... libadblockplus-android/src/org/adblockplus/libadblockplus/android/AndroidFileSystem.java:149: } I think it inconsistent with C++ version where the method returns the absolute path to a file. https://github.com/adblockplus/libadblockplus/blob/master/include/AdblockPlus...
uploaded new patch set https://codereview.adblockplus.org/29424615/diff/29424666/libadblockplus-andr... File libadblockplus-android-tests/src/org/adblockplus/libadblockplus/tests/AndroidFileSystemTest.java (right): https://codereview.adblockplus.org/29424615/diff/29424666/libadblockplus-andr... libadblockplus-android-tests/src/org/adblockplus/libadblockplus/tests/AndroidFileSystemTest.java:107: final String singleLineData = DATA.replace("\n", ""); On 2017/05/22 12:09:06, sergei wrote: > DATA is 12345qwerty, so this replace has no effect. it just shows an intention to have single-line data even is `DATA` is changed later https://codereview.adblockplus.org/29424615/diff/29424666/libadblockplus-andr... File libadblockplus-android-tests/src/org/adblockplus/libadblockplus/tests/FilterEngineGenericTest.java (right): https://codereview.adblockplus.org/29424615/diff/29424666/libadblockplus-andr... libadblockplus-android-tests/src/org/adblockplus/libadblockplus/tests/FilterEngineGenericTest.java:101: jsEngine.setFileSystem(new PatternsIniStubFileSystem()); On 2017/05/22 12:09:06, sergei wrote: > Just for reference, I would expect that the proposed above implementation breaks > some tests. Previously there were no patterns.ini nor prefs.json files but now > there are and they are bare minimal what changes the behavior of core. Acknowledged. https://codereview.adblockplus.org/29424615/diff/29424666/libadblockplus-andr... File libadblockplus-android/jni/JniFileSystem.cpp (right): https://codereview.adblockplus.org/29424615/diff/29424666/libadblockplus-andr... libadblockplus-android/jni/JniFileSystem.cpp:64: AdblockPlus::FileSystem() On 2017/05/22 12:09:07, sergei wrote: > The call of AdblockPlus::FileSystem() is not required. Acknowledged. https://codereview.adblockplus.org/29424615/diff/29424666/libadblockplus-andr... File libadblockplus-android/jni/JniJsEngine.cpp (right): https://codereview.adblockplus.org/29424615/diff/29424666/libadblockplus-andr... libadblockplus-android/jni/JniJsEngine.cpp:145: AdblockPlus::FileSystemPtr fileSystem(JniLongToTypePtr<JniFileSystemCallback>(fileSystemPtr)); On 2017/05/22 12:09:07, sergei wrote: > This will result in double deletion because JniFileSystemCallback will be > destroyed by JsEngine and by JniDtor. I think we should use a pointer to the > smart pointer, the same as for WebRequest, for example. Acknowledged. https://codereview.adblockplus.org/29424615/diff/29424666/libadblockplus-andr... File libadblockplus-android/jni/Utils.h (right): https://codereview.adblockplus.org/29424615/diff/29424666/libadblockplus-andr... libadblockplus-android/jni/Utils.h:165: On 2017/05/22 12:09:07, sergei wrote: > This line seems unrelated. Acknowledged. https://codereview.adblockplus.org/29424615/diff/29424666/libadblockplus-andr... File libadblockplus-android/src/org/adblockplus/libadblockplus/FileSystemUtils.java (right): https://codereview.adblockplus.org/29424615/diff/29424666/libadblockplus-andr... libadblockplus-android/src/org/adblockplus/libadblockplus/FileSystemUtils.java:43: return scanner.useDelimiter("\\A").next(); On 2017/05/22 12:09:07, sergei wrote: > I'm not sure that it's the best way to read files in java at least I don't see > any reason to parse it here and use any delimiter. What if \a will be a valid > byte in the file? I would recommend to rather not use strings but use just bytes > when we are writing and reading from a file. BTW, C++ interface is operating > bytes for reading and writing. there are lots of ways for "reading file as string" and lot's of research for this topic. as for me using scanner is pretty old-school so i'll change it to more common-spread impl with streams. https://codereview.adblockplus.org/29424615/diff/29424666/libadblockplus-andr... libadblockplus-android/src/org/adblockplus/libadblockplus/FileSystemUtils.java:72: fos.write(data.getBytes()); On 2017/05/22 12:09:07, sergei wrote: > If we still use String then I think we should pass UTF-8 into getBytes method. Acknowledged. https://codereview.adblockplus.org/29424615/diff/29424666/libadblockplus-andr... File libadblockplus-android/src/org/adblockplus/libadblockplus/android/AndroidFileSystem.java (right): https://codereview.adblockplus.org/29424615/diff/29424666/libadblockplus-andr... libadblockplus-android/src/org/adblockplus/libadblockplus/android/AndroidFileSystem.java:56: this(); On 2017/04/28 13:23:53, diegocarloslima wrote: > Recapping our discussion here from the old code review: I didn't get your point > here. If we omit 'this()' call, by default it will implicitly call 'super()', > which is the empty ctor of FileSystem. Shouldn't that be the desired behaviour? > Right now, it just calls the empty ctor of AndroidFileSystem, and this one > implicitly calls super(), which produces the same result. AndroidFileSystem can be used without passing `basePath` in ctor and with it. The desired behaviour is to have a method to init instance for both use-cases and currently it's empty ctor body. So the desired behaviour is to call this instance empty ctor, not super ctor. We could remove `this()` invocation but then we should add some `init()` methods and call it from both empty and 1-var ctors. https://codereview.adblockplus.org/29424615/diff/29424666/libadblockplus-andr... libadblockplus-android/src/org/adblockplus/libadblockplus/android/AndroidFileSystem.java:149: } On 2017/05/22 12:09:07, sergei wrote: > I think it inconsistent with C++ version where the method returns the absolute > path to a file. > https://github.com/adblockplus/libadblockplus/blob/master/include/AdblockPlus... if it's created with `basePath` argument (see ctor) then basePath is the root and `absolute path` means `relative to basePath` (see `buildFullPath`). If `basePath` is not passed, then it's real absolute path.
https://codereview.adblockplus.org/29424615/diff/29424666/libadblockplus-andr... File libadblockplus-android-tests/src/org/adblockplus/libadblockplus/tests/AndroidFileSystemTest.java (right): https://codereview.adblockplus.org/29424615/diff/29424666/libadblockplus-andr... libadblockplus-android-tests/src/org/adblockplus/libadblockplus/tests/AndroidFileSystemTest.java:107: final String singleLineData = DATA.replace("\n", ""); On 2017/05/22 13:04:37, anton wrote: > On 2017/05/22 12:09:06, sergei wrote: > > DATA is 12345qwerty, so this replace has no effect. > > it just shows an intention to have single-line data even is `DATA` is changed > later In this case I would rather define a new one line value here instead of using DATA. https://codereview.adblockplus.org/29424615/diff/29424666/libadblockplus-andr... File libadblockplus-android/src/org/adblockplus/libadblockplus/android/AndroidFileSystem.java (right): https://codereview.adblockplus.org/29424615/diff/29424666/libadblockplus-andr... libadblockplus-android/src/org/adblockplus/libadblockplus/android/AndroidFileSystem.java:149: } On 2017/05/22 13:04:38, anton wrote: > On 2017/05/22 12:09:07, sergei wrote: > > I think it inconsistent with C++ version where the method returns the absolute > > path to a file. > > > https://github.com/adblockplus/libadblockplus/blob/master/include/AdblockPlus... > > if it's created with `basePath` argument (see ctor) then basePath is the root > and `absolute path` means `relative to basePath` (see `buildFullPath`). > If `basePath` is not passed, then it's real absolute path. I think that `resolve` method had been introduced to firstly resolve any kind of relative paths and it should always return a full absolute path, thus potentially exposing any information about internal base path. E.g., though it's like an error in the app, for the following call resolve("dirX/../../fileZ"); and base path "/dirA/dirB" the result should be "/dirA/fileZ", however with that implementation it will be "/dirA/fileZ".substring("/dirA/dirB".length => 10) => "Z". For instance, the implementation for windows is https://github.com/adblockplus/libadblockplus/blob/master/src/DefaultFileSyst.... Anyway, in the recent version of adblockpluscore it seems there are no anymore calls to this resolve method and I guess it's supposed that other methods are aware about it, so if it works now, then maybe it's fine because later it will be removed as a dead code. https://codereview.adblockplus.org/29424615/diff/29445621/libadblockplus-andr... File libadblockplus-android/src/org/adblockplus/libadblockplus/FileSystemUtils.java (right): https://codereview.adblockplus.org/29424615/diff/29445621/libadblockplus-andr... libadblockplus-android/src/org/adblockplus/libadblockplus/FileSystemUtils.java:35: private static String readStream(InputStream inputStream, int bufferSize) throws IOException What about returning some bytes array or some bytes array builder (what is the better Java class), I don't fill comfortable with String because I am afraid it can be corrupted if the file is a binary file.
https://codereview.adblockplus.org/29424615/diff/29424666/libadblockplus-andr... File libadblockplus-android/src/org/adblockplus/libadblockplus/android/AndroidFileSystem.java (right): https://codereview.adblockplus.org/29424615/diff/29424666/libadblockplus-andr... libadblockplus-android/src/org/adblockplus/libadblockplus/android/AndroidFileSystem.java:149: } On 2017/05/22 13:51:04, sergei wrote: > On 2017/05/22 13:04:38, anton wrote: > > On 2017/05/22 12:09:07, sergei wrote: > > > I think it inconsistent with C++ version where the method returns the > absolute > > > path to a file. > > > > > > https://github.com/adblockplus/libadblockplus/blob/master/include/AdblockPlus... > > > > if it's created with `basePath` argument (see ctor) then basePath is the root > > and `absolute path` means `relative to basePath` (see `buildFullPath`). > > If `basePath` is not passed, then it's real absolute path. > > I think that `resolve` method had been introduced to firstly resolve any kind of > relative paths and it should always return a full absolute path, thus > potentially exposing any information about internal base path. > E.g., though it's like an error in the app, for the following call > resolve("dirX/../../fileZ"); and base path "/dirA/dirB" the result should be > "/dirA/fileZ", however with that implementation it will be > "/dirA/fileZ".substring("/dirA/dirB".length => 10) => "Z". For instance, the > implementation for windows is > https://github.com/adblockplus/libadblockplus/blob/master/src/DefaultFileSyst.... > > Anyway, in the recent version of adblockpluscore it seems there are no anymore > calls to this resolve method and I guess it's supposed that other methods are > aware about it, so if it works now, then maybe it's fine because later it will > be removed as a dead code. You did not get the idea that was introduced by android file system specific. The app's files root is /data/data/package/ in Android. So we need to consider all paths in c++ as related to that root. If you're using path "/somefile" in c++, it will be /data/data/package/somefile in Android and when you request "/somefile/../somanotherfile" in android it will be "/data/data/package/somefile/../someanotherfile" but in c++ you will have "/someanotherfile" path returned by "resolve". So in c++ you will never know actual basePath used on android side. Another example. You have c++ path "patterns.ini". On android side it will be "/data/data/package/patterns.ini". if you request resolve("patterns.ini") in c++, it will return you "/patterns.ini" (though on android side path "/data/data/package/patterns.ini" is used). https://codereview.adblockplus.org/29424615/diff/29445621/libadblockplus-andr... File libadblockplus-android/src/org/adblockplus/libadblockplus/FileSystemUtils.java (right): https://codereview.adblockplus.org/29424615/diff/29445621/libadblockplus-andr... libadblockplus-android/src/org/adblockplus/libadblockplus/FileSystemUtils.java:35: private static String readStream(InputStream inputStream, int bufferSize) throws IOException On 2017/05/22 13:51:04, sergei wrote: > What about returning some bytes array or some bytes array builder (what is the > better Java class), I don't fill comfortable with String because I am afraid it > can be corrupted if the file is a binary file. Strings in java are not like null-terminated strings in c++ and can have '0x0' or other non-text characters. Can you provide an example of binary (few bytes) and i will test it.
https://codereview.adblockplus.org/29424615/diff/29424666/libadblockplus-andr... File libadblockplus-android/src/org/adblockplus/libadblockplus/android/AndroidFileSystem.java (right): https://codereview.adblockplus.org/29424615/diff/29424666/libadblockplus-andr... libadblockplus-android/src/org/adblockplus/libadblockplus/android/AndroidFileSystem.java:149: } On 2017/05/23 05:46:47, anton wrote: > On 2017/05/22 13:51:04, sergei wrote: > > On 2017/05/22 13:04:38, anton wrote: > > > On 2017/05/22 12:09:07, sergei wrote: > > > > I think it inconsistent with C++ version where the method returns the > > absolute > > > > path to a file. > > > > > > > > > > https://github.com/adblockplus/libadblockplus/blob/master/include/AdblockPlus... > > > > > > if it's created with `basePath` argument (see ctor) then basePath is the > root > > > and `absolute path` means `relative to basePath` (see `buildFullPath`). > > > If `basePath` is not passed, then it's real absolute path. > > > > I think that `resolve` method had been introduced to firstly resolve any kind > of > > relative paths and it should always return a full absolute path, thus > > potentially exposing any information about internal base path. > > E.g., though it's like an error in the app, for the following call > > resolve("dirX/../../fileZ"); and base path "/dirA/dirB" the result should be > > "/dirA/fileZ", however with that implementation it will be > > "/dirA/fileZ".substring("/dirA/dirB".length => 10) => "Z". For instance, the > > implementation for windows is > > > https://github.com/adblockplus/libadblockplus/blob/master/src/DefaultFileSyst.... > > > > Anyway, in the recent version of adblockpluscore it seems there are no anymore > > calls to this resolve method and I guess it's supposed that other methods are > > aware about it, so if it works now, then maybe it's fine because later it will > > be removed as a dead code. > > You did not get the idea that was introduced by android file system specific. > > The app's files root is /data/data/package/ in Android. So we need to consider > all paths in c++ as related to that root. > If you're using path "/somefile" in c++, it will be /data/data/package/somefile > in Android and when you request "/somefile/../somanotherfile" in android > it will be "/data/data/package/somefile/../someanotherfile" but in c++ you will > have "/someanotherfile" path returned by "resolve". > > So in c++ you will never know actual basePath used on android side. > > Another example. You have c++ path "patterns.ini". On android side it will be > "/data/data/package/patterns.ini". if you request resolve("patterns.ini") in > c++, it will > return you "/patterns.ini" (though on android side path > "/data/data/package/patterns.ini" is used). It was clear and I think that it's incorrect. I think that it's expected that resolve always returns an absolute path what is consistent with documentation and in addition without any relative things like "..", "./". E.g. for for "patterns.ini", for "./patterns.ini" and for "./some-path/../patterns.ini" the result is "/data/data/package/patterns.ini". BTW, the same for other methods, one should use base path only if the path does not start from "/". How can one write to /sdcard/file-some using the proposed API? https://codereview.adblockplus.org/29424615/diff/29445621/libadblockplus-andr... File libadblockplus-android/src/org/adblockplus/libadblockplus/FileSystemUtils.java (right): https://codereview.adblockplus.org/29424615/diff/29445621/libadblockplus-andr... libadblockplus-android/src/org/adblockplus/libadblockplus/FileSystemUtils.java:35: private static String readStream(InputStream inputStream, int bufferSize) throws IOException On 2017/05/23 05:46:47, anton wrote: > On 2017/05/22 13:51:04, sergei wrote: > > What about returning some bytes array or some bytes array builder (what is the > > better Java class), I don't fill comfortable with String because I am afraid > it > > can be corrupted if the file is a binary file. > > Strings in java are not like null-terminated strings in c++ and can have '0x0' > or other non-text characters. > Can you provide an example of binary (few bytes) and i will test it. I'm not talking only about null characters, what is also allowed in std::string, though can be confusing. Try for example a file with "C0 C0" (it's a hex, two bytes). It's an invalid UTF-8 sequence, because after 110xxxxx there should be 10xxxxxx but we put again 110xxxxx, because C0 is 11000000.
https://codereview.adblockplus.org/29424615/diff/29445621/libadblockplus-andr... File libadblockplus-android/src/org/adblockplus/libadblockplus/FileSystemUtils.java (right): https://codereview.adblockplus.org/29424615/diff/29445621/libadblockplus-andr... libadblockplus-android/src/org/adblockplus/libadblockplus/FileSystemUtils.java:35: private static String readStream(InputStream inputStream, int bufferSize) throws IOException On 2017/05/23 08:08:19, sergei wrote: > On 2017/05/23 05:46:47, anton wrote: > > On 2017/05/22 13:51:04, sergei wrote: > > > What about returning some bytes array or some bytes array builder (what is > the > > > better Java class), I don't fill comfortable with String because I am afraid > > it > > > can be corrupted if the file is a binary file. > > > > Strings in java are not like null-terminated strings in c++ and can have '0x0' > > or other non-text characters. > > Can you provide an example of binary (few bytes) and i will test it. > > I'm not talking only about null characters, what is also allowed in std::string, > though can be confusing. Try for example a file with "C0 C0" (it's a hex, two > bytes). It's an invalid UTF-8 sequence, because after 110xxxxx there should be > 10xxxxxx but we put again 110xxxxx, because C0 is 11000000. Here are http://www.cl.cam.ac.uk/~mgk25/ucs/examples/UTF-8-test.txt more tests. Do I correctly understand that according to the doc http://docs.oracle.com/javase/6/docs/api/java/lang/String.html ("This method always replaces malformed-input and unmappable-character sequences with this charset's default replacement string.") some unmappable-character sequences will be replaced by something else?
https://codereview.adblockplus.org/29424615/diff/29424666/libadblockplus-andr... File libadblockplus-android/src/org/adblockplus/libadblockplus/android/AndroidFileSystem.java (right): https://codereview.adblockplus.org/29424615/diff/29424666/libadblockplus-andr... libadblockplus-android/src/org/adblockplus/libadblockplus/android/AndroidFileSystem.java:149: } On 2017/05/23 08:08:19, sergei wrote: > On 2017/05/23 05:46:47, anton wrote: > > On 2017/05/22 13:51:04, sergei wrote: > > > On 2017/05/22 13:04:38, anton wrote: > > > > On 2017/05/22 12:09:07, sergei wrote: > > > > > I think it inconsistent with C++ version where the method returns the > > > absolute > > > > > path to a file. > > > > > > > > > > > > > > > https://github.com/adblockplus/libadblockplus/blob/master/include/AdblockPlus... > > > > > > > > if it's created with `basePath` argument (see ctor) then basePath is the > > root > > > > and `absolute path` means `relative to basePath` (see `buildFullPath`). > > > > If `basePath` is not passed, then it's real absolute path. > > > > > > I think that `resolve` method had been introduced to firstly resolve any > kind > > of > > > relative paths and it should always return a full absolute path, thus > > > potentially exposing any information about internal base path. > > > E.g., though it's like an error in the app, for the following call > > > resolve("dirX/../../fileZ"); and base path "/dirA/dirB" the result should be > > > "/dirA/fileZ", however with that implementation it will be > > > "/dirA/fileZ".substring("/dirA/dirB".length => 10) => "Z". For instance, the > > > implementation for windows is > > > > > > https://github.com/adblockplus/libadblockplus/blob/master/src/DefaultFileSyst.... > > > > > > Anyway, in the recent version of adblockpluscore it seems there are no > anymore > > > calls to this resolve method and I guess it's supposed that other methods > are > > > aware about it, so if it works now, then maybe it's fine because later it > will > > > be removed as a dead code. > > > > You did not get the idea that was introduced by android file system specific. > > > > The app's files root is /data/data/package/ in Android. So we need to consider > > all paths in c++ as related to that root. > > If you're using path "/somefile" in c++, it will be > /data/data/package/somefile > > in Android and when you request "/somefile/../somanotherfile" in android > > it will be "/data/data/package/somefile/../someanotherfile" but in c++ you > will > > have "/someanotherfile" path returned by "resolve". > > > > So in c++ you will never know actual basePath used on android side. > > > > Another example. You have c++ path "patterns.ini". On android side it will be > > "/data/data/package/patterns.ini". if you request resolve("patterns.ini") in > > c++, it will > > return you "/patterns.ini" (though on android side path > > "/data/data/package/patterns.ini" is used). > > It was clear and I think that it's incorrect. Can't get your point. All the paths are considered to be relative to 'basePath'. > I think that it's expected that resolve always returns an absolute path what is > consistent with documentation and in addition without any relative things like > "..", "./". E.g. for for "patterns.ini", for "./patterns.ini" and for > "./some-path/../patterns.ini" the result is "/data/data/package/patterns.ini". "/data/data/package" was introduced by android side and it should not be exposed to c++ side. For all incoming paths "/data/data/package" is added when entering android side and stripped when returning back to c++ side. You will not get any relative paths just because of adding basePath at the beginning. I mean adding basePath is fully transparent for c++ side and does not affect absolute/relative path point. For resolve("/somepath/../file") you will get "/file" > BTW, the same for other methods, one should use base path only if the path does > not start from "/". > How can one write to /sdcard/file-some using the proposed API? if you wish to access "/sdcard/file" in c++ code it means you're trying to leave app sand-box and you just don't need to pass 'basePath' in AndroidFileSystem ctor. You can do it but it means c++ part is aware of the environment and it should take into account it's an android and it's fs limitations, e.g you can't write to "/" and so on. https://codereview.adblockplus.org/29424615/diff/29445621/libadblockplus-andr... File libadblockplus-android/src/org/adblockplus/libadblockplus/FileSystemUtils.java (right): https://codereview.adblockplus.org/29424615/diff/29445621/libadblockplus-andr... libadblockplus-android/src/org/adblockplus/libadblockplus/FileSystemUtils.java:35: private static String readStream(InputStream inputStream, int bufferSize) throws IOException On 2017/05/23 08:08:19, sergei wrote: > On 2017/05/23 05:46:47, anton wrote: > > On 2017/05/22 13:51:04, sergei wrote: > > > What about returning some bytes array or some bytes array builder (what is > the > > > better Java class), I don't fill comfortable with String because I am afraid > > it > > > can be corrupted if the file is a binary file. > > > > Strings in java are not like null-terminated strings in c++ and can have '0x0' > > or other non-text characters. > > Can you provide an example of binary (few bytes) and i will test it. > > I'm not talking only about null characters, what is also allowed in std::string, > though can be confusing. Try for example a file with "C0 C0" (it's a hex, two > bytes). It's an invalid UTF-8 sequence, because after 110xxxxx there should be > 10xxxxxx but we put again 110xxxxx, because C0 is 11000000. we should decide first if we consider all the files as utf-8 (as currently assumed) or not. If it's not, we can change API to work with byte[].
https://codereview.adblockplus.org/29424615/diff/29424666/libadblockplus-andr... File libadblockplus-android/src/org/adblockplus/libadblockplus/android/AndroidFileSystem.java (right): https://codereview.adblockplus.org/29424615/diff/29424666/libadblockplus-andr... libadblockplus-android/src/org/adblockplus/libadblockplus/android/AndroidFileSystem.java:149: } On 2017/05/23 11:11:18, anton wrote: > On 2017/05/23 08:08:19, sergei wrote: > > On 2017/05/23 05:46:47, anton wrote: > > > On 2017/05/22 13:51:04, sergei wrote: > > > > On 2017/05/22 13:04:38, anton wrote: > > > > > On 2017/05/22 12:09:07, sergei wrote: > > > > > > I think it inconsistent with C++ version where the method returns the > > > > absolute > > > > > > path to a file. > > > > > > > > > > > > > > > > > > > > > https://github.com/adblockplus/libadblockplus/blob/master/include/AdblockPlus... > > > > > > > > > > if it's created with `basePath` argument (see ctor) then basePath is the > > > root > > > > > and `absolute path` means `relative to basePath` (see `buildFullPath`). > > > > > If `basePath` is not passed, then it's real absolute path. > > > > > > > > I think that `resolve` method had been introduced to firstly resolve any > > kind > > > of > > > > relative paths and it should always return a full absolute path, thus > > > > potentially exposing any information about internal base path. > > > > E.g., though it's like an error in the app, for the following call > > > > resolve("dirX/../../fileZ"); and base path "/dirA/dirB" the result should > be > > > > "/dirA/fileZ", however with that implementation it will be > > > > "/dirA/fileZ".substring("/dirA/dirB".length => 10) => "Z". For instance, > the > > > > implementation for windows is > > > > > > > > > > https://github.com/adblockplus/libadblockplus/blob/master/src/DefaultFileSyst.... > > > > > > > > Anyway, in the recent version of adblockpluscore it seems there are no > > anymore > > > > calls to this resolve method and I guess it's supposed that other methods > > are > > > > aware about it, so if it works now, then maybe it's fine because later it > > will > > > > be removed as a dead code. > > > > > > You did not get the idea that was introduced by android file system > specific. > > > > > > The app's files root is /data/data/package/ in Android. So we need to > consider > > > all paths in c++ as related to that root. > > > If you're using path "/somefile" in c++, it will be > > /data/data/package/somefile > > > in Android and when you request "/somefile/../somanotherfile" in android > > > it will be "/data/data/package/somefile/../someanotherfile" but in c++ you > > will > > > have "/someanotherfile" path returned by "resolve". > > > > > > So in c++ you will never know actual basePath used on android side. > > > > > > Another example. You have c++ path "patterns.ini". On android side it will > be > > > "/data/data/package/patterns.ini". if you request resolve("patterns.ini") in > > > c++, it will > > > return you "/patterns.ini" (though on android side path > > > "/data/data/package/patterns.ini" is used). > > > > It was clear and I think that it's incorrect. > > Can't get your point. All the paths are considered to be relative to 'basePath'. > > > I think that it's expected that resolve always returns an absolute path what > is > > consistent with documentation and in addition without any relative things like > > "..", "./". E.g. for for "patterns.ini", for "./patterns.ini" and for > > "./some-path/../patterns.ini" the result is "/data/data/package/patterns.ini". > > "/data/data/package" was introduced by android side and it should not be exposed > to c++ side. > For all incoming paths "/data/data/package" is added when entering android side > and stripped when returning back to c++ side. > You will not get any relative paths just because of adding basePath at the > beginning. > I mean adding basePath is fully transparent for c++ side and does not affect > absolute/relative path point. > For resolve("/somepath/../file") you will get "/file" > > > BTW, the same for other methods, one should use base path only if the path > does > > not start from "/". > > How can one write to /sdcard/file-some using the proposed API? > > if you wish to access "/sdcard/file" in c++ code it means you're trying to leave > app sand-box and you just don't need to pass 'basePath' in AndroidFileSystem > ctor. You can do it but it means c++ part is aware of the environment and it > should take into account it's an android and it's fs limitations, e.g you can't > write to "/" and so on. In my opinion the aim of FileSystem interface is to provide with an abstraction from platform low level functions. However, here, merely because it's convenient and seems logical, we use it in addition to override a current working directory for ABP which is denoted as the base path here. Since there was no demand in additional sendboxing level so far I think we should not add it until it's required. If the code is willing to write or read at absolute location and it's not allowed, then it's an issue of that code. For example, there can be a completely legit code in libadblockplus-android which is aware about current environment and which should work with something outside the base path, any why should it not be able to work with the provided interface for file system? Even more, it is expected that method of the interface returns "the absolute path to a file" (https://github.com/adblockplus/libadblockplus/blob/master/include/AdblockPlus...) and it's an abstraction around platform low level functions. https://codereview.adblockplus.org/29424615/diff/29445621/libadblockplus-andr... File libadblockplus-android/src/org/adblockplus/libadblockplus/FileSystemUtils.java (right): https://codereview.adblockplus.org/29424615/diff/29445621/libadblockplus-andr... libadblockplus-android/src/org/adblockplus/libadblockplus/FileSystemUtils.java:35: private static String readStream(InputStream inputStream, int bufferSize) throws IOException On 2017/05/23 11:11:18, anton wrote: > On 2017/05/23 08:08:19, sergei wrote: > > On 2017/05/23 05:46:47, anton wrote: > > > On 2017/05/22 13:51:04, sergei wrote: > > > > What about returning some bytes array or some bytes array builder (what is > > the > > > > better Java class), I don't fill comfortable with String because I am > afraid > > > it > > > > can be corrupted if the file is a binary file. > > > > > > Strings in java are not like null-terminated strings in c++ and can have > '0x0' > > > or other non-text characters. > > > Can you provide an example of binary (few bytes) and i will test it. > > > > I'm not talking only about null characters, what is also allowed in > std::string, > > though can be confusing. Try for example a file with "C0 C0" (it's a hex, two > > bytes). It's an invalid UTF-8 sequence, because after 110xxxxx there should be > > 10xxxxxx but we put again 110xxxxx, because C0 is 11000000. > > we should decide first if we consider all the files as utf-8 (as currently > assumed) or not. > If it's not, we can change API to work with byte[]. Well, C++ API is using std::istream which is for binary data, why should we limit the type of data in the Java part which is implementation of that interface?
https://codereview.adblockplus.org/29424615/diff/29445621/libadblockplus-andr... File libadblockplus-android/src/org/adblockplus/libadblockplus/FileSystemUtils.java (right): https://codereview.adblockplus.org/29424615/diff/29445621/libadblockplus-andr... libadblockplus-android/src/org/adblockplus/libadblockplus/FileSystemUtils.java:35: private static String readStream(InputStream inputStream, int bufferSize) throws IOException On 2017/05/23 13:00:37, sergei wrote: > On 2017/05/23 11:11:18, anton wrote: > > On 2017/05/23 08:08:19, sergei wrote: > > > On 2017/05/23 05:46:47, anton wrote: > > > > On 2017/05/22 13:51:04, sergei wrote: > > > > > What about returning some bytes array or some bytes array builder (what > is > > > the > > > > > better Java class), I don't fill comfortable with String because I am > > afraid > > > > it > > > > > can be corrupted if the file is a binary file. > > > > > > > > Strings in java are not like null-terminated strings in c++ and can have > > '0x0' > > > > or other non-text characters. > > > > Can you provide an example of binary (few bytes) and i will test it. > > > > > > I'm not talking only about null characters, what is also allowed in > > std::string, > > > though can be confusing. Try for example a file with "C0 C0" (it's a hex, > two > > > bytes). It's an invalid UTF-8 sequence, because after 110xxxxx there should > be > > > 10xxxxxx but we put again 110xxxxx, because C0 is 11000000. > > > > we should decide first if we consider all the files as utf-8 (as currently > > assumed) or not. > > If it's not, we can change API to work with byte[]. > > Well, C++ API is using std::istream which is for binary data, why should we > limit the type of data in the Java part which is implementation of that > interface? moved to byte array instead of String. Check out new patch set https://codereview.adblockplus.org/29424615/diff/29450591/libadblockplus-andr... File libadblockplus-android/src/org/adblockplus/libadblockplus/android/AndroidFileSystem.java (right): https://codereview.adblockplus.org/29424615/diff/29450591/libadblockplus-andr... libadblockplus-android/src/org/adblockplus/libadblockplus/android/AndroidFileSystem.java:65: File file = new File(path); Warning: it does not do resolve() just like in DefaultFileSystem.cpp. This is going to be updated as soon as it's changed in c++ https://codereview.adblockplus.org/29424615/diff/29450591/libadblockplus-andr... libadblockplus-android/src/org/adblockplus/libadblockplus/android/AndroidFileSystem.java:84: File file = new File(path); Warning: it does not do resolve() just like in DefaultFileSystem.cpp. This is going to be updated as soon as it's changed in c++
https://codereview.adblockplus.org/29424615/diff/29450591/libadblockplus-andr... File libadblockplus-android/jni/JniCallbacks.cpp (right): https://codereview.adblockplus.org/29424615/diff/29450591/libadblockplus-andr... libadblockplus-android/jni/JniCallbacks.cpp:58: bool JniCallbackBase::CheckAndLogJavaException(JNIEnv* env) const Is it an unrelated change? https://codereview.adblockplus.org/29424615/diff/29450591/libadblockplus-andr... File libadblockplus-android/jni/JniCallbacks.h (right): https://codereview.adblockplus.org/29424615/diff/29450591/libadblockplus-andr... libadblockplus-android/jni/JniCallbacks.h:87: class JniFileSystemCallback : public JniCallbackBase, public AdblockPlus::FileSystem FileSystem interface is removed in the recent libadblockplus, one should use IFileSystem instead, though I'm not sure whether it makes sense to do it as a part of this codereview, can be done later. https://codereview.adblockplus.org/29424615/diff/29450591/libadblockplus-andr... File libadblockplus-android/jni/JniFileSystem.cpp (right): https://codereview.adblockplus.org/29424615/diff/29450591/libadblockplus-andr... libadblockplus-android/jni/JniFileSystem.cpp:45: }; All these things should be in the anonymous namespace, however fortunately all these C++ streams will go away after rabasing. https://codereview.adblockplus.org/29424615/diff/29450591/libadblockplus-andr... libadblockplus-android/jni/JniFileSystem.cpp:68: statResultClass = NULL; nullptr? https://codereview.adblockplus.org/29424615/diff/29450591/libadblockplus-andr... libadblockplus-android/jni/JniFileSystem.cpp:72: static jlong JNICALL JniCtor(JNIEnv* env, jclass clazz, jobject callbackObject) I think that it should be done a little bit differently. JniFileSystemCallback inherits JniCallbackBase which holds a strong reference to a corresponding Java class, the lifetime of JniFileSystemCallback is managed by JsEngine and there are no calls from Java class of methods of JniFileSystemCallback. So, there is no need to have a pointer to C++ class in Java, instead there should be an interface in Java which is passed into some Java JsEngine.SetFileSystem (BTW, it will reduce the number of further changes if it's done in the constructor of JsEngine) and finally as jobject into some JniJsEngine.cpp::JniSetFileSystem (or in JniCtor). In JniSetFileSystem we simply create JniFileSystemCallback accepting jobject and holding it. The implementation of methods of AdblockPlus::FileSystem should simply call corresponding methods of the stored Java interface. In addition the most recent changes don't use a std::shared_ptr<FileSystem>, the ownership is passed to a corresponding class via std::unique_ptr what basically implies and forces the described above approach. https://codereview.adblockplus.org/29424615/diff/29450591/libadblockplus-andr... File libadblockplus-android/jni/Utils.h (right): https://codereview.adblockplus.org/29424615/diff/29450591/libadblockplus-andr... libadblockplus-android/jni/Utils.h:34: namespace AdblockPlus This will be removed after rabasing. 
|
