Bug 213714 – Make NetworkSendQueue use CString instead of String for UTF-8 data

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

[youenn fablet]

Make NetworkSendQueue use Cstring instead of String for UTF-8 data

[youenn fablet]

Created attachment 403040 [details]
Patch

[youenn fablet]

Created attachment 403053 [details]
Patch

[youenn fablet]

Created attachment 403059 [details]
Patch

[Darin Adler]

Comment on attachment 403059 [details]
Patch

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

Test failures look like they might be caused by the refactoring.

> Source/WebCore/Modules/mediastream/RTCDataChannel.cpp:124
> +    // FIXME: We might want to use strict conversion like WebSocket.

Maybe test coverage would help us decide this?

> Source/WebCore/fileapi/NetworkSendQueue.h:49
> +    using WriteString = Function<void(const CString&)>;

Might be helpful to make it clear that this is UTF-8 encoded. In WTF::String that’s included in the definition of the class, but not WTF::CString, so we might need an argument name?

> Source/WebCore/fileapi/NetworkSendQueue.h:56
> +    void enqueue(const CString&);

Would be nice to take a CString&&, add some WTFMove at call sites, and save a little bit of reference count churn.

Might also be helpful to make it clear that this is UTF-8 encoded. In WTF::String that’s included in the definition of the class, but not WTF::CString, so we might need an argument name?

> Source/WebKit/NetworkProcess/cocoa/WebSocketTaskCocoa.mm:120
> +    auto text = adoptNS([[NSString alloc] initWithBytes:utf8String.data() length:utf8String.size() encoding:NSUTF8StringEncoding]);

Do we have a guarantee that this won’t fail UTF-8 decoding? If not, might need a null check.

> Source/WebKit/NetworkProcess/cocoa/WebSocketTaskCocoa.mm:121
> +    auto message = adoptNS([[NSURLSessionWebSocketMessage alloc] initWithString: text.get()]);

Stray space after the colon.

[youenn fablet]

(In reply to Darin Adler from comment #4)
> Comment on attachment 403059 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=403059&action=review
> 
> Test failures look like they might be caused by the refactoring.
> 
> > Source/WebCore/Modules/mediastream/RTCDataChannel.cpp:124
> > +    // FIXME: We might want to use strict conversion like WebSocket.
> 
> Maybe test coverage would help us decide this?

Right, this patch keeps the status quo and I plan to validate one way or the other as a follow-up.

> > Source/WebCore/fileapi/NetworkSendQueue.h:49
> > +    using WriteString = Function<void(const CString&)>;
> 
> Might be helpful to make it clear that this is UTF-8 encoded. In WTF::String
> that’s included in the definition of the class, but not WTF::CString, so we
> might need an argument name?

OK

> > Source/WebCore/fileapi/NetworkSendQueue.h:56
> > +    void enqueue(const CString&);
> 
> Would be nice to take a CString&&, add some WTFMove at call sites, and save
> a little bit of reference count churn.

I'll look into that.
This is not done as in most cases, no count churn will anyway happen (it only happens in case of mixing blob calls with string calls, which is unusual). 

But, since the call sites create the Ref and do not use after...

> Might also be helpful to make it clear that this is UTF-8 encoded. In
> WTF::String that’s included in the definition of the class, but not
> WTF::CString, so we might need an argument name?

OK.

> > Source/WebKit/NetworkProcess/cocoa/WebSocketTaskCocoa.mm:120
> > +    auto text = adoptNS([[NSString alloc] initWithBytes:utf8String.data() length:utf8String.size() encoding:NSUTF8StringEncoding]);
> 
> Do we have a guarantee that this won’t fail UTF-8 decoding? If not, might
> need a null check.

This comes straight from IPC so we cannot be sure.
Will fix this.

> > Source/WebKit/NetworkProcess/cocoa/WebSocketTaskCocoa.mm:121
> > +    auto message = adoptNS([[NSURLSessionWebSocketMessage alloc] initWithString: text.get()]);
> 
> Stray space after the colon.

OK

[youenn fablet]

Created attachment 403186 [details]
Patch

[Darin Adler]

Windows crash seems related?

[Darin Adler]

Comment on attachment 403186 [details]
Patch

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

r=me assuming Windows failures are not due to these changes

> Source/WebCore/fileapi/NetworkSendQueue.h:49
> +    using WriteString = Function<void(const CString& /* utf8 */)>;

Could use an argument name. It does not need to be commented out. Also, in a comment I would spell it UTF-8, not utf8.

> Source/WebCore/fileapi/NetworkSendQueue.h:56
> +    void enqueue(CString&& /* utf8 */);

Ditto.

[youenn fablet]

Win failures seem to be due to http://trac.webkit.org/changeset/263700/webkit as per https://results.webkit.org/?suite=layout-tests&test=http%2Fwpt%2Ffetch%2Frequest-stream-disturbed-1.html.

[youenn fablet]

(In reply to youenn fablet from comment #9)
> Win failures seem to be due to
> http://trac.webkit.org/changeset/263700/webkit as per
> https://results.webkit.org/?suite=layout-
> tests&test=http%2Fwpt%2Ffetch%2Frequest-stream-disturbed-1.html.

https://bugs.webkit.org/show_bug.cgi?id=213792

[youenn fablet]

Created attachment 403277 [details]
Patch for landing

[EWS]

Committed r263797: <https://trac.webkit.org/changeset/263797>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 403277 [details].

[Radar WebKit Bug Importer]

<rdar://problem/64986255>