Bug 213245 - iOS Safari incorrectly reports "AppleCoreMedia" as UA string
Summary: iOS Safari incorrectly reports "AppleCoreMedia" as UA string
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Media (show other bugs)
Version: Safari 13
Hardware: iPhone / iPad iOS 13
: P2 Normal
Assignee: Jer Noble
URL:
Keywords: InRadar
Depends on: 213640
Blocks:
  Show dependency treegraph
 
Reported: 2020-06-16 06:24 PDT by John Luther
Modified: 2020-06-27 12:31 PDT (History)
7 users (show)

See Also:


Attachments
Patch (11.84 KB, patch)
2020-06-24 23:14 PDT, Jer Noble
no flags Details | Formatted Diff | Diff
Patch for landing (11.79 KB, patch)
2020-06-25 10:15 PDT, Jer Noble
no flags Details | Formatted Diff | Diff
Patch for landing (11.79 KB, patch)
2020-06-25 13:07 PDT, Jer Noble
no flags Details | Formatted Diff | Diff
Patch for landing (11.75 KB, patch)
2020-06-25 13:23 PDT, Jer Noble
no flags Details | Formatted Diff | Diff
Patch for landing (11.78 KB, patch)
2020-06-26 16:36 PDT, Jer Noble
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description John Luther 2020-06-16 06:24:49 PDT
Safari on iOS reports an incorrect user agent string (AppleCoreMedia). This impacts us at JW Player by making it impossible to distinguish between native mobile apps and mobile web in our analytics. 

Filing this bug at the recommendation of Jer Noble.
Comment 1 Alexey Proskuryakov 2020-06-17 18:07:35 PDT
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?
Comment 2 Radar WebKit Bug Importer 2020-06-17 18:07:47 PDT
<rdar://problem/64471582>
Comment 3 Jer Noble 2020-06-17 20:21:56 PDT
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.
Comment 4 Jer Noble 2020-06-17 20:48:09 PDT
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.
Comment 5 Alexey Proskuryakov 2020-06-18 09:26:39 PDT
I see, so this was a misunderstanding: "loading didn't go through WebKit".
Comment 6 Jer Noble 2020-06-24 23:14:04 PDT
Created attachment 402718 [details]
Patch
Comment 7 youenn fablet 2020-06-25 00:38:22 PDT
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.
Comment 8 Jer Noble 2020-06-25 10:15:14 PDT
Created attachment 402744 [details]
Patch for landing
Comment 9 EWS 2020-06-25 13:00:57 PDT
ChangeLog entry in Source/WebCore/ChangeLog contains OOPS!.
Comment 10 Alex Christensen 2020-06-25 13:04:48 PDT
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?
Comment 11 Jer Noble 2020-06-25 13:07:00 PDT
Created attachment 402799 [details]
Patch for landing
Comment 12 Jer Noble 2020-06-25 13:11:08 PDT
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 13 Alex Christensen 2020-06-25 13:13:17 PDT
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.
Comment 14 Jer Noble 2020-06-25 13:23:11 PDT
Created attachment 402802 [details]
Patch for landing
Comment 15 EWS 2020-06-25 15:39:26 PDT
Committed r263537: <https://trac.webkit.org/changeset/263537>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 402802 [details].
Comment 16 Jonathan Bedard 2020-06-25 21:59:25 PDT
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)
Comment 17 Yusuke Suzuki 2020-06-25 22:01:21 PDT
Re-opened since this is blocked by bug 213640
Comment 18 Jer Noble 2020-06-26 16:32:53 PDT
Okay, looks like tvOS and watchOS don't `HAVE(NETWORK_FRAMEWORK)`. Just needs a compile guard around the test case.
Comment 19 Jer Noble 2020-06-26 16:36:59 PDT
Created attachment 402926 [details]
Patch for landing
Comment 20 EWS 2020-06-27 12:31:20 PDT
Committed r263627: <https://trac.webkit.org/changeset/263627>

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