Add isolatedCopy() to ResourceRequest, ResourceResponse, and ResourceError
Note ResourceError.copy() is already an isolatedCopy, but you wouldn't know that by name.
Note: The network/cf Request/Response/Errors already have isolatedCopy(). But the other platforms do not. isolatedCopy() should really be a function of RequestBase/ResponseBase/ErrorBase.
The entire notion of "CrossThreadResourceRequestData" (and equivalent for Response/Error) is silly and should probably be abolished. Fortunately, abolishing it will be easy - It's not used in too many places.
Two steps here: 1 - Make the crossThread*Data objects private, expose a public isolatedCopy that uses crossThread*Data internally, and update all uses in WebCore. 2 - Get rid of crossThread*Data internally. I'll file another bug for #2 later.
Created attachment 280337 [details] Patch
Attachment 280337 [details] did not pass style-queue: ERROR: Source/WebCore/platform/network/ResourceRequestBase.h:42: One space before end of line comments [whitespace/comments] [5] ERROR: Source/WebCore/platform/network/ResourceRequestBase.h:53: Should have only a single space after a punctuation in a comment. [whitespace/comments] [5] ERROR: Source/WebCore/platform/network/ResourceRequestBase.h:64: The parameter name "url" adds no information, so it should be removed. [readability/parameter_name] [5] ERROR: Source/WebCore/platform/network/ResourceRequestBase.h:69: The parameter name "cachePolicy" adds no information, so it should be removed. [readability/parameter_name] [5] ERROR: Source/WebCore/platform/network/ResourceRequestBase.h:72: The parameter name "timeoutInterval" adds no information, so it should be removed. [readability/parameter_name] [5] ERROR: Source/WebCore/platform/network/ResourceRequestBase.h:78: The parameter name "httpMethod" adds no information, so it should be removed. [readability/parameter_name] [5] ERROR: Source/WebCore/platform/network/ResourceRequestBase.h:126: The parameter name "allowCookies" adds no information, so it should be removed. [readability/parameter_name] [5] ERROR: Source/WebCore/platform/network/ResourceRequestBase.h:151: enum members should use InterCaps with an initial capital letter or initial 'k' for C-style enums. [readability/enum_casing] [4] Total errors found: 8 in 13 files If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 280337 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=280337&action=review Looks fine. Really inconvenient for Youenn who has a patch for this same file. The re-indenting is what will screw him up. Seems like we need even more functions that make it clear they do isolated copying with proper naming. Function names like deepCopy and copyData don’t make it clear enough, I think. > Source/WebCore/platform/network/ResourceRequestBase.cpp:93 > std::unique_ptr<CrossThreadResourceRequestData> ResourceRequestBase::copyData() const Unclear why this returns a unique_ptr instead of a CrossThreadResourceRequestData. Maybe predates move semantics. Seems like this function needs “isolated” in its name. > Source/WebCore/platform/network/cf/ResourceRequest.h:160 > +inline bool ResourceRequest::resourcePrioritiesEnabled() > +{ > #if PLATFORM(MAC) > - return true; > +return true; > #elif PLATFORM(IOS) > - return true; > +return true; > #elif PLATFORM(WIN) > - return false; > +return false; > #endif > - } > +} Wrong indenting here for the return statements.
(In reply to comment #7) > Comment on attachment 280337 [details] > Patch > Seems like we need even more functions that make it clear they do isolated > copying with proper naming. Function names like deepCopy and copyData don’t > make it clear enough, I think. From now on, whenever I see such a function, I plan on changing it. That said, for these classes, the CrossThread*Data will be going away next (mentioned in https://bugs.webkit.org/show_bug.cgi?id=158293#c4) > > Source/WebCore/platform/network/ResourceRequestBase.cpp:93 > > std::unique_ptr<CrossThreadResourceRequestData> ResourceRequestBase::copyData() const > > Unclear why this returns a unique_ptr instead of a > CrossThreadResourceRequestData. Maybe predates move semantics. Yup, it predated Move semantics. > > Seems like this function needs “isolated” in its name. Not too concerned, as it will be going away in the next patch. > > Source/WebCore/platform/network/cf/ResourceRequest.h:160 > > +inline bool ResourceRequest::resourcePrioritiesEnabled() > > +{ > > #if PLATFORM(MAC) > > - return true; > > +return true; > > #elif PLATFORM(IOS) > > - return true; > > +return true; > > #elif PLATFORM(WIN) > > - return false; > > +return false; > > #endif > > - } > > +} > > Wrong indenting here for the return statements. Whoops! Will fix.
http://trac.webkit.org/changeset/201603
It broke the WinCairo build. cc-ing port maintainers
(In reply to comment #10) > It broke the WinCairo build. cc-ing port maintainers One of these days, the response to "this broke WinCairo" will either be "Let's drop WinCairo" or "let's actually get a WinCairo EWS" *sigh*
Comment on attachment 280337 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=280337&action=review > Source/WebCore/platform/network/ResourceResponseBase.cpp:69 > + auto data = copyData(); copyData() de-atomizes our data members.... > Source/WebCore/platform/network/ResourceResponseBase.cpp:72 > + response.setMimeType(data->m_mimeType); ... but then you call the data members right away (in the same thread) which re-atomizes them in the wrong thread. The previous code was fine because we would call copyData() in threadA and adopt() in threadB. So we would de-atomize in thread A and re-atomize them in threadB.
Comment on attachment 280337 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=280337&action=review >> Source/WebCore/platform/network/ResourceResponseBase.cpp:72 >> + response.setMimeType(data->m_mimeType); > > ... but then you call the data members right away (in the same thread) which re-atomizes them in the wrong thread. > > The previous code was fine because we would call copyData() in threadA and adopt() in threadB. So we would de-atomize in thread A and re-atomize them in threadB. I don't think there is a "quick" fix for this so I would suggest rolling out the patch until we figure out a nice and safe way of doing this.
(In reply to comment #13) > Comment on attachment 280337 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=280337&action=review > > >> Source/WebCore/platform/network/ResourceResponseBase.cpp:72 > >> + response.setMimeType(data->m_mimeType); > > > > ... but then you call the data members right away (in the same thread) which re-atomizes them in the wrong thread. > > > > The previous code was fine because we would call copyData() in threadA and adopt() in threadB. So we would de-atomize in thread A and re-atomize them in threadB. I see. That's a detail of AtomicStrings I didn't fully grok. > > I don't think there is a "quick" fix for this so I would suggest rolling out > the patch until we figure out a nice and safe way of doing this. Other patches have gone in on top of this in the meantime. I'm going to stare at it for a little bit first.
I'm going to restore (an updated...) cross thread data just for ResourceResponse, which is the only one with this problem.
(In reply to comment #15) > I'm going to restore (an updated...) cross thread data just for > ResourceResponse, which is the only one with this problem. If you do, it'd be nice if isolatedCopy() returned a CrossThreadResourceResponseData instead of a std::unique_ptr<CrossThreadResourceResponseData>. Also, it'd be nice if we could implicitly construct a ResourceResponse from a CrossThreadResourceResponseData I think. This way, the lambda capture side would look pretty much identical as after your initial patch.
(In reply to comment #16) > (In reply to comment #15) > > I'm going to restore (an updated...) cross thread data just for > > ResourceResponse, which is the only one with this problem. > > If you do, it'd be nice if isolatedCopy() returned a > CrossThreadResourceResponseData instead of a > std::unique_ptr<CrossThreadResourceResponseData>. Also, it'd be nice if we > could implicitly construct a ResourceResponse from a > CrossThreadResourceResponseData I think. I commented in the other bug why I think the implicit constructor is a bad idea. > This way, the lambda capture side would look pretty much identical as after your initial patch. I don't think it's worth covering up the heavy handedness of the cross thread data object just to keep the lambda capture/body looking the same way they do now.