Bug 37218

Created attachment 52755 [details] Patch

Comment on attachment 52755 [details] Patch The patch depends on another patch for bug 37217.

Created attachment 52761 [details] Patch

Comment on attachment 52761 [details] Patch > --- /dev/null > +++ b/WebCore/html/FileStreamProxy.cpp > +#include "FileThread.h" > +#include "FileThreadTask.h" This include should be placed above so that it will be in the alphabetical order. > FileStreamProxy::~FileStreamProxy() > { > if (fileThread()) > fileThread()->unscheduleTasks(m_stream.get()); Is it possible that m_context is destructed before FileStreamProxy instance? > +void FileStreamProxy::read(char* buffer, int length) > +{ > + fileThread()->postTask(createFileThreadTask(m_stream.get(), &FileStream::read, buffer, length)); > +} > + > +void FileStreamProxy::write(Blob* blob, long long position, size_t length) We should use either size_t or int for length in read() and write(), for consistency. > +FileThread* FileStreamProxy::fileThread() > +{ > + ASSERT(m_context->fileThread()); // FIXME What is this FIXME about? > + return m_context->fileThread(); > +} > +void FileStreamProxy::abort() > +{ > + if (fileThread()) Do we really need to check for fileThread()? > + fileThread()->unscheduleTasks(m_stream.get()); > +} > +static void notifyWriteOnContext(ScriptExecutionContext* context, FileStreamProxy* stream, int nwritten) Better to name it as bytesWritten.

Created attachment 52817 [details] Patch

(In reply to comment #4) > (From update of attachment 52761 [details]) > > --- /dev/null > > +++ b/WebCore/html/FileStreamProxy.cpp > > +#include "FileThread.h" > > +#include "FileThreadTask.h" > This include should be placed above so that it will be in the alphabetical > order. The order is enforced by check-webkit-style, so I assume the current order is ok. > > FileStreamProxy::~FileStreamProxy() > > { > > if (fileThread()) > > fileThread()->unscheduleTasks(m_stream.get()); > Is it possible that m_context is destructed before FileStreamProxy instance? I fixed some refs/destructor issues. The new code assumes the object that holds a reference to FileStreamProxy calls FileStreamProxy::stop() in its destructor to safely shutdown the context/thread. > > +void FileStreamProxy::write(Blob* blob, long long position, size_t length) > We should use either size_t or int for length in read() and write(), for > consistency. Changed size_t to int. > > +FileThread* FileStreamProxy::fileThread() > > +{ > > + ASSERT(m_context->fileThread()); // FIXME > What is this FIXME about? If thread creation fails it will cause SEGV... but currently I have no idea to 'fix' it, so I removed the comment. > > +static void notifyWriteOnContext(ScriptExecutionContext* context, FileStreamProxy* stream, int nwritten) > Better to name it as bytesWritten. Done.

Created attachment 52883 [details] Patch

Made one more fix (added fileThread()->unscheduleTasks() in FileStream::stop()).

Comment on attachment 52883 [details] Patch > diff --git a/WebCore/html/FileStreamProxy.cpp b/WebCore/html/FileStreamProxy.cpp > new file mode 100644 > index 0000000..59abd68 We should not have the above info in the patch. Otherwise, you will make svn think it is branched from other existing file, like previously landed patch: http://trac.webkit.org/changeset/57229 > +void FileStreamProxy::abort() > +{ > + fileThread()->unscheduleTasks(m_stream.get()); > +} > + > +void FileStreamProxy::stop() > +{ > + fileThread()->unscheduleTasks(m_stream.get()); > + fileThread()->postTask(createFileThreadTask(m_stream.get(), &FileStream::stop)); > +} It seems to be quite confusing with both abort() and stop() and they seem to do the similar things. For stop(), you call unscheduleTasks to remove all file thread tasks and then call postTask to add a new one. This seems to be a bit unnatural. Can you illustrate how the whole destruction logic work? > +static void notifyGetSizeOnContext(ScriptExecutionContext* context, FileStreamProxy* proxy, long long size) > +{ > + ASSERT(context->isContextThread()); Probably this ASSERT is really not needed. I think it would be better to have ASSERTs on did*** methods to ensure they're called from file thread; and ASSERTS on open/read/write/... methods to ensure they're called from non-file thread. > +void FileStreamProxy::didStop() > +{ > + m_context->postTask(createCallbackTask(&notifyStopOnContext, this)); > +} > + > +void FileStreamProxy::didStopAndDestroy() didStop() is called on file thread and didStopAndDestroy is called on context thread and both of them are prefixed with "did", which increases the difficulty to understand.

(In reply to comment #9) > (From update of attachment 52883 [details]) > > diff --git a/WebCore/html/FileStreamProxy.cpp b/WebCore/html/FileStreamProxy.cpp > > new file mode 100644 > > index 0000000..59abd68 > We should not have the above info in the patch. Otherwise, you will make svn > think it > is branched from other existing file, like previously landed patch: > http://trac.webkit.org/changeset/57229 Hmm they're done by webkit-patch. I'll look into how I can avoid that. > > +void FileStreamProxy::stop() > > +{ > > + fileThread()->unscheduleTasks(m_stream.get()); > > + fileThread()->postTask(createFileThreadTask(m_stream.get(), &FileStream::stop)); > > +} > It seems to be quite confusing with both abort() and stop() and they seem to do > the similar things. > For stop(), you call unscheduleTasks to remove all file thread tasks and then > call postTask > to add a new one. This seems to be a bit unnatural. Can you illustrate how the > whole destruction > logic work? Abort() is straightforward, it needs to stop any (pending) writes/reads, thus it unschedules tasks. it's just an API and by definition it doesn't have anything to do with destructing FileStreamProxy/FileStream. Stop() is for enforcing a destruction order and closing a file handle on the file thread. It shouldn't be called for other purpose. It might be written better, but the logic I thought is: 0. FileStreamProxy holds refs for ScriptExecutionContext and FileStream, so they must be available throughout the proxy's life cycle. 1. when we need to stop/destroy FileStreamProxy, we call FileStreamProxy::stop() and then deref the proxy. (e.g. m_streamProxy->stop(); m_streamProxy = 0;) FileStreamProxy holds its own ref so it won't be deleted until it really thinks it's done. 2. FileStreamProxy::stop() calls unscheduleTasks to remove any pending writes/reads (i.e. tasks still in the queue) for the stream. 3. FileStreamProxy::stop() posts a task to close the file and waits for the close (and any ongoing tasks) to be completed. 4. When FileStreamProxy::didStop() is called we can safely say that the file is closed and there're no ongoing/pending tasks that need call back to the proxy. 5. FileStreamProxy::didStopAndDestroy() derefs itself and the context. > > +static void notifyGetSizeOnContext(ScriptExecutionContext* context, FileStreamProxy* proxy, long long size) > > +{ > > + ASSERT(context->isContextThread()); > Probably this ASSERT is really not needed. I think it would be better to have > ASSERTs on did*** methods to > ensure they're called from file thread; and ASSERTS on open/read/write/... > methods to ensure they're called > from non-file thread. Sounds good. notifyXXX are all static methods and they are less likely to be called unexpectedly. > > +void FileStreamProxy::didStop() > > +{ > > + m_context->postTask(createCallbackTask(&notifyStopOnContext, this)); > > +} > > + > > +void FileStreamProxy::didStopAndDestroy() > didStop() is called on file thread and didStopAndDestroy is called on context > thread and both of them > are prefixed with "did", which increases the difficulty to understand. I've tried to name it better but haven't come up with a good idea.

Created attachment 52921 [details] Patch

Made a minor cleanup. Removed didStopAndDestroy method. The patch still has ' new file mode...' part. I'll try fixing it... > new file mode 100644 > index 0000000..59abd68

Comment on attachment 52921 [details] Patch I think the way to fix "new file mode..." problem in the patch is to create a new file from the scratch and then copy and paste the content over. You need to revert all new files just added and then create files and add them to the repository. > diff --git a/WebCore/html/FileStreamProxy.cpp b/WebCore/html/FileStreamProxy.cpp > +FileStreamProxy::FileStreamProxy(ScriptExecutionContext* context, FileStreamClient* client) > + : m_context(context) > + , m_client(client) > + , m_stream(FileStream::create(this)) > +{ > + ref(); Could you please add the comment about why we need to keep its own ref here? > +void FileStreamProxy::abort() > +{ > + fileThread()->unscheduleTasks(m_stream.get()); > +} > + > +void FileStreamProxy::stop() > +{ > + fileThread()->unscheduleTasks(m_stream.get()); > + fileThread()->postTask(createFileThreadTask(m_stream.get(), &FileStream::stop)); > +} I still think we do not need both abort() and stop(). Since we are not going to resume after abort, can we simply call stop() from FileReader/FieWriter::abort()? > diff --git a/WebCore/html/FileStreamProxy.h b/WebCore/html/FileStreamProxy.h > +class FileStreamProxy : public RefCounted<FileStreamProxy>, public FileStreamClient { > +protected: If we're not going to have other child classes derived from FileStreamProxy, it would be better to make it "private".

Created attachment 53006 [details] Patch

Created attachment 53009 [details] Patch

(In reply to comment #13) > (From update of attachment 52921 [details]) > I think the way to fix "new file mode..." problem in the patch is to create a > new file > from the scratch and then copy and paste the content over. You need to revert > all new > files just added and then create files and add them to the repository. I tried something similar to this. Hope it'll work this time. > > diff --git a/WebCore/html/FileStreamProxy.cpp b/WebCore/html/FileStreamProxy.cpp > > +FileStreamProxy::FileStreamProxy(ScriptExecutionContext* context, FileStreamClient* client) > > + ref(); > Could you please add the comment about why we need to keep its own ref here? Done. > > +void FileStreamProxy::stop() > > +{ > > + fileThread()->unscheduleTasks(m_stream.get()); > > + fileThread()->postTask(createFileThreadTask(m_stream.get(), &FileStream::stop)); > > +} > I still think we do not need both abort() and stop(). Since we are not going to > resume > after abort, can we simply call stop() from FileReader/FieWriter::abort()? OK, I removed the abort method and added some more comments (chose stop() over abort() just for consistency). > > diff --git a/WebCore/html/FileStreamProxy.h b/WebCore/html/FileStreamProxy.h > > +class FileStreamProxy : public RefCounted<FileStreamProxy>, public FileStreamClient { > > +protected: > If we're not going to have other child classes derived from FileStreamProxy, > it would be better to make it "private". Right, I've been missing to fix it from another merge... Done.

Comment on attachment 53009 [details] Patch Only some minor things to resolve. > diff --git a/WebCore/html/FileStreamProxy.cpp b/WebCore/html/FileStreamProxy.cpp > +static void derefProxyOnContext(ScriptExecutionContext*, FileStreamProxy* proxy) > +{ > + proxy->deref(); Can we add an ASSERT before this line, like: ASSERT(proxy->hasOneRef()); This way, we can ensure that the caller, i.e., FileReader/FileWriter, has already released the reference and now it should only has ref count 1 now. > diff --git a/WebCore/html/FileStreamProxy.h b/WebCore/html/FileStreamProxy.h > +// A proxy module that calls corresponding FileStream methods on the file thread. Note: you must call stop() before destroying a FileStreamProxy instance, e.g. in the desctructor of an object that holds a reference to the proxy. Probably we could say the following for Note part: // ... Note: you must call stop() first and then release the reference to the FileStreamProxy instance. > + // Stop() stops any pending tasks, close the file and derefs itself; the caller should not recycle the instance after calling stop(). It is not needed to repeat stop() in the comment. Also we should use "closes" to be consistent with "stops". Probably it is better to say "destruct". // Stops the proxy and schedules it to be destructed. All the pending tasks will be aborted and the file stream will be closed. // Note: the caller should not recycle the instance after calling stop().

Created attachment 53020 [details] Patch

Thanks. (In reply to comment #17) > (From update of attachment 53009 [details]) > > +static void derefProxyOnContext(ScriptExecutionContext*, FileStreamProxy* proxy) > > +{ > > + proxy->deref(); > Can we add an ASSERT before this line, like: > ASSERT(proxy->hasOneRef()); > This way, we can ensure that the caller, i.e., FileReader/FileWriter, has > already > released the reference and now it should only has ref count 1 now. Asserting on ref count makes me a bit nervous, so I added another status flag to allow recycling even after stop() (just in case). > > diff --git a/WebCore/html/FileStreamProxy.h b/WebCore/html/FileStreamProxy.h > > +// A proxy module that calls corresponding FileStream methods on the file thread. Note: you must call stop() before destroying a FileStreamProxy instance, e.g. in the desctructor of an object that holds a reference to the proxy. > Probably we could say the following for Note part: > // ... Note: you must call stop() first and then release the reference to > the FileStreamProxy instance. Done. > > + // Stop() stops any pending tasks, close the file and derefs itself; the caller should not recycle the instance after calling stop(). > It is not needed to repeat stop() in the comment. Also we should use "closes" > to be consistent with "stops". > Probably it is better to say "destruct". > // Stops the proxy and schedules it to be destructed. All the pending > tasks will be aborted and the file stream will be closed. > // Note: the caller should not recycle the instance after calling stop(). Done. Also removed the notion about recycling as now it doesn't apply.

(In reply to comment #19) > Asserting on ref count makes me a bit nervous, so I added another status flag > to allow recycling even after stop() (just in case). > Is there any scenario we want to support reusing FileStreamProxy after calling stop()? If not at present, I'd rather keep the semantic strict because it is kind of confusing. Normally we assume calling something like start() or restart() to revive the instance. But with your new patch, calling any other methods of FileStreamProxy could trigger the reviving and this could make it easy to make the mistake.

Created attachment 53207 [details] Patch

(In reply to comment #20) > (In reply to comment #19) > > Asserting on ref count makes me a bit nervous, so I added another status flag > > to allow recycling even after stop() (just in case). > > Is there any scenario we want to support reusing FileStreamProxy after calling > stop()? If not at present, I'd rather keep the semantic strict because it is > kind of confusing. Normally we assume calling something like start() or > restart() to revive the instance. But with your new patch, calling any other > methods of FileStreamProxy could trigger the reviving and this could make it > easy to make the mistake. Ah ok FileReader has a reason to make it strict. (I changed like that since in FileWriter I didn't see a reason to force the caller to deref the proxy after abort().) Added the ASSERT as you suggested in your previous review. Also added a line to nullify the client in stop() to avoid calling back on the client after stop().

Created attachment 53208 [details] Rebased

Created attachment 53209 [details] Fixed the mess in diff

Comment on attachment 53209 [details] Fixed the mess in diff r=me Please fix several minor issues before landing. Thanks. > diff --git a/WebCore/html/FileStreamProxy.cpp b/WebCore/html/FileStreamProxy.cpp > +FileStreamProxy::FileStreamProxy(ScriptExecutionContext* context, FileStreamClient* client) > + : m_context(context) > + , m_client(client) > + , m_stream(FileStream::create(this)) > +{ > + // Holds an extra ref so that the instance will not get deleted while there can be any tasks on the file thread. > + ref(); Please add an empty line here for better readability. > +void FileStreamProxy::stop() > +{ > + // Clear the client so that we won't be calling callbacks on the client. > + m_client = 0; Please add an empty line here for better readability. > diff --git a/WebCore/html/FileStreamProxy.h b/WebCore/html/FileStreamProxy.h > + FileThread* fileThread(); Please add an empty line here. > + bool m_running; Please remove this leftover.

Committed r57749: <http://trac.webkit.org/changeset/57749>

Adam Barth had some comments on this design in bug 41547.

(In reply to comment #27) > Adam Barth had some comments on this design in bug 41547. Thanks, I'm going to open a new issue for that.