Bug 152919 – Custom protocol loading through AVFoundation does not support byte-range requests.

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Jer Noble]

Custom protocol loading through AVFoundation does not support byte-range requests.

[Jer Noble]

Created attachment 268695 [details]
Patch

[Jer Noble]

Created attachment 268721 [details]
Patch

[Alex Christensen]

Comment on attachment 268721 [details]
Patch

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

> Source/WebCore/platform/graphics/avfoundation/objc/WebCoreAVFResourceLoader.mm:68
>      URL requestURL = [[m_avRequest.get() request] URL];

You should store the result of [m_avRequest.get() request] to avoid having to call it twice.

> Source/WebCore/platform/graphics/avfoundation/objc/WebCoreAVFResourceLoader.mm:162
> +    ParsedContentRange contentRange = resource->response().contentRange();

resource should be a reference, or it should be null-checked before using it for the first time, which in this case was before setting data.

> Source/WebCore/platform/network/BlobResourceHandle.h:104
>      long long m_rangeSuffixLength;
> +    long long m_totalSize { 0 };

I think you should add the { 0 } for all of these, and remove the initializers in the constructors.  As it is now, it just looks inconsistent.

> Source/WebCore/platform/network/ResourceResponseBase.cpp:456
> +        m_haveParsedContentRangeHeader = true;

I think this needs to be set back to false in ResourceResponseBase::updateHeaderParsedState.

> Source/WebCore/xml/XMLHttpRequest.cpp:433
> +    case HTTPHeaderName::ContentRange:

Why is this being added as a forbidden request header?  https://fetch.spec.whatwg.org/#forbidden-header-name says it's ok, and it will probably be ignored by the server, right?

[Build Bot]

Comment on attachment 268721 [details]
Patch

Attachment 268721 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.webkit.org/results/680339

New failing tests:
http/tests/xmlhttprequest/blob-request-byte-range.html

[Build Bot]

Created attachment 268727 [details]
Archive of layout-test-results from ews103 for mac-yosemite

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews103  Port: mac-yosemite  Platform: Mac OS X 10.10.5

[Build Bot]

Comment on attachment 268721 [details]
Patch

Attachment 268721 [details] did not pass mac-debug-ews (mac):
Output: http://webkit-queues.webkit.org/results/680331

New failing tests:
http/tests/xmlhttprequest/blob-request-byte-range.html

[Build Bot]

Created attachment 268728 [details]
Archive of layout-test-results from ews112 for mac-yosemite

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews112  Port: mac-yosemite  Platform: Mac OS X 10.10.5

[Build Bot]

Comment on attachment 268721 [details]
Patch

Attachment 268721 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/680349

New failing tests:
http/tests/xmlhttprequest/blob-request-byte-range.html

[Build Bot]

Created attachment 268729 [details]
Archive of layout-test-results from ews106 for mac-yosemite-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews106  Port: mac-yosemite-wk2  Platform: Mac OS X 10.10.5

[Alex Christensen]

Comment on attachment 268721 [details]
Patch

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

Make the current tests pass.

> Source/WebCore/platform/network/ParsedContentRange.cpp:92
> +        instanceLength = headerValue.substring(instanceLengthSeparatorToken + 1).toUInt64();

What if there is no valid number or '*' here?  I think toUInt64 would return 0, but it should fail if you have something like "bytes asdf-1/2" or "bytes 0-asdf/*" or "bytes 0-1/asdf".  What should happen if there's whitespace, like "byte 0    - 1  / *"?  I think right now we would succeed parsing most of that, but the * would not succeed if there is whitespace before it.

Please add tests with strange and invalid headers.

> Source/WebCore/platform/network/ParsedContentRange.cpp:94
> +    return areContentRangeValuesValid(firstBytePosition, lastBytePosition, instanceLength);

I think this would be nicer if it only set any of the output variables if it succeeded.  Then it would be impossible to get a firstBytePosition without a lastBytePosition, for example.

[Jer Noble]

Comment on attachment 268721 [details]
Patch

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

>> Source/WebCore/platform/graphics/avfoundation/objc/WebCoreAVFResourceLoader.mm:68
>>      URL requestURL = [[m_avRequest.get() request] URL];
> 
> You should store the result of [m_avRequest.get() request] to avoid having to call it twice.

Not only that, but since requestURL is only used in a LOG() statement, this will throw a "unused variable" warning in release builds. Will fix.

>> Source/WebCore/platform/graphics/avfoundation/objc/WebCoreAVFResourceLoader.mm:162
>> +    ParsedContentRange contentRange = resource->response().contentRange();
> 
> resource should be a reference, or it should be null-checked before using it for the first time, which in this case was before setting data.

I think an ASSERT is fine here, as getting a null value here would indicate that the caller of CashedResourceClient was behaving badly.

>> Source/WebCore/platform/network/BlobResourceHandle.h:104
>> +    long long m_totalSize { 0 };
> 
> I think you should add the { 0 } for all of these, and remove the initializers in the constructors.  As it is now, it just looks inconsistent.

Sure.

>> Source/WebCore/platform/network/ParsedContentRange.cpp:92
>> +        instanceLength = headerValue.substring(instanceLengthSeparatorToken + 1).toUInt64();
> 
> What if there is no valid number or '*' here?  I think toUInt64 would return 0, but it should fail if you have something like "bytes asdf-1/2" or "bytes 0-asdf/*" or "bytes 0-1/asdf".  What should happen if there's whitespace, like "byte 0    - 1  / *"?  I think right now we would succeed parsing most of that, but the * would not succeed if there is whitespace before it.
> 
> Please add tests with strange and invalid headers.

With this patch, both "0-asdf/*" and "asdf-1/*" will fail, but "0-1/asdf" will not. I'll fix it so that the latter one will fail.

I'll make the "*" check whitespace agnostic as well.

>> Source/WebCore/platform/network/ParsedContentRange.cpp:94
>> +    return areContentRangeValuesValid(firstBytePosition, lastBytePosition, instanceLength);
> 
> I think this would be nicer if it only set any of the output variables if it succeeded.  Then it would be impossible to get a firstBytePosition without a lastBytePosition, for example.

I considered this, but thought that, for diagnostic purposes, it might be nice to see the values which caused the range to become invalid.

>> Source/WebCore/platform/network/ResourceResponseBase.cpp:456
>> +        m_haveParsedContentRangeHeader = true;
> 
> I think this needs to be set back to false in ResourceResponseBase::updateHeaderParsedState.

Ok.

>> Source/WebCore/xml/XMLHttpRequest.cpp:433
>> +    case HTTPHeaderName::ContentRange:
> 
> Why is this being added as a forbidden request header?  https://fetch.spec.whatwg.org/#forbidden-header-name says it's ok, and it will probably be ignored by the server, right?

It will be ignored by the server. I'm guessing that, since Content-Length is a forbidden header, the lack of Content-Range in the spec is an oversight. But until they update the spec to add it, i'll remove it here.

[Jer Noble]

Created attachment 268821 [details]
Patch

[Jer Noble]

Created attachment 268822 [details]
Patch

Forgot the change to clear m_haveParsedContentRangeHeader in updateHeaderParsedState().

[Jer Noble]

Created attachment 268824 [details]
Patch

[Alex Christensen]

Comment on attachment 268824 [details]
Patch

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

> Source/WebCore/platform/graphics/avfoundation/objc/WebCoreAVFResourceLoader.mm:79
> +        LOG_ERROR("Failed to start load for media at url %s", URL([nsRequest URL]).string().ascii().data());

Since we have an NSURL here, may as well do [[nsurl absoluteString] UTF8String]

> Source/WebCore/platform/graphics/avfoundation/objc/WebCoreAVFResourceLoader.mm:118
> +        ParsedContentRange contentRange = resource->response().contentRange();

There's no need to copy the whole data structure.  Use ParsedContentRange&
Also, I don't think resource is null-checked.

> Source/WebCore/platform/graphics/avfoundation/objc/WebCoreAVFResourceLoader.mm:152
>  void WebCoreAVFResourceLoader::fulfillRequestWithResource(CachedResource* resource)
>  {
> +    ASSERT(resource);

Maybe for a followup patch, but if resource is always non-null it should be a CachedResource&.  Same with WebCoreAVFResourceLoader::responseReceived

> Source/WebCore/platform/graphics/avfoundation/objc/WebCoreAVFResourceLoader.mm:163
> +    ParsedContentRange contentRange = resource->response().contentRange();

As the patch is right now, I think responseOffset should be left as 0 if resource is null.

> Source/WebCore/platform/network/BlobResourceHandle.h:105
> +    long long m_totalSize { 0 };
>      long long m_totalRemainingSize;

You didn't make all of these { 0 };
:(

> Source/WebCore/platform/network/ResourceResponseBase.h:43
> +class ParsedContentRange;

You shouldn't need to forward declare this if you include ParsedContentRange.h.

> Tools/ChangeLog:18
> [phone]  Jer Noble  <[e-mail]>

Too many changelog entries.

> Tools/TestWebKitAPI/Tests/WebCore/ParsedContentRange.cpp:35
> +TEST(WebCore, ParsedContentRangeFromString)

Add a test if -0 is the start of the range.

> Tools/TestWebKitAPI/Tests/WebCore/ParsedContentRange.cpp:39
> +    ASSERT_TRUE(ParsedContentRange("bytes  0 - 1 / * ").isValid());

This is probably looser than the RFC standard.

[Jer Noble]

Comment on attachment 268824 [details]
Patch

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

>> Source/WebCore/platform/graphics/avfoundation/objc/WebCoreAVFResourceLoader.mm:79
>> +        LOG_ERROR("Failed to start load for media at url %s", URL([nsRequest URL]).string().ascii().data());
> 
> Since we have an NSURL here, may as well do [[nsurl absoluteString] UTF8String]

Ok.

>> Source/WebCore/platform/graphics/avfoundation/objc/WebCoreAVFResourceLoader.mm:118
>> +        ParsedContentRange contentRange = resource->response().contentRange();
> 
> There's no need to copy the whole data structure.  Use ParsedContentRange&
> Also, I don't think resource is null-checked.

Ok.

Will add an ASSERT(resource) (see below).

>> Source/WebCore/platform/graphics/avfoundation/objc/WebCoreAVFResourceLoader.mm:152
>> +    ASSERT(resource);
> 
> Maybe for a followup patch, but if resource is always non-null it should be a CachedResource&.  Same with WebCoreAVFResourceLoader::responseReceived

Unfortunately, this would require re-writing every client of CachedResourceClient. (Which maybe we should do, but would absolutely need to be in a follow-up patch).

>> Source/WebCore/platform/graphics/avfoundation/objc/WebCoreAVFResourceLoader.mm:163
>> +    ParsedContentRange contentRange = resource->response().contentRange();
> 
> As the patch is right now, I think responseOffset should be left as 0 if resource is null.

We asserted above that resource will never be null.

>> Source/WebCore/platform/network/BlobResourceHandle.h:105
>>      long long m_totalRemainingSize;
> 
> You didn't make all of these { 0 };
> :(

Whoops!

>> Source/WebCore/platform/network/ResourceResponseBase.h:43
>> +class ParsedContentRange;
> 
> You shouldn't need to forward declare this if you include ParsedContentRange.h.

Whoops!

>> Tools/ChangeLog:18
>> [phone]  Jer Noble  <[e-mail]>
> 
> Too many changelog entries.

Whoops!

>> Tools/TestWebKitAPI/Tests/WebCore/ParsedContentRange.cpp:35
>> +TEST(WebCore, ParsedContentRangeFromString)
> 
> Add a test if -0 is the start of the range.

Added. Fails correctly.

>> Tools/TestWebKitAPI/Tests/WebCore/ParsedContentRange.cpp:39
>> +    ASSERT_TRUE(ParsedContentRange("bytes  0 - 1 / * ").isValid());
> 
> This is probably looser than the RFC standard.

Will tighten.

[Jer Noble]

Created attachment 268883 [details]
Patch

[Alex Christensen]

Comment on attachment 268883 [details]
Patch

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

r=me

> Tools/TestWebKitAPI/Tests/WebCore/ParsedContentRange.cpp:79
> +    ASSERT_FALSE(ParsedContentRange(0, 2, 1).isValid());

Add some negative values here and (0, 0, 0).

[Build Bot]

Comment on attachment 268883 [details]
Patch

Attachment 268883 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.webkit.org/results/687383

New failing tests:
http/tests/xmlhttprequest/blob-request-byte-range.html

[Build Bot]

Created attachment 268888 [details]
Archive of layout-test-results from ews101 for mac-yosemite

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews101  Port: mac-yosemite  Platform: Mac OS X 10.10.5

[Build Bot]

Comment on attachment 268883 [details]
Patch

Attachment 268883 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/687384

New failing tests:
http/tests/xmlhttprequest/blob-request-byte-range.html

[Build Bot]

Created attachment 268889 [details]
Archive of layout-test-results from ews104 for mac-yosemite-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews104  Port: mac-yosemite-wk2  Platform: Mac OS X 10.10.5

[Build Bot]

Comment on attachment 268883 [details]
Patch

Attachment 268883 [details] did not pass mac-debug-ews (mac):
Output: http://webkit-queues.webkit.org/results/687391

New failing tests:
http/tests/xmlhttprequest/blob-request-byte-range.html

[Build Bot]

Created attachment 268892 [details]
Archive of layout-test-results from ews112 for mac-yosemite

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews112  Port: mac-yosemite  Platform: Mac OS X 10.10.5

[Jer Noble]

(In reply to comment #24)
> Created attachment 268892 [details]
> Archive of layout-test-results from ews112 for mac-yosemite
> 
> The attached test failures were seen while running run-webkit-tests on the
> mac-debug-ews.
> Bot: ews112  Port: mac-yosemite  Platform: Mac OS X 10.10.5

Sigh. The expected.txt is missing an extra line-feed. Will fix.

[Jer Noble]

Created attachment 268898 [details]
Patch for landing

[Jer Noble]

Created attachment 268986 [details]
Patch for landing

[Jer Noble]

Created attachment 269009 [details]
Patch for landing

[Jer Noble]

Created attachment 269019 [details]
Patch for landing

[Jer Noble]

Created attachment 269020 [details]
Patch for landing

[Jer Noble]

Created attachment 269281 [details]
Patch for landing

[Jer Noble]

Created attachment 269285 [details]
Patch for landing

[Jer Noble]

Created attachment 270042 [details]
Patch for landing

[Jer Noble]

Created attachment 270043 [details]
Patch for landing

[Jer Noble]

I think the Windows EWS bot was broken at the time I added the last Patch for landing; adding another one to see if it passes this time.

[Build Bot]

Comment on attachment 270043 [details]
Patch for landing

Attachment 270043 [details] did not pass mac-debug-ews (mac):
Output: http://webkit-queues.webkit.org/results/747293

New failing tests:
imported/w3c/web-platform-tests/streams-api/readable-streams/garbage-collection-1.html

[Build Bot]

Created attachment 270056 [details]
Archive of layout-test-results from ews113 for mac-yosemite

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews113  Port: mac-yosemite  Platform: Mac OS X 10.10.5

[Jer Noble]

Committed r195764: <http://trac.webkit.org/changeset/195764>

[Csaba Osztrogonác]

(In reply to comment #38)
> Committed r195764: <http://trac.webkit.org/changeset/195764>

It broke the Windows build.

[Jer Noble]

Alex fixed it in <https://trac.webkit.org/changeset/195782>