Bug 43985 – Change FileStream implementation to prepare for blob resource handling

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore JavaScript

[Jian Li]

Change FileStream implementation to prepare for blob resource handling.

[Jian Li]

Created attachment 64369 [details]
Proposed Patch

[David Levin]

Comment on attachment 64369 [details]
Proposed Patch

Just a few issues to address.


Not the focus of this patch but how does lifetime of FileStream work? I see tasks posted from FileStreamProxy to be handled by FileStream on a different thread in WebCore/html/FileStreamProxy.cpp

It looks like the lifetime of FileStream held by FileStreamProxy which is held by FileReader (which is basically held by js), so the lifetime seems to be managed on the ScriptExecutionContext thread (but the class is used on another thread).


> diff --git a/WebCore/ChangeLog b/WebCore/ChangeLog
> index 4f69af6..4d0ec7e 100644
> --- a/WebCore/ChangeLog
> +++ b/WebCore/ChangeLog
> @@ -1,3 +1,42 @@
> [phone]  Jian Li  <[e-mail]>
> +
> +        Reviewed by NOBODY (OOPS!).
> +
> +        Change FileStream implementation to prepare for blob resource handling.
> +        https://bugs.webkit.org/show_bug.cgi?id=43985
> +
> +        Enhance FileStream implementation to support both synchronous and
> +        asynchronous usages. This is needed in preparation to use FileStream
> +        in upcoming blob resource handling change. Update the existing FileReader
> +        code to adapt to this change. 
> +
> +        * html/FileReader.cpp:
> +        (WebCore::FileReader::didStart): 

Account for refactoring of the openForRead method, by now getting the size which the openForRead method used to do. ?


> +        (WebCore::FileReader::didGetSize):
> +        (WebCore::FileReader::didOpen):
> +        (WebCore::FileReader::didRead):
> +        * html/FileReader.h:
> +        * html/FileStream.cpp:

Allow m_client to be 0 when not on the main thread to enable ???

> +        (WebCore::FileStream::start):
> +        (WebCore::FileStream::stop):
> +        (WebCore::FileStream::getSize):
> +        (WebCore::FileStream::openForRead):
> +        (WebCore::FileStream::openForWrite):
> +        (WebCore::FileStream::close):
> +        (WebCore::FileStream::read):
> +        (WebCore::FileStream::write):
> +        (WebCore::FileStream::truncate):
> +        * html/FileStream.h:
> +        * html/FileStreamClient.h:
> +        (WebCore::FileStreamClient::didOpen):
Added to allow ???

> +        * html/FileStreamProxy.cpp:
Connect the new methods across threads.

> +        (WebCore::FileStreamProxy::getSize):
> +        (WebCore::FileStreamProxy::openForRead):
> +        (WebCore::notifyOpenOnContext):
> +        (WebCore::FileStreamProxy::didOpen):

> +        * html/FileStreamProxy.h:
Account for FileStreamClient changes and added getSize/openForRead methods.

> +        * html/FileThreadTask.h:

Added return value for to allow calling FileStream::openForRead.

Some per item comments would be useful in understanding the patch.....I'll be adding my own as I go through it.


> diff --git a/WebCore/html/FileReader.cpp b/WebCore/html/FileReader.cpp

> +    ASSERT(m_fileBlob->items().size() == 1 && m_fileBlob->items().at(0)->toFileBlobItem());
> +    const FileRangeBlobItem* fileRangeItem = m_fileBlob->items().at(0)->toFileRangeBlobItem();
> +    long long start = fileRangeItem ? fileRangeItem->start() : 0;
> +
> +    m_totalBytes = fileRangeItem ? fileRangeItem->size() : size;

When does fileRangeItem->size() != size ?




> diff --git a/WebCore/html/FileStream.cpp b/WebCore/html/FileStream.cpp

Why not have a AsyncFileStream that has a client and a FileStream that doesn't? It looks like AsyncFileStream could enforce that m_client is not 0 and just a a thin layer on top of FileStream.


> +long long FileStream::getSize(const String& path, double expectedModificationTime)
> +
> +    // Check the modificationt time for the possible file change.

typo: modificationt


> +ExceptionCode FileStream::openForWrite(const String&)
>  {
> -    ASSERT(!isMainThread());
> +    ASSERT(!m_client || !isMainThread());
>      // FIXME: to be implemented.
> +    return 0;

Seems like it should return a failure code.


> +int FileStream::write(Blob*, long long, int)
>  {
> +    ASSERT(!m_client || !isMainThread());
>      // FIXME: to be implemented.
> +    return 0;

Seems like it should return a failure code.

> +ExceptionCode FileStream::truncate(long long)
>  {
> +    ASSERT(!m_client || !isMainThread());
>      // FIXME: to be implemented.
> +    return 0;

Seems like it should return a failure code.



WebCore/html/FileStream.h:62
 +      // Gets the size of a file. Also validates if the file has been changed or not if the expected modification time is provided.
"if the expected modification time is not 0."

(It looks like it always must be provided since it is just a double being passed in.)

[Kinuko Yasuda]

(In reply to comment #2)
> Not the focus of this patch but how does lifetime of FileStream work? I see tasks posted from FileStreamProxy to be handled by FileStream on a different thread in WebCore/html/FileStreamProxy.cpp
> 
> It looks like the lifetime of FileStream held by FileStreamProxy which is held by FileReader (which is basically held by js), so the lifetime seems to be managed on the ScriptExecutionContext thread (but the class is used on another thread).

FileStream is a per-file-stream object and is supposed to be managed by the stream owner (e.g. FileReader).  In async context the stream owner owns the proxy's reference instead of the stream itself, and the proxy manages the lifetime of its peer stream object on FileThread.   There's some dirty code in FileStreamProxy that handles cases where the context is getting destroyed while there're queued/ongoing tasks on FileThread.  (I have a local patch that tries to clean up some of this but haven't uploaded it yet.)

FileThread is a per-ScriptExecutionContext thread (there's an objection that this might be excessive) and its lifetime is roughly bound to the ScriptExecutionContext.

> > diff --git a/WebCore/html/FileStream.cpp b/WebCore/html/FileStream.cpp
> 
> Why not have a AsyncFileStream that has a client and a FileStream that doesn't? It looks like AsyncFileStream could enforce that m_client is not 0 and just a a thin layer on top of FileStream

+1 having AsyncFileStream or some separate wrapper class for async cases.
Now that we both have sync and async cases separating classes would simplify several issues.

[David Levin]

(In reply to comment #3)
> (In reply to comment #2)
> > Not the focus of this patch but how does lifetime of FileStream work? I see tasks posted from FileStreamProxy to be handled by FileStream on a different thread in WebCore/html/FileStreamProxy.cpp

> FileStream is a per-file-stream object and is supposed to be managed by the stream owner (e.g. FileReader).  In async context the stream owner owns the proxy's reference instead of the stream itself, and the proxy manages the lifetime of its peer stream object on FileThread.   There's some dirty code in FileStreamProxy that handles cases where the context is getting destroyed while there're queued/ongoing tasks on FileThread. 

Ok, I see now. This is a pretty typical pattern.

[Jian Li]

Should we also rename FileStreamProxy to AsynFileStreamProxy?

[Kinuko Yasuda]

(In reply to comment #5)
> Should we also rename FileStreamProxy to AsynFileStreamProxy?

Hmm, I have no strong opinion but sounds like it makes more sense.

[Jian Li]

Created attachment 64538 [details]
Proposed Patch

All fixed.

As discussed, I removed FileStreamClient callbacks from FileStream and make all methods synchronously only. Also I moved all the asynchronous and callback logics in FileStreamProxy.

[WebKit Review Bot]

Attachment 64538 [details] did not pass style-queue:

Failed to run "['WebKitTools/Scripts/check-webkit-style']" exit_code: 1
WebCore/html/FileStream.cpp:129:  An else if statement should be written as an if statement when the prior "if" concludes with a return, break, continue or goto statement.  [readability/control_flow] [4]
Total errors found: 1 in 9 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Jian Li]

Created attachment 64540 [details]
Proposed Patch

Fixed style error.

[Kinuko Yasuda]

(In reply to comment #7)
> Created an attachment (id=64538) [details]
> Proposed Patch
> 
> All fixed.
> 
> As discussed, I removed FileStreamClient callbacks from FileStream and make all methods synchronously only. Also I moved all the asynchronous and callback logics in FileStreamProxy.

This change looks good to me.

Don't we also want to change Method definitions in WebCore/html/FileThreadTask.h back to return void?
I would wonder how we use the return value of a task body.

[David Levin]

Comment on attachment 64540 [details]
Proposed Patch

Just a few nits.

> diff --git a/WebCore/html/FileStream.h b/WebCore/html/FileStream.h
> +    int write(Blob* blob, long long position, int length);

param name |blob| not needed.


> diff --git a/WebCore/html/FileStreamProxy.h b/WebCore/html/FileStreamProxy.h
> +    void writeOnFileThread(Blob* blob, long long position, int length);

param name |blob| not needed.

> diff --git a/WebCore/html/FileThreadTask.h b/WebCore/html/FileThreadTask.h
> +    typedef R (T::*Method)(MP1, MP2, MP3);

I agree with Kinuko. (I understand you are doing this for consistency with the other methods in this file but I wonder if all the methods should just handle void returns. I'm fine with this change or changing all of them to be void.)

[Jian Li]

Committed as http://trac.webkit.org/changeset/65523.

All problems are fixed at landing.