Bug 133079 – Use C++11 lambdas to construct FileThread::Task objects

• Component: New Bugs
• Status: RESOLVED FIXED

[Zan Dobersek]

Use C++11 lambdas to construct FileThread::Task objects

[Zan Dobersek]

Created attachment 231708 [details]
Patch

[WebKit Commit Bot]

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


ERROR: Source/WebCore/fileapi/FileThread.h:60:  Extra space before ( in function call  [whitespace/parens] [4]
Total errors found: 1 in 4 files


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

[Alexey Proskuryakov]

Comment on attachment 231708 [details]
Patch

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

> Source/WebCore/fileapi/AsyncFileStream.cpp:81
> -    fileThread()->postTask(std::make_unique<FileThread::Task>(proxy.get(), &AsyncFileStream::startOnFileThread));
> +    AsyncFileStream* proxyPtr = proxy.get();

This code looks very suspicious. Both main thread and file thread have pointers to AsyncFileStream, potentially referencing and dereferencing it concurrently. But it's not even ThreadSafeRefCounted.

[Darin Adler]

Comment on attachment 231708 [details]
Patch

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

>> Source/WebCore/fileapi/AsyncFileStream.cpp:81
>> +    AsyncFileStream* proxyPtr = proxy.get();
> 
> This code looks very suspicious. Both main thread and file thread have pointers to AsyncFileStream, potentially referencing and dereferencing it concurrently. But it's not even ThreadSafeRefCounted.

Alexey, is that a comment on some new aspect of the code or just on the code as it already was?

> Source/WebCore/fileapi/AsyncFileStream.cpp:84
> +        if (!proxyPtr->client())
> +            return;

Why did you delete the comment about a race condition that was in the startOnFileThread function?

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

I wonder why we need isolatedCopy for String, but not for URL. Maybe URL needs an isolatedCopy function too. That’s not something that’s changing in this patch, but might be worth thinking through.

> Source/WebCore/fileapi/FileThread.h:71
> +        Task(Task&& other)
> +            : m_task(std::move(other.m_task))
> +            , m_instance(other.m_instance)
> +        {
> +        }

Do we need to write this explicitly? Won’t the compiler generate this if we don’t write it?

[Alexey Proskuryakov]

> Alexey, is that a comment on some new aspect of the code or just on the code as it already was?

I think that it was already unsafe, yes.

[Zan Dobersek]

Comment on attachment 231708 [details]
Patch

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

>>> Source/WebCore/fileapi/AsyncFileStream.cpp:81
>>> +    AsyncFileStream* proxyPtr = proxy.get();
>> 
>> This code looks very suspicious. Both main thread and file thread have pointers to AsyncFileStream, potentially referencing and dereferencing it concurrently. But it's not even ThreadSafeRefCounted.
> 
> Alexey, is that a comment on some new aspect of the code or just on the code as it already was?

I believe this patch neither fixes this issue nor does it make it worse. I'd prefer to spin this off into a separate bug, if that's fine by both of you.

>> Source/WebCore/fileapi/AsyncFileStream.cpp:84
>> +            return;
> 
> Why did you delete the comment about a race condition that was in the startOnFileThread function?

Unintentional -- I'll re-add it.

>> Source/WebCore/fileapi/AsyncFileStream.cpp:171
>> +    URL blobURLCopy = blobURL.copy();
> 
> I wonder why we need isolatedCopy for String, but not for URL. Maybe URL needs an isolatedCopy function too. That’s not something that’s changing in this patch, but might be worth thinking through.

URL::copy() does an isolated copy of the internal String object, and should perhaps have the same name. copy() makes it sound like it returns a general copy, not a cross-thread one.

>> Source/WebCore/fileapi/FileThread.h:71
>> +        }
> 
> Do we need to write this explicitly? Won’t the compiler generate this if we don’t write it?

Yes, it's required because the implicit move constructor doesn't get generated due to the copy constructor being declared deleted through WTF_MAKE_NONCOPYABLE.

[Alexey Proskuryakov]

> I'd prefer to spin this off into a separate bug, if that's fine by both of you.

OK.

[Zan Dobersek]

(In reply to comment #7)
> > I'd prefer to spin this off into a separate bug, if that's fine by both of you.
> 
> OK.

Bug #133540.

[Zan Dobersek]

Created attachment 232532 [details]
Patch for landing

[WebKit Commit Bot]

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


ERROR: Source/WebCore/fileapi/FileThread.h:58:  Extra space before ( in function call  [whitespace/parens] [4]
Total errors found: 1 in 4 files


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

[Build Bot]

Comment on attachment 232532 [details]
Patch for landing

Attachment 232532 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/5911483581988864

New failing tests:
media/W3C/video/networkState/networkState_during_loadstart.html

[Build Bot]

Created attachment 232565 [details]
Archive of layout-test-results from webkit-ews-15 for mac-mountainlion-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: webkit-ews-15  Port: mac-mountainlion-wk2  Platform: Mac OS X 10.8.5

[Alexey Proskuryakov]

This is not a real failure (and not even a crash). I wish I knew what was going on.

[Zan Dobersek]

Comment on attachment 232532 [details]
Patch for landing

Clearing flags on attachment: 232532

Committed r169675: <http://trac.webkit.org/changeset/169675>

[Zan Dobersek]

All reviewed patches have been landed.  Closing bug.