Fix various cases of incorrect cross-thread capture of non-thread-safe RefCounted
Created attachment 241243 [details] Patch
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.
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.
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.
Or better yet, introduce a URLCapture class.
I’ll add URLCapture.
Created attachment 241247 [details] Patch
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.
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.
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.
Committed r175792: <http://trac.webkit.org/changeset/175792>
(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.
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.