Bug 213245

Jer Noble would know better, but how is this a WebKit issue? This indicates that loading didn't go through WebKit, as it should have.

Do you have steps to reproduce that would work for us?

<rdar://problem/64471582>

We pass a custom user-agent string to AVFoundation in a dictionary of header values, but that particular one appears to be ignored. We can work around this behavior by overriding the user-agent value at the WebCoreNSURLSession level.

And the issue is trivially reproducible. You can verify that we're sending the wrong UA string in WebInspector with any page with a <video> element.

I see, so this was a misunderstanding: "loading didn't go through WebKit".

Created attachment 402718 [details]
Patch

Comment on attachment 402718 [details]
Patch

LGTM.
In theory, CoreMedia should probably split their code so that they only set these headers (user-agent, accept...) in the code that actually sends the requests themselves.

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

> Tools/TestWebKitAPI/Tests/WebKitCocoa/MediaLoading.mm:38
> +static String parseUserAgent(Vector<char>&& request)

const Vector<char>&

> Tools/TestWebKitAPI/Tests/WebKitCocoa/MediaLoading.mm:40
> +    Vector<String> headers = String::fromUTF8(request.data(), request.size()).split("\r\n");

auto

> Tools/TestWebKitAPI/Tests/WebKitCocoa/MediaLoading.mm:41
> +    auto index = headers.findMatching([] (const String& header) { return header.startsWith("User-Agent:"); });

auto

> Tools/TestWebKitAPI/Tests/WebKitCocoa/MediaLoading.mm:57
> +        connection.receiveHTTPRequest([&] (Vector<char>&& request) {

auto&&

> Tools/TestWebKitAPI/Tests/WebKitCocoa/MediaLoading.mm:58
> +            auto userAgent = parseUserAgent(WTFMove(request));

no need to move. Ditto below.

Created attachment 402744 [details]
Patch for landing

ChangeLog entry in Source/WebCore/ChangeLog contains OOPS!.

Comment on attachment 402744 [details]
Patch for landing

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

source change looks fine

> Tools/TestWebKitAPI/Tests/WebKitCocoa/MediaLoading.mm:38
> +static String parseUserAgent(const Vector<char>& request)

Couldn't you just look for "User-Agent: TestWebKitAPI\r\n" in the request instead of this?

> Tools/TestWebKitAPI/Tests/WebKitCocoa/MediaLoading.mm:52
> +    webView.get()._customUserAgent = @"TestWebKitAPI";

Could you use customUserAgent instead of _customUserAgent?

> Tools/TestWebKitAPI/Tests/WebKitCocoa/MediaLoading.mm:105
> +            connection.send(makeString("HTTP/1.1 200 OK\r\n",

Does this test even need to send a reply?

Created attachment 402799 [details]
Patch for landing

Comment on attachment 402744 [details]
Patch for landing

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

>> Tools/TestWebKitAPI/Tests/WebKitCocoa/MediaLoading.mm:38
>> +static String parseUserAgent(const Vector<char>& request)
> 
> Couldn't you just look for "User-Agent: TestWebKitAPI\r\n" in the request instead of this?

I could; but with this way, a failing test shows the incorrect User-Agent in the test output, rather than just a "strncmp expected TRUE, got FALSE".

>> Tools/TestWebKitAPI/Tests/WebKitCocoa/MediaLoading.mm:52
>> +    webView.get()._customUserAgent = @"TestWebKitAPI";
> 
> Could you use customUserAgent instead of _customUserAgent?

I didn't know that was a thing! Yes, it could.

>> Tools/TestWebKitAPI/Tests/WebKitCocoa/MediaLoading.mm:105
>> +            connection.send(makeString("HTTP/1.1 200 OK\r\n",
> 
> Does this test even need to send a reply?

Yes it does. I wanted to make sure both the initial request for the manifest contained the correct MIME type, and also follow-up requests for media segments also did.  Without sending a correct reply, the follow up media request will never be sent.

Comment on attachment 402799 [details]
Patch for landing

Aha, you're right.  I thought receivedManifestRequest and receivedMediaRequest were the same.  It turns out, they're not the same.

Created attachment 402802 [details]
Patch for landing

Committed r263537: <https://trac.webkit.org/changeset/263537>

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

This broke tvOS and watchOS builds: https://build.webkit.org/builders/Apple-tvOS-13-Release-Build/builds/171

(we don't have EWS yet, operations has been consumed by Big Sur work, it's coming soon)

Re-opened since this is blocked by bug 213640

Okay, looks like tvOS and watchOS don't `HAVE(NETWORK_FRAMEWORK)`. Just needs a compile guard around the test case.

Created attachment 402926 [details]
Patch for landing

Committed r263627: <https://trac.webkit.org/changeset/263627>

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