Bug 58443

*** Bug 57925 has been marked as a duplicate of this bug. ***

Do you mean something else than Source/WebCore/platform/gtk/FileSystemGtk.cpp? - which is the implementation of Souce/WebCore/platform/FileSystem.h (this can be found in r84622)

Yes. I believe this is the relevant draft: http://dev.w3.org/2009/dap/file-system/pub/FileSystem/

Created attachment 102035 [details] WIP patch This is a work-in-progress patch that brings support for FileSystem API to Gtk port. This is a first implementation (AFAIK) for a port that uses JavaScriptCore so some fixes were needed in bindings/js. The only other port to implement this feature is Chromium, so some changes (for instance those in LocalFileSystem class) should be discussed to avoid any problems. Also to discuss are the possible security problems and additional API for Gtk port. AsyncFileSystemGtk, AsyncFileWriterGtk and AsyncFileWriterClientGtk are implemented. Two helper classes are also added - AsyncFileSystemTaskControllerGtk and AsyncFileSystemCallbacksGtk. The first controls posting tasks to ScriptExecutionContext (either a Document object or a Worker) and the second is used to wrap normal callbacks and properly execute them using a task controller. With this patch non-worker tests in fast/filesystems/ (24 of them) pass flawlessly. When running a single worker test, for example async-operations.html, repeating it 5 times, the second time the test fails because of a malformed EntryArray object (object has undefined length when there are actual entries that were read) and the third time the test crashes because of what appears to be an already freed HashEntry key. Here's the backtrace: Program received signal SIGSEGV, Segmentation fault. [Switching to Thread 0x7fff9cd31700 (LWP 15534)] 0x00007ffff4458540 in JSC::HashEntry::key (this=0x72) at ../../Source/JavaScriptCore/runtime/Lookup.h:76 76 StringImpl* key() const { return m_key; } #0 0x00007ffff4458540 in JSC::HashEntry::key (this=0x72) at ../../Source/JavaScriptCore/runtime/Lookup.h:76 No locals. #1 0x00007ffff44586fb in JSC::HashTable::entry (this=0x7ffff750a020, identifier=...) at ../../Source/JavaScriptCore/runtime/Lookup.h:169 __PRETTY_FUNCTION__ = "const JSC::HashEntry* JSC::HashTable::entry(const JSC::Identifier&) const" entry = 0x72 #2 0x00007ffff4458640 in JSC::HashTable::entry (this=0x7ffff750a020, exec=0x7fff9b9300c0, identifier=...) at ../../Source/JavaScriptCore/runtime/Lookup.h:155 No locals. #3 0x00007ffff529756e in WebCore::JSEntryArray::getOwnPropertySlot (this=0x7ffff7ecba38, exec=0x7fff9b9300c0, propertyName=..., slot=...) at DerivedSources/WebCore/JSEntryArray.cpp:165 entry = 0x7fff9cd30310 ok = false index = 4425488 #4 0x00007ffff77e67e5 in JSC::JSCell::fastGetOwnPropertySlot (this=0x7ffff7ecba38, exec=0x7fff9b9300c0, propertyName=..., slot=...) at ../../Source/JavaScriptCore/runtime/JSObject.h:536 No locals. #5 0x00007ffff785c39f in JSC::JSValue::get (this=0x7fff9cd30430, exec=0x7fff9b9300c0, propertyName=..., slot=...) at ../../Source/JavaScriptCore/runtime/JSObject.h:788 prototype = {u = {asInt64 = 0, ptr = 0x0, asBits = {payload = 0, tag = 0}}} cell = 0x7ffff7ecba38 #6 0x00007ffff788b736 in JSC::cti_op_get_by_id (args=0x7fff9cd30480) ---Type <return> to continue, or q <return> to quit--- at ../../Source/JavaScriptCore/jit/JITStubs.cpp:1563 stackHack = {stackFrame = @0x7fff9cd30480, savedReturnAddress = {m_value = 0x7fff9d950c68}} callFrame = 0x7fff9b9300c0 slot = {m_getValue = 0x7fff9b930118, m_getIndexValue = 0x2, m_slotBase = {u = {asInt64 = 140737352874552, ptr = 0x7ffff7ecba38, asBits = {payload = -135480776, tag = 32767}}}, m_data = {getterFunc = 0xdf8350, index = 14648144}, m_value = {u = {asInt64 = 0, ptr = 0x0, asBits = {payload = 0, tag = 0}}}, m_thisValue = {u = {asInt64 = 0, ptr = 0x0, asBits = {payload = 0, tag = 0}}}, m_offset = 0, m_cachedPropertyType = JSC::PropertySlot::Uncacheable} stubInfo = 0x7fff9cd30480 stackFrame = @0x7fff9cd30480 ident = @0xdf5550 baseValue = {u = {asInt64 = 140737352874552, ptr = 0x7ffff7ecba38, asBits = {payload = -135480776, tag = 32767}}} result = {u = {asInt64 = 140735824462960, ptr = 0x7fff9cd30470, asBits = {payload = -1663892368, tag = 32767}}} codeBlock = 0x7fff9cd30480 #7 0x00007ffff7889d65 in JSC::JITThunks::tryCacheGetByID (callFrame=0x7fff9cd303a0, codeBlock=0x7fff9cd303d0, returnAddress=..., baseValue=..., propertyName=..., slot=..., stubInfo=0x7ffff7ecba38) at ../../Source/JavaScriptCore/jit/JITStubs.cpp:941 No locals. #8 0x00007fff9cd304b0 in ?? () . . . This might be a problem in how AsyncFileSystemTaskControllerGtk utilizes workers or maybe even how JavaScriptCore implements them. Given all the worker tests pass when running with --singly flag it might be a matter of a hash table not cleared or something similar ... Throughout the patch there might be memory leaks and debugging leftovers. This is a work-in-progress patch, not meant for review, but don't worry, such things will be fixed or removed before a patch is posted for reviewing. I am open to discussion about approach and fixing the worker crashes.

Attachment 102035 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/fast/filesystem/script-tests/f..." exit_code: 1 Source/WebCore/platform/gtk/AsyncFileWriterGtk.h:8: Alphabetical sorting problem. [build/include_order] [4] Source/WebCore/platform/gtk/AsyncFileWriterGtk.cpp:15: Alphabetical sorting problem. [build/include_order] [4] Source/WebCore/platform/gtk/AsyncFileWriterGtk.cpp:44: Local variables should never be PassRefPtr (see http://webkit.org/coding/RefPtr.html). [readability/pass_ptr] [5] Source/WebCore/platform/gtk/AsyncFileWriterGtk.cpp:49: Should have a space between // and comment [whitespace/comments] [4] Source/WebCore/platform/gtk/AsyncFileWriterGtk.cpp:53: Should have a space between // and comment [whitespace/comments] [4] Source/WebCore/platform/gtk/AsyncFileSystemCallbacksGtk.cpp:11: Alphabetical sorting problem. [build/include_order] [4] Source/WebCore/platform/gtk/AsyncFileSystemCallbacksGtk.h:14: This { should be at the end of the previous line [whitespace/braces] [4] Source/WebCore/platform/gtk/AsyncFileSystemCallbacksGtk.h:32: The parameter name "writer" adds no information, so it should be removed. [readability/parameter_name] [5] Source/WebCore/platform/gtk/AsyncFileSystemGtk.h:32: The parameter name "client" adds no information, so it should be removed. [readability/parameter_name] [5] Source/WebCore/platform/gtk/AsyncFileSystemTaskControllerGtk.cpp:22: One line control clauses should not use braces. [whitespace/braces] [4] Source/WebCore/platform/gtk/AsyncFileSystemTaskControllerGtk.cpp:77: Should have a space between // and comment [whitespace/comments] [4] Source/WebCore/platform/gtk/AsyncFileSystemTaskControllerGtk.cpp:106: Should have a space between // and comment [whitespace/comments] [4] Source/WebCore/platform/gtk/AsyncFileWriterClientGtk.h:8: Alphabetical sorting problem. [build/include_order] [4] Source/WebCore/platform/gtk/AsyncFileSystemGtk.cpp:20: Alphabetical sorting problem. [build/include_order] [4] Source/WebCore/platform/gtk/AsyncFileSystemTaskControllerGtk.h:16: This { should be at the end of the previous line [whitespace/braces] [4] Source/WebCore/platform/gtk/AsyncFileSystemTaskControllerGtk.h:28: The parameter name "taskMode" adds no information, so it should be removed. [readability/parameter_name] [5] Total errors found: 16 in 21 files If any of these errors are false positives, please file a bug against check-webkit-style.

Any progress? :)

Let me continue this bug, I may able to share my patch in a few days. :)

Created attachment 135569 [details] Patch

Created attachment 135620 [details] Patch

(In reply to comment #9) > Created an attachment (id=135620) [details] > Patch Updated copyright for Zan Dobersek. @Zan Dobersek, I added your copyright. But if you think you need to modify it, let me know. :)

Created attachment 135795 [details] Patch

(In reply to comment #11) > Created an attachment (id=135795) [details] > Patch Removed fast/filesystem/flags-passing.html in Skipped list. Because the test has a bug and it's resolved in bug 83137.

*** Bug 82853 has been marked as a duplicate of this bug. ***

*** Bug 82855 has been marked as a duplicate of this bug. ***

Comment on attachment 135795 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=135795&action=review OK, I'm starting to grasp this code a bit more. I still need to do some more reading of the spec and understand the interaction with ScriptController, I'll probably be able to do it over the weekend, I have a few comments for now: > Source/WebCore/Modules/filesystem/LocalFileSystem.cpp:55 > +#if PLATFORM(GTK) > +#include "AsyncFileSystemCallbacksGtk.h" > +#endif > + We're essentially replacing the whole file for GTK+, I don't see any reason to use #if PLATFORM(GTK) in this file at all, better do like Chromium: http://trac.webkit.org/browser/trunk/Source/WebCore/Modules/filesystem/LocalFileSystem.cpp#L34 > Source/WebCore/platform/gtk/AsyncFileSystemCallbacksGtk.cpp:3 > + * Copyright (C) 2012 ChangSeok Oh <shivamidow@gmail.com> This file seems to be essentially unchanged from Zan's patch, except for some code shuffling, so I think he should be the only copyright holder. > Source/WebCore/platform/gtk/AsyncFileSystemCallbacksGtk.h:3 > + * Copyright (C) 2012 ChangSeok Oh <shivamidow@gmail.com> Ditto. Please check for more such cases =) > Source/WebCore/platform/gtk/AsyncFileSystemGtk.cpp:34 > +#include "LocalFileSystem.h" > #include "NotImplemented.h" > Empty line, don't need NotImplemented.h anymore, too. > LayoutTests/platform/gtk/Skipped:304 > +# Following tests are passed, if FileSystem API feature is enabled. I think when we land this we should enable filesystem by default in build-webkit, so that it gets tested in the bots.

Created attachment 136219 [details] Patch

Comment on attachment 135795 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=135795&action=review Thank you for the review :) >> Source/WebCore/Modules/filesystem/LocalFileSystem.cpp:55 >> + > > We're essentially replacing the whole file for GTK+, I don't see any reason to use #if PLATFORM(GTK) in this file at all, better do like Chromium: http://trac.webkit.org/browser/trunk/Source/WebCore/Modules/filesystem/LocalFileSystem.cpp#L34 Done. >> Source/WebCore/platform/gtk/AsyncFileSystemCallbacksGtk.cpp:3 >> + * Copyright (C) 2012 ChangSeok Oh <shivamidow@gmail.com> > > This file seems to be essentially unchanged from Zan's patch, except for some code shuffling, so I think he should be the only copyright holder. O.K Acceptable :) I removed my copyright except LocalFileSystemGtk.cpp, AsyncFileSystemGtk.cpp, & AsyncFileWriterGtk.cpp >> Source/WebCore/platform/gtk/AsyncFileSystemCallbacksGtk.h:3 >> + * Copyright (C) 2012 ChangSeok Oh <shivamidow@gmail.com> > > Ditto. Please check for more such cases =) Done. >> Source/WebCore/platform/gtk/AsyncFileSystemGtk.cpp:34 >> > > Empty line, don't need NotImplemented.h anymore, too. Done. Removed. >> LayoutTests/platform/gtk/Skipped:304 >> +# Following tests are passed, if FileSystem API feature is enabled. > > I think when we land this we should enable filesystem by default in build-webkit, so that it gets tested in the bots. Done. Enabled file-system for gtk port in build-webkit. And also I removed passed tests here.

Comment on attachment 136219 [details] Patch Attachment 136219 [details] did not pass gtk-ews (gtk): Output: http://queues.webkit.org/results/12373209

Comment on attachment 136219 [details] Patch Attachment 136219 [details] did not pass gtk-ews (gtk): Output: http://queues.webkit.org/results/12373216

Created attachment 136422 [details] Patch

Comment on attachment 136422 [details] Patch Fixed build break for WK2

Comment on attachment 136422 [details] Patch Attachment 136422 [details] did not pass gtk-ews (gtk): Output: http://queues.webkit.org/results/12370989

(In reply to comment #22) > (From update of attachment 136422 [details]) > Attachment 136422 [details] did not pass gtk-ews (gtk): > Output: http://queues.webkit.org/results/12370989 It seems false alarm. I guess the build bot needs clean build.

Comment on attachment 136422 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=136422&action=review > Source/WebCore/platform/gtk/AsyncFileSystemCallbacksGtk.cpp:127 > +void AsyncFileSystemCallbacksGtk::didReadDirectoryEntries(bool hasMore) This hasMore parameter seems to be completely useless. It doesn't seem to be used outside of this class, and is used in the others because of the way they manage the list of directories that still need to be read is different. > Source/WebCore/platform/gtk/AsyncFileSystemCallbacksGtk.cpp:136 > + Vector<DirectoryEntry>::iterator end = m_directoryEntries.end(); > + for (Vector<DirectoryEntry>::iterator it = m_directoryEntries.begin(); it != end; ++it) > + m_callbacks->didReadDirectoryEntry(it->name, it->isDirectory); > + > + m_directoryEntries.clear(); This makes mse wonder. The way we're handling things is when we find a directory, we call didReadDirectoryEntry with isDirectory as true, and append the entry to m_directoryEntries. But we got the end iterator before the loop. I'm pretty sure the end iterator we got there won't be moved as new entries are added, so we will end up clearing them here and not reading them, am I missing something? Would using the strategy adopted by Chromium be any better? It seems simpler. > Source/WebCore/platform/gtk/AsyncFileSystemGtk.cpp:84 > + if (originString == String("null")) > + return String(); > + In addition to this check we should also have an ASSERT and check for type not being one we're expecting. > Source/WebCore/platform/gtk/AsyncFileSystemGtk.cpp:92 > + if (type() == Temporary) > + result.append(temporaryPathPrefix); > + if (type() == Persistent) > + result.append(persistentPathPrefix); This would be best as else if. You can use an ASSERT + empty return on the else clause if you prefer that instead of checking/asserting above. > Source/WebCore/platform/gtk/AsyncFileSystemGtk.cpp:103 > + GRefPtr<GFileInfo> sourceInfo = adoptGRef(g_file_query_info(sourceFile.get(), "*", G_FILE_QUERY_INFO_NONE, 0, 0)); We don't need to query all attributes if we're only going to use name, and type. Make this "standard::*". > Source/WebCore/platform/gtk/AsyncFileSystemGtk.cpp:109 > + GRefPtr<GFileInfo> destinationInfo = adoptGRef(g_file_query_info(destinationFile.get(), "*", G_FILE_QUERY_INFO_NONE, 0, 0)); Ditto. > Source/WebCore/platform/gtk/AsyncFileSystemGtk.cpp:115 > + helperCallbacks->didFail(FileError::INVALID_MODIFICATION_ERR); Would type mismatch make more sense here? http://dev.w3.org/2009/dap/file-system/file-dir-sys.html#dfn-typemismatcherror > Source/WebCore/platform/gtk/AsyncFileSystemGtk.cpp:120 > + helperCallbacks->didFail(FileError::INVALID_MODIFICATION_ERR); How about PathExistsError for this one? > Source/WebCore/platform/gtk/AsyncFileSystemGtk.cpp:126 > + helperCallbacks->didFail(FileError::NOT_FOUND_ERR); This would be much better if we actually used the error to check what happened and were able to report a more informative error. > Source/WebCore/platform/gtk/AsyncFileSystemGtk.cpp:147 > + GOwnPtr<GDir> directory(g_dir_open(g_file_get_path(sourceDirectory), 0, error)); This does not use GIO... I am here pondering if that may be problematic in the future, but I don't think there are any reasons to believe source/destination here would be outside of the main file system, is there? Still I would prefer if we used g_file_enumerate_children instead for this function, how's that? > Source/WebCore/platform/gtk/AsyncFileSystemGtk.cpp:155 > + GRefPtr<GFileInfo> sourceInfo = adoptGRef(g_file_query_info(sourceFile.get(), "*", G_FILE_QUERY_INFO_NONE, 0, error)); Looks like we only need G_FILE_ATTRIBUTE_STANDARD_TYPE here. > Source/WebCore/platform/gtk/AsyncFileSystemGtk.cpp:160 > + return; Hmm. So we found a directory, then we copy recursively and return? This would make no sense if we're trying to copy everything. You probably forgot to do the error check before returning? > Source/WebCore/platform/gtk/AsyncFileSystemGtk.cpp:179 > + GRefPtr<GFileInfo> destinationInfo = adoptGRef(g_file_query_info(destinationFile.get(), "*", G_FILE_QUERY_INFO_NONE, 0, 0)); G_FILE_ATTRIBUTE_STANDARD_TYPE seems to be enough here as well. > Source/WebCore/platform/gtk/AsyncFileSystemGtk.cpp:191 > + if (destinationInfo) { > + GFileType destinationFileType = g_file_info_get_file_type(destinationInfo.get()); > + if (sourceFileType != destinationFileType) { > + helperCallbacks->didFail(FileError::INVALID_MODIFICATION_ERR); > + return; > + } > + if (destinationFileType == G_FILE_TYPE_DIRECTORY > + && !g_file_delete(destinationFile.get(), 0, 0)) { > + helperCallbacks->didFail(FileError::INVALID_MODIFICATION_ERR); > + return; > + } > + } This is the same as the code above, the same comments apply, and consider making this a helper function, I think. > Source/WebCore/platform/gtk/AsyncFileSystemGtk.cpp:197 > + helperCallbacks->didFail(FileError::NOT_FOUND_ERR); Better error reporting would be great =) maybe have a helper function that translates GIOErrors to FileErrors. > Source/WebCore/platform/gtk/AsyncFileSystemGtk.cpp:223 > + GOwnPtr<GDir> directory(g_dir_open(g_file_get_path(file), 0, error)); Same here, enumerate children perhaps? > Source/WebCore/platform/gtk/AsyncFileSystemGtk.cpp:252 > + if (error) { > + FileError::ErrorCode errorCode; > + switch (error->code) { > + case G_IO_ERROR_NOT_EMPTY: > + errorCode = FileError::INVALID_MODIFICATION_ERR; > + break; > + case G_IO_ERROR_NOT_FOUND: > + default: > + errorCode = FileError::NOT_FOUND_ERR; > + } That's more like it =). Here's the seed for our helper function! > Source/WebCore/platform/gtk/AsyncFileSystemGtk.cpp:280 > + GRefPtr<GFileInfo> fileInfo = adoptGRef(g_file_query_info(file.get(), "*", G_FILE_QUERY_INFO_NONE, 0, 0)); "standard::*,time::*" looks right for here. > Source/WebCore/platform/gtk/AsyncFileSystemGtk.cpp:313 > + if (type == G_FILE_TYPE_REGULAR && absolutePath == String("/")) { > + helperCallbacks->didFail(FileError::SECURITY_ERR); > + return; > + } Why do we need this check? Is this denying creating the root directory (of the filesystem hierarchy)? Wouldn't that be checked at a higher level? > Source/WebCore/platform/gtk/AsyncFileSystemGtk.cpp:328 > + errorCode = FileError::INVALID_MODIFICATION_ERR; PathExistsError seems to be a better choice here. (http://dev.w3.org/2009/dap/file-system/file-dir-sys.html#dfn-pathexistserror) > Source/WebCore/platform/gtk/AsyncFileSystemGtk.cpp:362 > + GRefPtr<GFileInfo> fileInfo = adoptGRef(g_file_query_info(object.get(), "*", G_FILE_QUERY_INFO_NONE, 0, 0)); G_FILE_ATTRIBUTE_STANDARD_TYPE > Source/WebCore/platform/gtk/AsyncFileSystemGtk.cpp:400 > + GRefPtr<GFileInfo> fileInfo = adoptGRef(g_file_query_info(child.get(), "*", G_FILE_QUERY_INFO_NONE, 0, 0)); Ditto. > Source/WebCore/platform/gtk/AsyncFileSystemGtk.cpp:408 > + } else { > + callbacks->didFail(FileError::NOT_FOUND_ERR); > + return; > + } Instead of checking for !error it would be better to check for error and remove the indentation for the normal case. if (error) { callbacks->didFail(FileError::NOT_FOUND_ERR); return; } ...rest of code here ... > Source/WebCore/platform/gtk/AsyncFileSystemGtk.cpp:424 > + GRefPtr<GFileInfo> fileInfo = adoptGRef(g_file_query_info(file.get(), "*", G_FILE_QUERY_INFO_NONE, 0, 0)); G_FILE_ATTRIBUTE_STANDARD_SIZE > Source/WebCore/platform/gtk/AsyncFileSystemGtk.cpp:446 > +void AsyncFileSystemGtk::createSnapshotFileAndReadMetadata(const String& path, PassOwnPtr<AsyncFileSystemCallbacks> callbacks) > +{ > + // FIXME: Is this api really same with readMetadata? > + readMetadata(path, callbacks); > +} I'm pretty sure it isn't. File creation is certainly involved here. It seems to be called by DOMFileSystemSync::createFile and DOMFileSystem::createFile. The tests we have do not exercise this? > Source/WebCore/platform/gtk/AsyncFileSystemTaskControllerGtk.h:64 > +#endif // AsyncFileSystemCallbacks_h Name is wrong here. > LayoutTests/platform/gtk/Skipped:287 > +# Passed but we should enable file-system & mutation-observers at the same time. Anything stops us from doing that? =)

The good news is the patch build ok in a clean build here. =)

Comment on attachment 136422 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=136422&action=review Thanks for the your kind review! >> Source/WebCore/platform/gtk/AsyncFileSystemCallbacksGtk.cpp:127 >> +void AsyncFileSystemCallbacksGtk::didReadDirectoryEntries(bool hasMore) > > This hasMore parameter seems to be completely useless. It doesn't seem to be used outside of this class, and is used in the others because of the way they manage the list of directories that still need to be read is different. Removed it. >> Source/WebCore/platform/gtk/AsyncFileSystemCallbacksGtk.cpp:136 >> + m_directoryEntries.clear(); > > This makes mse wonder. The way we're handling things is when we find a directory, we call didReadDirectoryEntry with isDirectory as true, and append the entry to m_directoryEntries. But we got the end iterator before the loop. I'm pretty sure the end iterator we got there won't be moved as new entries are added, so we will end up clearing them here and not reading them, am I missing something? Would using the strategy adopted by Chromium be any better? It seems simpler. Done like chromium's way. >> Source/WebCore/platform/gtk/AsyncFileSystemGtk.cpp:84 >> + > > In addition to this check we should also have an ASSERT and check for type not being one we're expecting. Inserted ASSERT in below else clause. >> Source/WebCore/platform/gtk/AsyncFileSystemGtk.cpp:92 >> + result.append(persistentPathPrefix); > > This would be best as else if. You can use an ASSERT + empty return on the else clause if you prefer that instead of checking/asserting above. Done. >> Source/WebCore/platform/gtk/AsyncFileSystemGtk.cpp:103 >> + GRefPtr<GFileInfo> sourceInfo = adoptGRef(g_file_query_info(sourceFile.get(), "*", G_FILE_QUERY_INFO_NONE, 0, 0)); > > We don't need to query all attributes if we're only going to use name, and type. Make this "standard::*". Replaced the way how we get the file type, with using g_file_query_file_type. >> Source/WebCore/platform/gtk/AsyncFileSystemGtk.cpp:109 >> + GRefPtr<GFileInfo> destinationInfo = adoptGRef(g_file_query_info(destinationFile.get(), "*", G_FILE_QUERY_INFO_NONE, 0, 0)); > > Ditto. Ditto. >> Source/WebCore/platform/gtk/AsyncFileSystemGtk.cpp:115 >> + helperCallbacks->didFail(FileError::INVALID_MODIFICATION_ERR); > > Would type mismatch make more sense here? http://dev.w3.org/2009/dap/file-system/file-dir-sys.html#dfn-typemismatcherror Yeh. It looks reasonable but failed on fast/filesystem/op-move.html if we do like that. It might be an issue of the testcase itself. I think we need to discuss with the original author of it more. >> Source/WebCore/platform/gtk/AsyncFileSystemGtk.cpp:120 >> + helperCallbacks->didFail(FileError::INVALID_MODIFICATION_ERR); > > How about PathExistsError for this one? Ditto. >> Source/WebCore/platform/gtk/AsyncFileSystemGtk.cpp:126 >> + helperCallbacks->didFail(FileError::NOT_FOUND_ERR); > > This would be much better if we actually used the error to check what happened and were able to report a more informative error. Done. I added a new function toFileErrorCode which converts GError to FileError to check what happened. >> Source/WebCore/platform/gtk/AsyncFileSystemGtk.cpp:147 >> + GOwnPtr<GDir> directory(g_dir_open(g_file_get_path(sourceDirectory), 0, error)); > > This does not use GIO... I am here pondering if that may be problematic in the future, but I don't think there are any reasons to believe source/destination here would be outside of the main file system, is there? Still I would prefer if we used g_file_enumerate_children instead for this function, how's that? I think it's a better approach. Done :) >> Source/WebCore/platform/gtk/AsyncFileSystemGtk.cpp:155 >> + GRefPtr<GFileInfo> sourceInfo = adoptGRef(g_file_query_info(sourceFile.get(), "*", G_FILE_QUERY_INFO_NONE, 0, error)); > > Looks like we only need G_FILE_ATTRIBUTE_STANDARD_TYPE here. We're going to use g_file_query_file_type glib utility function to just get the file type. >> Source/WebCore/platform/gtk/AsyncFileSystemGtk.cpp:160 >> + return; > > Hmm. So we found a directory, then we copy recursively and return? This would make no sense if we're trying to copy everything. You probably forgot to do the error check before returning? Right. It looks there is a logical bug. Done. >> Source/WebCore/platform/gtk/AsyncFileSystemGtk.cpp:179 >> + GRefPtr<GFileInfo> destinationInfo = adoptGRef(g_file_query_info(destinationFile.get(), "*", G_FILE_QUERY_INFO_NONE, 0, 0)); > > G_FILE_ATTRIBUTE_STANDARD_TYPE seems to be enough here as well. Done. We're going to use g_file_query_file_type to get the file type. >> Source/WebCore/platform/gtk/AsyncFileSystemGtk.cpp:191 >> + } > > This is the same as the code above, the same comments apply, and consider making this a helper function, I think. Done. Added a helper function, validateFileType. >> Source/WebCore/platform/gtk/AsyncFileSystemGtk.cpp:197 >> + helperCallbacks->didFail(FileError::NOT_FOUND_ERR); > > Better error reporting would be great =) maybe have a helper function that translates GIOErrors to FileErrors. Done with adding toFileErrorCode. >> Source/WebCore/platform/gtk/AsyncFileSystemGtk.cpp:223 >> + GOwnPtr<GDir> directory(g_dir_open(g_file_get_path(file), 0, error)); > > Same here, enumerate children perhaps? Done. >> Source/WebCore/platform/gtk/AsyncFileSystemGtk.cpp:252 >> + } > > That's more like it =). Here's the seed for our helper function! Yep. >> Source/WebCore/platform/gtk/AsyncFileSystemGtk.cpp:280 >> + GRefPtr<GFileInfo> fileInfo = adoptGRef(g_file_query_info(file.get(), "*", G_FILE_QUERY_INFO_NONE, 0, 0)); > > "standard::*,time::*" looks right for here. Done. >> Source/WebCore/platform/gtk/AsyncFileSystemGtk.cpp:313 >> + } > > Why do we need this check? Is this denying creating the root directory (of the filesystem hierarchy)? Wouldn't that be checked at a higher level? We should prevent creating a new file in upper directory than baseFileSystemPath(logically '/'). Some users (maybe contents) may try to access upper directory by using relative path like '..'. But we should not allow it becuase of the security issue I think. If that kind of invalid path comes in, a higher level (DOMFileSystemBase) converts it to '/', so we here get '/'. If we don't deal with it here. we might face some failures. >> Source/WebCore/platform/gtk/AsyncFileSystemGtk.cpp:328 >> + errorCode = FileError::INVALID_MODIFICATION_ERR; > > PathExistsError seems to be a better choice here. (http://dev.w3.org/2009/dap/file-system/file-dir-sys.html#dfn-pathexistserror) I think it needs more discussion. :\ >> Source/WebCore/platform/gtk/AsyncFileSystemGtk.cpp:362 >> + GRefPtr<GFileInfo> fileInfo = adoptGRef(g_file_query_info(object.get(), "*", G_FILE_QUERY_INFO_NONE, 0, 0)); > > G_FILE_ATTRIBUTE_STANDARD_TYPE Replaced with g_file_query_file_type. >> Source/WebCore/platform/gtk/AsyncFileSystemGtk.cpp:400 >> + GRefPtr<GFileInfo> fileInfo = adoptGRef(g_file_query_info(child.get(), "*", G_FILE_QUERY_INFO_NONE, 0, 0)); > > Ditto. Done. >> Source/WebCore/platform/gtk/AsyncFileSystemGtk.cpp:408 >> + } > > Instead of checking for !error it would be better to check for error and remove the indentation for the normal case. > > if (error) { > callbacks->didFail(FileError::NOT_FOUND_ERR); > return; > } > > ...rest of code here ... Done. >> Source/WebCore/platform/gtk/AsyncFileSystemGtk.cpp:424 >> + GRefPtr<GFileInfo> fileInfo = adoptGRef(g_file_query_info(file.get(), "*", G_FILE_QUERY_INFO_NONE, 0, 0)); > > G_FILE_ATTRIBUTE_STANDARD_SIZE Done. >> Source/WebCore/platform/gtk/AsyncFileSystemGtk.cpp:446 >> +} > > I'm pretty sure it isn't. File creation is certainly involved here. It seems to be called by DOMFileSystemSync::createFile and DOMFileSystem::createFile. The tests we have do not exercise this? Done. There is a test for this, but it works without any problem. :p I guess the already existing file affects the test case result. My current implementation might not be perfect now since less information what this API does. Let me watch this... >> Source/WebCore/platform/gtk/AsyncFileSystemTaskControllerGtk.h:64 >> +#endif // AsyncFileSystemCallbacks_h > > Name is wrong here. Done >> LayoutTests/platform/gtk/Skipped:287 >> +# Passed but we should enable file-system & mutation-observers at the same time. > > Anything stops us from doing that? =) Removed as well. :)

Created attachment 137858 [details] Patch

Comment on attachment 137858 [details] Patch This might require clean build.

Attachment 137858 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/plat..." exit_code: 1 LayoutTests/platform/gtk/test_expectations.txt:366: Duplicate or ambiguous expectation. editing/undo/undo-smart-delete-reversed-selection.html [test/expectations] [5] Total errors found: 1 in 22 files If any of these errors are false positives, please file a bug against check-webkit-style.

Created attachment 137860 [details] Patch

Attachment 137860 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/plat..." exit_code: 1 LayoutTests/platform/gtk/test_expectations.txt:366: Duplicate or ambiguous expectation. editing/undo/undo-smart-delete-reversed-selection.html [test/expectations] [5] Total errors found: 1 in 22 files If any of these errors are false positives, please file a bug against check-webkit-style.

Created attachment 137861 [details] Patch

Comment on attachment 137861 [details] Patch Removed a duplicated test above.

Comment on attachment 137861 [details] Patch Attachment 137861 [details] did not pass gtk-ews (gtk): Output: http://queues.webkit.org/results/12297248

(In reply to comment #26) > > I'm pretty sure it isn't. File creation is certainly involved here. It seems to be called by DOMFileSystemSync::createFile and DOMFileSystem::createFile. The tests we have do not exercise this? > > Done. There is a test for this, but it works without any problem. :p I guess the already existing file affects the test case result. My current implementation might not be perfect now since less information what this API does. Let me watch this... What test tests this?

Comment on attachment 137861 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=137861&action=review r- just because I'd like to be sure we know what we're doing at createObjectAndReadMetadataAsync, so I think we need another iteration. > Source/WebCore/GNUmakefile.list.am:4999 > +webcore_sources += \ > Source/WebCore/platform/gtk/AsyncFileSystemGtk.cpp \ > Source/WebCore/platform/gtk/AsyncFileSystemGtk.h Why did you move these to webcore_sources? > Source/WebCore/platform/gtk/AsyncFileSystemCallbacksGtk.cpp:131 > +void AsyncFileSystemCallbacksGtk::didReadDirectoryEntries(bool) > +{ > + for (size_t i = 0; i < m_directoryEntries.size(); ++i) > + m_callbacks->didReadDirectoryEntry(m_directoryEntries[i].name, m_directoryEntries[i].isDirectory); > + m_directoryEntries.clear(); > + > + m_taskController->postCallbackTask(createCallbackTask(&performDidReadDirectoryEntriesAndFinishedCallback, m_callbacks.release()), m_taskMode); > +} I think you addressed my concern by removing the recursive listing, now you're not even trying to read the directories that are found. Is that acceptable? It's starting to look like it's what's intended from reading the spec and the code. The spec indicates a call to readEntries returns the 'next block' of entries, which is why there's a hasMore boolean here - you're not expected to return all entries at once. We may want to look into that, but I don't think it's very important right now: http://www.w3.org/TR/file-system-api/#the-directoryreader-interface > Source/WebCore/platform/gtk/AsyncFileSystemGtk.cpp:65 > +static FileError::ErrorCode validateFileType(GFile* sourceFile, GFile* destinationFile) validateFileTypes sounds like a better choice > Source/WebCore/platform/gtk/AsyncFileSystemGtk.cpp:242 > + if (recursive && g_file_test(filePath.get(), G_FILE_TEST_IS_DIR)) { We can use query_file_type here too, instead of testing the path. > Source/WebCore/platform/gtk/AsyncFileSystemGtk.cpp:329 > + ASSERT(type == G_FILE_TYPE_REGULAR || type == G_FILE_TYPE_DIRECTORY); > + // We should not allow to access upper directory to create a new file than base directory > + // because of the security issue. > + if (type == G_FILE_TYPE_REGULAR && absolutePath == String("/")) { > + helperCallbacks->didFail(FileError::SECURITY_ERR); > + return; > + } I understand the security implications but I am still really doubtful we need this check. Wouldn't higher level stuff check for someone trying to create /? The comment as it is is not very useful, I would prefer if it explained why we need to do this check here. > Source/WebCore/platform/gtk/AsyncFileSystemGtk.cpp:342 > + if (error) { > + if (!needToReadMetadata || error->code != G_IO_ERROR_EXISTS) { > + helperCallbacks->didFail(toFileErrorCode(error.get())); > + return; > + } > + } I am not sure I understand. This is very convoluted. Why do we only complain about the error if it's not a file exists? We should make all of these checks in 1 if, in any case. > Source/WebCore/platform/gtk/AsyncFileSystemGtk.cpp:348 > + helperCallbacks->didSucceed(); I believe you need to report success also after reading metadata.

Created attachment 138384 [details] Patch

Comment on attachment 137861 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=137861&action=review Thank you for your thoughtful review. :) >> Source/WebCore/GNUmakefile.list.am:4999 >> Source/WebCore/platform/gtk/AsyncFileSystemGtk.h > > Why did you move these to webcore_sources? Because of build-break for WK2. AsyncFileSystemGtk.cpp uses temporaryPathPrefix & persistentPathPrefix, but they are defined in AsyncFileSystem.cpp that included in webcore_sources. >> Source/WebCore/platform/gtk/AsyncFileSystemCallbacksGtk.cpp:131 >> +} > > I think you addressed my concern by removing the recursive listing, now you're not even trying to read the directories that are found. Is that acceptable? It's starting to look like it's what's intended from reading the spec and the code. The spec indicates a call to readEntries returns the 'next block' of entries, which is why there's a hasMore boolean here - you're not expected to return all entries at once. We may want to look into that, but I don't think it's very important right now: http://www.w3.org/TR/file-system-api/#the-directoryreader-interface > I think you addressed my concern by removing the recursive listing, now you're not even trying to read the directories that are found. Is that acceptable? In my humble opinion, yes it is. We don't need to actually read directory & do something here. we just gather directory entries and return them to JS layer. That's enough. :) > The spec indicates a call to readEntries returns the 'next block' of entries, which is why there's a hasMore boolean here - you're not expected to return all entries at once. I think there is no problem too, though we return all directory entries at once. The spec is just saying that readEntries might not return all entries for a single call, so users need to check if the returned entries is empty array. In our case, we can return all entries at once and then return empty block, so it seems suitable to the spec. But I'm still not understand clearly what the situation is when we need to read directories more here. I guess we may need to check 'hasmore' for multithread model(workers)... >> Source/WebCore/platform/gtk/AsyncFileSystemGtk.cpp:65 >> +static FileError::ErrorCode validateFileType(GFile* sourceFile, GFile* destinationFile) > > validateFileTypes sounds like a better choice Done. >> Source/WebCore/platform/gtk/AsyncFileSystemGtk.cpp:242 >> + if (recursive && g_file_test(filePath.get(), G_FILE_TEST_IS_DIR)) { > > We can use query_file_type here too, instead of testing the path. Done. >> Source/WebCore/platform/gtk/AsyncFileSystemGtk.cpp:329 >> + } > > I understand the security implications but I am still really doubtful we need this check. Wouldn't higher level stuff check for someone trying to create /? The comment as it is is not very useful, I would prefer if it explained why we need to do this check here. I've updated the comment but couldn't find a better place for this. :\ >> Source/WebCore/platform/gtk/AsyncFileSystemGtk.cpp:342 >> + } > > I am not sure I understand. This is very convoluted. Why do we only complain about the error if it's not a file exists? We should make all of these checks in 1 if, in any case. Done. >> Source/WebCore/platform/gtk/AsyncFileSystemGtk.cpp:348 >> + helperCallbacks->didSucceed(); > > I believe you need to report success also after reading metadata. Actually we don't need to call didSucceed for reading metadata. didReadMetadata works similar to didSucceed for readMetadata api. You can refer the comment in AsyncFileSystem.h. :) It says.. > AsyncFileSystemCallbacks::didReadMetadata() is called when the operation is completed successfully. AsyncFileSystemCallbacks::didFail() is called otherwise.

Comment on attachment 138384 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=138384&action=review This patch seems a bit too big to review at once. Why not try splitting it up? Since the API is divided into operations it makes sense to implement one per patch and unskip tests that pass with each iteration. One thing that I wonder about looking at this initially is the threading behavior. Instead of actually using the asychronous GIO APIs, the operations are just posted back to the main thread. That seems wrong -- unless this is supposed to be called from workers and all operations need to be on the main thread. Is that the case? If so perhaps it would be better to have a "filesystem thread" so that these operations don't block the main thread. > Source/WTF/wtf/gobject/GOwnPtr.cpp:54 > +template <> void freeOwnedGPtr<GOutputStream>(GOutputStream* ptr) > +{ > + g_output_stream_close(ptr, 0, 0); > +} > + GOutputStream is a reference counted GObject, so you should not use GOwnPtr to manage it. g_output_stream_close doesn't free a GOuputStream, only the internal bits.

(In reply to comment #39) > (From update of attachment 138384 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=138384&action=review > > This patch seems a bit too big to review at once. Why not try splitting it up? Since the API is divided into operations it makes sense to implement one per patch and unskip tests that pass with each iteration. Hmm.. I want to hear kov's thought. > One thing that I wonder about looking at this initially is the threading behavior. Instead of actually using the asychronous GIO APIs, the operations are just posted back to the main thread. That seems wrong -- unless this is supposed to be called from workers and all operations need to be on the main thread. Is that the case? If so perhaps it would be better to have a "filesystem thread" so that these operations don't block the main thread. Reasonable. How about sharing fileThread that is used by BLOB?

(In reply to comment #40) > Reasonable. How about sharing fileThread that is used by BLOB? What do other ports do?

(In reply to comment #41) > (In reply to comment #40) > > > Reasonable. How about sharing fileThread that is used by BLOB? > > What do other ports do? Another important question: Does the spec specify a certain behavior?

(In reply to comment #41) > (In reply to comment #40) > > > Reasonable. How about sharing fileThread that is used by BLOB? > > What do other ports do? Hard to know. It seems Chromium & Blackberry has the feature, but the code is not upstream yet. The only clue I got was LocalFileSystem.cpp uses "context->postTask ", so I believe other ports use main thread.

(In reply to comment #42) > (In reply to comment #41) > > (In reply to comment #40) > > > > > Reasonable. How about sharing fileThread that is used by BLOB? > > > > What do other ports do? > > Another important question: Does the spec specify a certain behavior? Nothing special related with asynchronous behavior, as I know. http://www.w3.org/TR/file-system-api/#the-asynchronous-filesystem-interface.

From this part of the spec it seems like these operations should not be on the main thread: > There are several ways in which a file system entry point can be obtained. Not all user agents may in > fact implement all of them. However, in order to avoid blocking UI actions while waiting on > filesystem IO, we define only an asynchronous interface for Window, and restrict the synchronous API > to the Worker context defined in

I like Martin's idea of splitting the patch per operation, it is a great idea. What I do not agree with is just stubbing everything, that makes it impossible to do a meaninful review as well. About the threading issue, I understood it was being handled by having a thread be spawned by webcore and only the result would be posted to the main thread, isn't that correct?

(In reply to comment #46) > I like Martin's idea of splitting the patch per operation, it is a great idea. What I do not agree with is just stubbing everything, that makes it impossible to do a meaninful review as well. About the threading issue, I understood it was being handled by having a thread be spawned by webcore and only the result would be posted to the main thread, isn't that correct? I read the code and the spec a little and here's what I think is happening: There are two ways of using the filesystem API, synchronously and asynchronously. Currently, the argument that determines whether the filesystem is asynchronous is ignored. See the call to LocalFileSystem::requestFileSystem in WorkerContextFileSystem::webkitRequestFileSystemSync and note that the "synchronous" parameter is ignored completely. So before this patch can be reviewed, we need to determine (probably just by asking around) whether operations should be synchronous or asynchronous. My guess is that they should be asynchronous and that the synchronous version of the API will just block until the callback is called. If that's the case, this patch shouldn't be doing operations synchronously and definitely should not be doing them on the main thread. This API can be used from a worker thread. An easy way to accomplish this is to simply use the asychronous versions of the GIO APIs.

Hi, first off thanks for taking up the work here and sorry for such a late reply. (In reply to comment #47) > There are two ways of using the filesystem API, synchronously and asynchronously. Currently, the argument that determines whether the filesystem is asynchronous is ignored. See the call to LocalFileSystem::requestFileSystem in WorkerContextFileSystem::webkitRequestFileSystemSync and note that the "synchronous" parameter is ignored completely. The synchronous methods are intended to be used with workers only. > So before this patch can be reviewed, we need to determine (probably just by asking around) whether operations should be synchronous or asynchronous. My guess is that they should be asynchronous and that the synchronous version of the API will just block until the callback is called. Operations (preferably GIO functions calls) should definitely be all asynchronous. The FileSystem API expects that all these operations are done via ScriptExecutionContext. This can be either a Document or a WorkerContext. If the former, calling scriptExecutionContext->postTask(task) will post the task on the main thread, if the latter, doing that will post the task on the worker thread. To use a unified approach for both cases the operations should all be asynchronous as, as said, you'd hate to clog up the main thread. When using a synchronous version of the FileSystem API (through workers) all the calls should still be asynchronous because when using that version the SyncCallbackHelpers[1] are used that basically wait until the operation is complete (either succeeds or fails, producing an error). > If that's the case, this patch shouldn't be doing operations synchronously and definitely should not be doing them on the main thread. This API can be used from a worker thread. An easy way to accomplish this is to simply use the asychronous versions of the GIO APIs. While I was still trying to get things working now almost a year back, the main problem I was facing was keeping the AsyncFileSystemCallbacks objects around long enough for the asynchronous operation to complete so the callbacks could be triggered. Just to top it off I was trying for every operation being as asynchronous as possible - from GIO operations to even the callbacks being fired asynchronously. [1] http://trac.webkit.org/browser/trunk/Source/WebCore/Modules/filesystem/SyncCallbackHelper.h

*** Bug 50948 has been marked as a duplicate of this bug. ***

For what it's worth, the File API: Directories and System specification[1] is being considered for a move to the Note track[2]. Translated, it means the work on this particular specification will probably cease. There are already considerations of a similar but simplified approach though[3][4]. [1] http://dev.w3.org/2009/dap/file-system/file-dir-sys.html [2] http://lists.w3.org/Archives/Public/public-webapps/2012JulSep/0791.html [3] http://lists.w3.org/Archives/Public/public-webapps/2012JulSep/0823.html [4] http://lists.w3.org/Archives/Public/public-webapps/2012JulSep/0827.html

(In reply to comment #50) > For what it's worth, the File API: Directories and System specification[1] is being considered for a move to the Note track[2]. > > Translated, it means the work on this particular specification will probably cease. There are already considerations of a similar but simplified approach though[3][4]. > > [1] http://dev.w3.org/2009/dap/file-system/file-dir-sys.html > [2] http://lists.w3.org/Archives/Public/public-webapps/2012JulSep/0791.html > [3] http://lists.w3.org/Archives/Public/public-webapps/2012JulSep/0823.html > [4] http://lists.w3.org/Archives/Public/public-webapps/2012JulSep/0827.html I don't think it is worthless. Because, there are already some web applications use this File System API to read/write files. And, there is no other way to do this kind of things until new file system spec is announced. So, I think still it is valuable to implement this File System API in WebKit. What do you think?

(In reply to comment #51) > I don't think it is worthless. > > Because, there are already some web applications use this File System API to read/write files. > And, there is no other way to do this kind of things until new file system spec is announced. > > So, I think still it is valuable to implement this File System API in WebKit. > > What do you think? I'd agree it isn't worthless, the specification is not yet confirmed to be put on the Note track and there are indeed applications that use it. Furthermore the code used here might as well be reusable when implementing the future file system specification (if there'll actually be one). Despite all that, enabling this feature for the GTK port was on my personal long-term todo list but I won't really be picking the work up until the specification status clears up (though there might be someone else that will do the work, of which I'm totally supportive).

Not sure this is going to happen. :/