Bug 138539 – Fix various cases of incorrect cross-thread capture of non-thread-safe RefCounted

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

[Darin Adler]

Fix various cases of incorrect cross-thread capture of non-thread-safe RefCounted

[Darin Adler]

Created attachment 241243 [details]
Patch

[Darin Adler]

This covers all the places I could find trying to use isolatedCopy to capture something and pass it to a new thread, but dealing with the last deref on the main thread incorrectly.

[WebKit Commit Bot]

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


ERROR: Source/WebCore/Modules/websockets/WorkerThreadableWebSocketChannel.cpp:332:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
Total errors found: 1 in 10 files


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

[Alexey Proskuryakov]

Comment on attachment 241243 [details]
Patch

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

> Source/WebCore/Modules/websockets/WorkerThreadableWebSocketChannel.cpp:421
> +    StringCapture capturedURL(url.string());

This idiom makes URL forget that it was invalid - we get a string, and then push it back into a URL without checks.

I think that we need to explicitly check for url being valid here and everywhere else.

[Alexey Proskuryakov]

Or better yet, introduce a URLCapture class.

[Darin Adler]

I’ll add URLCapture.

[Darin Adler]

Created attachment 241247 [details]
Patch

[WebKit Commit Bot]

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


ERROR: Source/WebCore/Modules/websockets/WorkerThreadableWebSocketChannel.cpp:332:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
Total errors found: 1 in 13 files


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

[Alexey Proskuryakov]

Comment on attachment 241247 [details]
Patch

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

Very nice!

> Source/WebCore/platform/URL.h:269
> +    URL releaseURL();

This function ended up unused, would you still like to keep it?

> Source/WebCore/platform/URL.h:272
> +    void operator=(const URLCapture&) = delete;

It is unusual to have a copy constructor, but delete operator=. I'm not sure if that's legitimate.

[Darin Adler]

Comment on attachment 241247 [details]
Patch

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

>> Source/WebCore/platform/URL.h:269
>> +    URL releaseURL();
> 
> This function ended up unused, would you still like to keep it?

I simply copied StringCapture. I think the two should stay in sync, but I am not sure if either needs this. I think we should have either url() or releaseURL(), but I don’t think we need both.

>> Source/WebCore/platform/URL.h:272
>> +    void operator=(const URLCapture&) = delete;
> 
> It is unusual to have a copy constructor, but delete operator=. I'm not sure if that's legitimate.

I simply copied StringCapture. I think the two should stay in sync.

I don’t feel strongly about this design issue; it’s a little arbitrary to not have operator=, but seems OK to me. Note that the only reason we have a copy constructor in this class is because that’s what’s used to capture. If we could make it legal only for capture and make any other copying illegal, I think we would do that.

[Darin Adler]

Committed r175792: <http://trac.webkit.org/changeset/175792>

[Antti Koivisto]

(In reply to comment #10)
> I don’t feel strongly about this design issue; it’s a little arbitrary to
> not have operator=, but seems OK to me. Note that the only reason we have a
> copy constructor in this class is because that’s what’s used to capture. If
> we could make it legal only for capture and make any other copying illegal,
> I think we would do that.

Right, I tried to make it harder to use this class for anything else beyond capturing.

[Alexey Proskuryakov]

I was thinking about the rule of three <http://en.wikipedia.org/wiki/Rule_of_three_(C%2B%2B_programming)>.

I certainly don't feel strongly about this at all. If there is anything to change here, it's probably to delete operator=(URLCapture&&) as well.