Bug 158293 – Overhaul cross-thread use of ResourceRequest, ResourceResponse, and ResourceError

• Component: WebCore Misc.
• Status: RESOLVED FIXED

[Brady Eidson]

Add isolatedCopy() to ResourceRequest, ResourceResponse, and ResourceError

[Brady Eidson]

Note ResourceError.copy() is already an isolatedCopy, but you wouldn't know that by name.

[Brady Eidson]

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.

[Brady Eidson]

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.

[Brady Eidson]

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.

[Brady Eidson]

Created attachment 280337 [details]
Patch

[WebKit Commit Bot]

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.

[Darin Adler]

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.

[Brady Eidson]

(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.

[Brady Eidson]

http://trac.webkit.org/changeset/201603

[Csaba Osztrogonác]

It broke the WinCairo build. cc-ing port maintainers

[Brady Eidson]

(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*

[Chris Dumez]

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.

[Chris Dumez]

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.

[Brady Eidson]

(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.

[Brady Eidson]

I'm going to restore (an updated...) cross thread data just for ResourceResponse, which is the only one with this problem.

[Chris Dumez]

(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.

[Brady Eidson]

(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.