Bug 138282 – Replace FileThread class with a single function

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore Misc.

[Darin Adler]

Replace FileStream class with a single function

[Darin Adler]

Created attachment 240796 [details]
Patch

[WebKit Commit Bot]

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


ERROR: Source/WebCore/fileapi/AsyncFileStream.cpp:143:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/WebCore/fileapi/AsyncFileStream.cpp:143:  Extra space before [  [whitespace/braces] [5]
ERROR: Source/WebCore/fileapi/AsyncFileStream.cpp:154:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/WebCore/fileapi/AsyncFileStream.cpp:154:  Extra space before [  [whitespace/braces] [5]
ERROR: Source/WebCore/fileapi/AsyncFileStream.cpp:165:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/WebCore/fileapi/AsyncFileStream.cpp:165:  Extra space before [  [whitespace/braces] [5]
ERROR: Source/WebCore/fileapi/AsyncFileStream.cpp:185:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/WebCore/fileapi/AsyncFileStream.cpp:185:  Extra space before [  [whitespace/braces] [5]
ERROR: Source/WebCore/fileapi/AsyncFileStream.cpp:196:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/WebCore/fileapi/AsyncFileStream.cpp:196:  Extra space before [  [whitespace/braces] [5]
ERROR: Source/WebCore/fileapi/AsyncFileStream.cpp:206:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/WebCore/fileapi/AsyncFileStream.cpp:206:  Extra space before [  [whitespace/braces] [5]
Total errors found: 12 in 14 files


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

[Antti Koivisto]

Comment on attachment 240796 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=240796&action=review

> Source/WebCore/fileapi/AsyncFileStream.cpp:141
> +    perform([pathCopy, expectedModificationTime] (FileStream& stream) {

Anders told me that the coding style for lambdas is to not  have space between ](.

[Alexey Proskuryakov]

Comment on attachment 240796 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=240796&action=review

Very nice! I have a lot of comments, but I think that all of them can be addressed without another review round.

This patch breaks Windows build, EWS is red for a reason this time.

> Source/WTF/ChangeLog:9
> +        * wtf/MessageQueue.h: Made queue work on any type and not require wrapping
> +        everything in a unique_ptr.

I'm not sure if I fully understand the consequences of this change, but it seems like it makes the class easier to misuse. E.g.

messageQueue.append(myString.isolatedCopy());

Doesn't this result in a race when a stack instance of the isolated copy is destroyed on sender thread? Unless I'm missing something, this is an undesirable change.

> Source/WTF/wtf/MessageQueue.h:136
> +    inline auto MessageQueue<DataType>::waitForMessageFilteredWithTimeout(MessageQueueWaitResult& result, Predicate&& predicate, double absoluteTime) -> DataType

There is a "return nullptr" below in this function, so it will fail to compile with any random DataType.

> Source/WTF/wtf/Threading.cpp:105
> +// Safari uses these old versions of createThread() and waitForThreadCompletion() functions
> +// directly and we need to keep the old ABI compatibility until everyone has a newer version
> +// of Safari to work with.

Is there a Radar that tracks getting Safari off these functions? It's unclear why we expect a newer Safari version to not need them, or why these old functions are bad.

> Source/WebCore/fileapi/AsyncFileStream.cpp:52
> +    static ThreadIdentifier fileThread = createThread("WebCore: File", [] {

It's strange to have an unused static variable. Can we use std::call_once instead?

> Source/WebCore/fileapi/AsyncFileStream.cpp:55
> +            queue.get().waitForMessage()();

waitForMessage() can return nullptr. We should probably check for that, if only for static analyzer happiness. I'm not sure what happens when a nullptr converted to a std::function<void()> gets called, but I assume that it's nothing good.

> Source/WebCore/fileapi/AsyncFileStream.cpp:91
> +    PassRef<AsyncFileStream> result = adoptRef(*new AsyncFileStream(client));

It's strange to see a "Pass" class when not passing an argument. In the world of C++11, what is the purpose of PassRef, can we just return a Ref<AsyncFileStream>?

> Source/WebCore/fileapi/AsyncFileStream.cpp:98
> +    ASSERT(isMainThread());

Thank you for adding these asserts!

> Source/WebCore/fileapi/AsyncFileStream.cpp:127
> +        if (m_stopped)

This check is racy, are we OK with that? Seems like it is just an optimization.

> Source/WebCore/fileapi/AsyncFileStream.cpp:140
>      String pathCopy = path.isolatedCopy();

It is a pre-existing issue, but this code is racy. It's not possible to use a String like this, we have to use StringImpl* and manual memory management.

> Source/WebCore/fileapi/AsyncFileStream.cpp:151
>      String pathCopy = path.isolatedCopy();

Ditto.

> Source/WebCore/fileapi/AsyncFileStream.cpp:162
>      String pathCopy = path.isolatedCopy();

Ditto.

> Source/WebCore/fileapi/AsyncFileStream.cpp:178
> +    if (m_stopped)
> +        return;
> +
> +    callOnFileThread([this] {
>          m_stream->close();
> -    } });
> +    });

This looks somewhat confusing. This function checks m_stopped, but it is not the function that is responsible for setting it. What guarantees that m_stopped doesn't become true before the file thread executes close()? And if nothing guarantees that, what does the check on main thread achieve, is it also a racy performance optimization?

> Source/WebCore/fileapi/AsyncFileStream.cpp:193
>      URL blobURLCopy = blobURL.copy();

Same concern about racing blobURLCopy destructors on main and file threads.

[Darin Adler]

Comment on attachment 240796 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=240796&action=review

>> Source/WTF/ChangeLog:9
>> +        everything in a unique_ptr.
> 
> I'm not sure if I fully understand the consequences of this change, but it seems like it makes the class easier to misuse. E.g.
> 
> messageQueue.append(myString.isolatedCopy());
> 
> Doesn't this result in a race when a stack instance of the isolated copy is destroyed on sender thread? Unless I'm missing something, this is an undesirable change.

I think the right thing to do is to change the class template to use r-value references and always move rather than copy. You make an excellent point that we don’t want this to be easy to misuse. But I also don’t want it to always require a std::unique_ptr, since there are other classes that can be moved, such as std::function. Does that sound practical to you. Alexey?

>> Source/WTF/wtf/MessageQueue.h:136
>> +    inline auto MessageQueue<DataType>::waitForMessageFilteredWithTimeout(MessageQueueWaitResult& result, Predicate&& predicate, double absoluteTime) -> DataType
> 
> There is a "return nullptr" below in this function, so it will fail to compile with any random DataType.

This function is only used in one place, and I think it’s critical to have it work with non-pointer types.

I could also change those to "return DataType()", which will work with more types, but still isn’t universal.

>> Source/WTF/wtf/Threading.cpp:105
>> +// of Safari to work with.
> 
> Is there a Radar that tracks getting Safari off these functions? It's unclear why we expect a newer Safari version to not need them, or why these old functions are bad.

I have no idea if there is a Radar. I could research the old check-in that made this change. I suspect that this requirement is already gone from any version of Safari people are currently actively using with WebKit.

To me it’s entirely clear why we expect a newer Safari version to not need these. Any version of Safari compiled against the current Threading.h header would not use any of these! It’s just for binary compatibility with older versions compiled against an older version of the header.

And these old functions are not “bad”, they are simply obsolete. We should remove them because the newer functions suffice.

>> Source/WebCore/fileapi/AsyncFileStream.cpp:52
>> +    static ThreadIdentifier fileThread = createThread("WebCore: File", [] {
> 
> It's strange to have an unused static variable. Can we use std::call_once instead?

Sounds fine.

>> Source/WebCore/fileapi/AsyncFileStream.cpp:55
>> +            queue.get().waitForMessage()();
> 
> waitForMessage() can return nullptr. We should probably check for that, if only for static analyzer happiness. I'm not sure what happens when a nullptr converted to a std::function<void()> gets called, but I assume that it's nothing good.

I’m not sure what this really means in practice. Since there is no such thing as a null std::function, I wonder if there is any way to change the underlying class to avoid adding a check for something that can never happen? I don’t see how it could return nullptr, given that nullptr can’t be converted to std::function<void()>.

Maybe you can help me figure this out.

>> Source/WebCore/fileapi/AsyncFileStream.cpp:91
>> +    PassRef<AsyncFileStream> result = adoptRef(*new AsyncFileStream(client));
> 
> It's strange to see a "Pass" class when not passing an argument. In the world of C++11, what is the purpose of PassRef, can we just return a Ref<AsyncFileStream>?

Good question. I don’t know the answer. I could just have this return a PassRefPtr in the mean time, but I hate to imply that there could be a null value here. Maybe someone can help me figure this out.

>> Source/WebCore/fileapi/AsyncFileStream.cpp:127
>> +        if (m_stopped)
> 
> This check is racy, are we OK with that? Seems like it is just an optimization.

That’s right. It’s racy, but it’s an optimization to do fewer file system operations if we can. My initial code omitted this but it seemed unnecessary to run all the queued operations once we know the queue is stopped.

>> Source/WebCore/fileapi/AsyncFileStream.cpp:140
>>      String pathCopy = path.isolatedCopy();
> 
> It is a pre-existing issue, but this code is racy. It's not possible to use a String like this, we have to use StringImpl* and manual memory management.

That seems critical to fix. I (or someone else) should deal with that in a separate patch before making this change.

I’m worried that the patches I have encouraged Zan to write using lambdas are introducing many cases of this race. We need to come up with an idiom that works, and fix this immediately. Maybe we should change the return type of String::isolatedCopy and we should also write/rename URL::isolatedCopy with a similar goal in mind.

>> Source/WebCore/fileapi/AsyncFileStream.cpp:141
>> +    perform([pathCopy, expectedModificationTime] (FileStream& stream) {
> 
> Anders told me that the coding style for lambdas is to not  have space between ](.

It’s the damn style checker that convinced me to change this. Maybe someone can fix that style checker script so I can change this back without having a complaint on every line of code that does this.

>> Source/WebCore/fileapi/AsyncFileStream.cpp:178
>> +    });
> 
> This looks somewhat confusing. This function checks m_stopped, but it is not the function that is responsible for setting it. What guarantees that m_stopped doesn't become true before the file thread executes close()? And if nothing guarantees that, what does the check on main thread achieve, is it also a racy performance optimization?

It’s safe to make any number of additional close calls on an already closed FileStream.

And yes, the check on the main thread is a performance optimization that could be omitted.

[Alexey Proskuryakov]

> Does that sound practical to you. Alexey?

Yes, I think so.

I later started to wonder if passing std::functions across threads was safe enough. We don't know what variables they capture - capture any WTF::String, and it seems nearly impossible to catch the mistake. But this patch makes the code so much cleaner in other ways. I'm torn about this.

> Any version of Safari compiled against the current Threading.h header would not use any of these!

There are two sets of deprecated functionality here. One is the old two-argument variant that only exists in .cpp file, createThread(ThreadFunctionWithReturnValue entryPoint, void* data). I'm also pretty sure that it can be simply removed now, no need to keep it.

But your patch also marks createThread(ThreadFunction, void*, const char* threadName) as deprecated, and that function is still used by Safari. What I question is that while we have a comment saying "until everyone has a newer version of Safari to work with", there is no recorded plan to create such a version. As this one is newly deprecated, a Radar for Safari needs to be filed.

> given that nullptr can’t be converted to std::function<void()>

Turns out that "std::function<void()> f = nullptr" compiles perfectly fine. Trying to call this function results in a C++ exception being raised.

I believe that all the usual ways of checking a pointer for being null work with std::function too.

> I’m worried that the patches I have encouraged Zan to write using lambdas are introducing many cases of this race. We need to come up with an idiom that works, and fix this immediately. Maybe we should change the return type of String::isolatedCopy and we should also write/rename URL::isolatedCopy with a similar goal in mind.

Sounds like a good idea to me! We used to have such things automatic in CrossThreadCopier, but that wasn't useful with modern C++, and got deleted. Making isolatedCopy() functions hard to misuse is definitely worth trying.

> And yes, the check on the main thread is a performance optimization that could be omitted.

It seems worth having a comment about, so that the racy nature of these checks doesn't confuse anyone in the future.

[Zan Dobersek]

(In reply to comment #6)
> I believe that all the usual ways of checking a pointer for being null work
> with std::function too.
> 
> > I’m worried that the patches I have encouraged Zan to write using lambdas are introducing many cases of this race. We need to come up with an idiom that works, and fix this immediately. Maybe we should change the return type of String::isolatedCopy and we should also write/rename URL::isolatedCopy with a similar goal in mind.
> 

There's already the StringCapture class that is meant to be used for thread-safe lambda capturing. It's not yet adopted everywhere it should be, obviously.
https://trac.webkit.org/browser/trunk/Source/WTF/wtf/text/WTFString.h#L637

It should be possible to build on that and create something more generic, like a class template that only works for classes that support isolated copying.

[Darin Adler]

(In reply to comment #6)
> > Any version of Safari compiled against the current Threading.h header would not use any of these!
> 
> There are two sets of deprecated functionality here. One is the old
> two-argument variant that only exists in .cpp file,
> createThread(ThreadFunctionWithReturnValue entryPoint, void* data). I'm also
> pretty sure that it can be simply removed now, no need to keep it.

Yes, the comment is only about this function and the two others that are declared only in the .cpp file and not mentioned at all in the header, and also only compiled on PLATFORM(MAC) and only kept around for the benefit of old versions of Mac Safari. It’s not intended to comment on any of the functions mentioned in the header.

> But your patch also marks createThread(ThreadFunction, void*, const char*
> threadName) as deprecated, and that function is still used by Safari. What I
> question is that while we have a comment saying "until everyone has a newer
> version of Safari to work with", there is no recorded plan to create such a
> version. As this one is newly deprecated, a Radar for Safari needs to be
> filed.

The comment is inside the #if. The other deprecated function does not have a comment like this, is declared in the header files, and is still used in WebKit source code. Is there some way I can make it clearer that this comment has nothing to do with the that other deprecated function? Or we could deleted this PLATFORM(MAC) code and the comment along with it. Or we could just remove the term "deprecated" from the header and come back to deprecating it later.

> Turns out that "std::function<void()> f = nullptr" compiles perfectly fine.

I didn’t know a std::function could be null.

> Sounds like a good idea to me! We used to have such things automatic in
> CrossThreadCopier, but that wasn't useful with modern C++, and got deleted.

CrossThreadCopier was deleted because I didn’t think it did anything helpful. But you are making a case now that it was doing something helpful. I’m sorry we didn’t notice that before phasing it out.

The real fix here is probably to finish Anders’ experiment to make the String reference count thread-safe.

> > And yes, the check on the main thread is a performance optimization that could be omitted.
> 
> It seems worth having a comment about, so that the racy nature of these
> checks doesn't confuse anyone in the future.

I’d rather take it out than add a comment about it.

[Darin Adler]

(In reply to comment #7)
> (In reply to comment #6)
> > I believe that all the usual ways of checking a pointer for being null work
> > with std::function too.
> > 
> > > I’m worried that the patches I have encouraged Zan to write using lambdas are introducing many cases of this race. We need to come up with an idiom that works, and fix this immediately. Maybe we should change the return type of String::isolatedCopy and we should also write/rename URL::isolatedCopy with a similar goal in mind.
> 
> There's already the StringCapture class that is meant to be used for
> thread-safe lambda capturing. It's not yet adopted everywhere it should be,
> obviously.
> https://trac.webkit.org/browser/trunk/Source/WTF/wtf/text/WTFString.h#L637
> 
> It should be possible to build on that and create something more generic,
> like a class template that only works for classes that support isolated
> copying.

Lets find a good idiom for this ASAP! We want to make it easy to make the isolated copy and capture it for a function to be used on another thread, without a chance of leaving a RefPtr temporary or anything else that will call deref() later on the main thread.

I’d love this to be the *only* way to use isolatedCopy and replace both copy and isolatedCopy for both classes; I think it's the only reason we ever want to copy a String or URL since they are immutable.

[Darin Adler]

Comment on attachment 240796 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=240796&action=review

>>> Source/WebCore/fileapi/AsyncFileStream.cpp:91
>>> +    PassRef<AsyncFileStream> result = adoptRef(*new AsyncFileStream(client));
>> 
>> It's strange to see a "Pass" class when not passing an argument. In the world of C++11, what is the purpose of PassRef, can we just return a Ref<AsyncFileStream>?
> 
> Good question. I don’t know the answer. I could just have this return a PassRefPtr in the mean time, but I hate to imply that there could be a null value here. Maybe someone can help me figure this out.

I don’t think we can return Ref<> since Ref is neither copyable nor movable.

[Darin Adler]

(In reply to comment #6)
> I later started to wonder if passing std::functions across threads was safe
> enough. We don't know what variables they capture - capture any WTF::String,
> and it seems nearly impossible to catch the mistake. But this patch makes
> the code so much cleaner in other ways. I'm torn about this.

It’s true, we need to be careful not to capture RefCounted objects such as WTF::String and WebCore::URL. Instead we should capture WTF::StringCapture. For the moment, it’s not too hard to do that. It would be good to find even more ways to prevent misuse.

However, it’s no better to move a std::unique_ptr<std::function> into and out of the MessageQueue than it would be to move the std::function itself, so I still think it would be worth changing the MessageQueue class long term.

However, I rolled that out of the patch.

[Darin Adler]

Created attachment 241233 [details]
Patch

[Darin Adler]

Created attachment 241234 [details]
Patch

[Darin Adler]

OK, now I have uploaded a new patch that should address everything that was raised during the review, except perhaps whatever was preventing this from compiling on Windows.

[WebKit Commit Bot]

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


ERROR: Source/WebCore/fileapi/AsyncFileStream.cpp:136:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/WebCore/fileapi/AsyncFileStream.cpp:136:  Extra space before [  [whitespace/braces] [5]
ERROR: Source/WebCore/fileapi/AsyncFileStream.cpp:147:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/WebCore/fileapi/AsyncFileStream.cpp:147:  Extra space before [  [whitespace/braces] [5]
ERROR: Source/WebCore/fileapi/AsyncFileStream.cpp:158:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/WebCore/fileapi/AsyncFileStream.cpp:158:  Extra space before [  [whitespace/braces] [5]
ERROR: Source/WebCore/fileapi/AsyncFileStream.cpp:176:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/WebCore/fileapi/AsyncFileStream.cpp:176:  Extra space before [  [whitespace/braces] [5]
ERROR: Source/WebCore/fileapi/AsyncFileStream.cpp:188:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/WebCore/fileapi/AsyncFileStream.cpp:188:  Extra space before [  [whitespace/braces] [5]
ERROR: Source/WebCore/fileapi/AsyncFileStream.cpp:198:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/WebCore/fileapi/AsyncFileStream.cpp:198:  Extra space before [  [whitespace/braces] [5]
Total errors found: 12 in 11 files


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

[Darin Adler]

Created attachment 241235 [details]
Patch

[Darin Adler]

New version of the patch tries to fix compilation on Windows, too. Working around bugs in the Windows compiler, I think.

[WebKit Commit Bot]

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


ERROR: Source/WebCore/fileapi/AsyncFileStream.cpp:139:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/WebCore/fileapi/AsyncFileStream.cpp:139:  Extra space before [  [whitespace/braces] [5]
ERROR: Source/WebCore/fileapi/AsyncFileStream.cpp:151:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/WebCore/fileapi/AsyncFileStream.cpp:151:  Extra space before [  [whitespace/braces] [5]
ERROR: Source/WebCore/fileapi/AsyncFileStream.cpp:162:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/WebCore/fileapi/AsyncFileStream.cpp:162:  Extra space before [  [whitespace/braces] [5]
ERROR: Source/WebCore/fileapi/AsyncFileStream.cpp:180:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/WebCore/fileapi/AsyncFileStream.cpp:180:  Extra space before [  [whitespace/braces] [5]
ERROR: Source/WebCore/fileapi/AsyncFileStream.cpp:192:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/WebCore/fileapi/AsyncFileStream.cpp:192:  Extra space before [  [whitespace/braces] [5]
ERROR: Source/WebCore/fileapi/AsyncFileStream.cpp:202:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/WebCore/fileapi/AsyncFileStream.cpp:202:  Extra space before [  [whitespace/braces] [5]
Total errors found: 12 in 11 files


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

[Darin Adler]

Created attachment 241236 [details]
Patch

[Darin Adler]

One more try at fixing the Windows build.

[WebKit Commit Bot]

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


ERROR: Source/WebCore/fileapi/AsyncFileStream.cpp:139:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/WebCore/fileapi/AsyncFileStream.cpp:139:  Extra space before [  [whitespace/braces] [5]
ERROR: Source/WebCore/fileapi/AsyncFileStream.cpp:151:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/WebCore/fileapi/AsyncFileStream.cpp:151:  Extra space before [  [whitespace/braces] [5]
ERROR: Source/WebCore/fileapi/AsyncFileStream.cpp:162:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/WebCore/fileapi/AsyncFileStream.cpp:162:  Extra space before [  [whitespace/braces] [5]
ERROR: Source/WebCore/fileapi/AsyncFileStream.cpp:180:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/WebCore/fileapi/AsyncFileStream.cpp:180:  Extra space before [  [whitespace/braces] [5]
ERROR: Source/WebCore/fileapi/AsyncFileStream.cpp:192:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/WebCore/fileapi/AsyncFileStream.cpp:192:  Extra space before [  [whitespace/braces] [5]
ERROR: Source/WebCore/fileapi/AsyncFileStream.cpp:202:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/WebCore/fileapi/AsyncFileStream.cpp:202:  Extra space before [  [whitespace/braces] [5]
Total errors found: 12 in 11 files


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

[Darin Adler]

Created attachment 241240 [details]
Patch

[WebKit Commit Bot]

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


ERROR: Source/WebCore/fileapi/AsyncFileStream.cpp:146:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/WebCore/fileapi/AsyncFileStream.cpp:146:  Extra space before [  [whitespace/braces] [5]
ERROR: Source/WebCore/fileapi/AsyncFileStream.cpp:158:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/WebCore/fileapi/AsyncFileStream.cpp:158:  Extra space before [  [whitespace/braces] [5]
ERROR: Source/WebCore/fileapi/AsyncFileStream.cpp:169:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/WebCore/fileapi/AsyncFileStream.cpp:169:  Extra space before [  [whitespace/braces] [5]
ERROR: Source/WebCore/fileapi/AsyncFileStream.cpp:187:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/WebCore/fileapi/AsyncFileStream.cpp:187:  Extra space before [  [whitespace/braces] [5]
ERROR: Source/WebCore/fileapi/AsyncFileStream.cpp:199:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/WebCore/fileapi/AsyncFileStream.cpp:199:  Extra space before [  [whitespace/braces] [5]
ERROR: Source/WebCore/fileapi/AsyncFileStream.cpp:209:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/WebCore/fileapi/AsyncFileStream.cpp:209:  Extra space before [  [whitespace/braces] [5]
Total errors found: 12 in 12 files


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

[Darin Adler]

Comment on attachment 241240 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=241240&action=review

> Source/WebCore/fileapi/AsyncFileStream.cpp:61
> +    // Work around a that prevents the default value above from compiling.

Locally fixed this comment to say "a bug that" instead of "a that".

[Darin Adler]

Committed r175782: <http://trac.webkit.org/changeset/175782>

[Alexey Proskuryakov]

Comment on attachment 241240 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=241240&action=review

> Source/WebCore/fileapi/AsyncFileStream.cpp:106
> +    // Release so that we can control the timing of deletion below.
> +    auto& internals = *m_internals.release();

I spent a while looking at this, and couldn't tell for sure if this is correct or not. It seems incorrect to me.

For this code to work at all, the reference has to extend the lifetime of the object it references. The destructor will be called when the reference goes out of scope. And it will also be called manually on another thread, so I expect this to always cause double deletion. What am I missing?

[Darin Adler]

Comment on attachment 241240 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=241240&action=review

>> Source/WebCore/fileapi/AsyncFileStream.cpp:61
>> +    // Work around a that prevents the default value above from compiling.
> 
> Locally fixed this comment to say "a bug that" instead of "a that".

Locally fixed this comment to say "a bug that" instead of "a that".

>> Source/WebCore/fileapi/AsyncFileStream.cpp:106
>> +    auto& internals = *m_internals.release();
> 
> I spent a while looking at this, and couldn't tell for sure if this is correct or not. It seems incorrect to me.
> 
> For this code to work at all, the reference has to extend the lifetime of the object it references. The destructor will be called when the reference goes out of scope. And it will also be called manually on another thread, so I expect this to always cause double deletion. What am I missing?

You are missing the fact that unique_ptr.release() relinquishes ownership of the object pointed to and returns a raw pointer to that object. Maybe you would find it more readable if I wrote:

    Internals& internals = *m_internals.release();

Or if I wrote:

    Internals* internals = m_internals.release();

And then rewrote the code below to use a raw pointer.

[Alexey Proskuryakov]

I see, indeed. It's not a temporary, so there is nothing to extend the lifetime of.

To me, it looks most obvious with pointers.