Bug 153669 – [Mac] Wrap a resource and resource loader in a NSURLSession-like object for use by lower level frameworks

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

[Jer Noble]

[Mac] Wrap a resource and resource loader in a NSURLSession-like object for use by lower level frameworks

[Jer Noble]

Created attachment 270257 [details]
Patch

[WebKit Commit Bot]

Attachment 270257 [details] did not pass style-queue:


ERROR: Tools/TestWebKitAPI/Configurations/FeatureDefines.xcconfig:0:  Patch introduces a new FeatureDefines.xcconfig, which check-webkit-style doesn't know about. Please add it to the list in featuredefines.py.  [featuredefines/new] [5]
ERROR: Source/WebCore/platform/network/cocoa/WebCoreNSURLSession.mm:45:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/WebCore/platform/network/cocoa/WebCoreNSURLSession.mm:58:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/WebCore/platform/network/cocoa/WebCoreNSURLSession.mm:471:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
Total errors found: 4 in 9 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Jer Noble]

Created attachment 270423 [details]
Patch

[WebKit Commit Bot]

Attachment 270423 [details] did not pass style-queue:


ERROR: Tools/TestWebKitAPI/Configurations/FeatureDefines.xcconfig:0:  Patch introduces a new FeatureDefines.xcconfig, which check-webkit-style doesn't know about. Please add it to the list in featuredefines.py.  [featuredefines/new] [5]
ERROR: Source/WebCore/platform/network/cocoa/WebCoreNSURLSession.mm:460:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
Total errors found: 2 in 12 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Jer Noble]

Created attachment 270427 [details]
Patch

[WebKit Commit Bot]

Attachment 270427 [details] did not pass style-queue:


ERROR: Tools/TestWebKitAPI/Configurations/FeatureDefines.xcconfig:0:  Patch introduces a new FeatureDefines.xcconfig, which check-webkit-style doesn't know about. Please add it to the list in featuredefines.py.  [featuredefines/new] [5]
ERROR: Source/WebCore/platform/network/cocoa/WebCoreNSURLSession.mm:453:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
Total errors found: 2 in 12 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Jer Noble]

Created attachment 270434 [details]
Patch

[WebKit Commit Bot]

Attachment 270434 [details] did not pass style-queue:


ERROR: Tools/TestWebKitAPI/Configurations/FeatureDefines.xcconfig:0:  Patch introduces a new FeatureDefines.xcconfig, which check-webkit-style doesn't know about. Please add it to the list in featuredefines.py.  [featuredefines/new] [5]
ERROR: Source/WebCore/platform/network/cocoa/WebCoreNSURLSession.mm:468:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
Total errors found: 2 in 12 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Jer Noble]

Created attachment 270447 [details]
Patch

[WebKit Commit Bot]

Attachment 270447 [details] did not pass style-queue:


ERROR: Tools/TestWebKitAPI/Configurations/FeatureDefines.xcconfig:0:  Patch introduces a new FeatureDefines.xcconfig, which check-webkit-style doesn't know about. Please add it to the list in featuredefines.py.  [featuredefines/new] [5]
ERROR: Source/WebCore/platform/network/cocoa/WebCoreNSURLSession.mm:469:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
Total errors found: 2 in 12 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Jer Noble]

Created attachment 270452 [details]
Patch

[WebKit Commit Bot]

Attachment 270452 [details] did not pass style-queue:


ERROR: Tools/TestWebKitAPI/Configurations/FeatureDefines.xcconfig:0:  Patch introduces a new FeatureDefines.xcconfig, which check-webkit-style doesn't know about. Please add it to the list in featuredefines.py.  [featuredefines/new] [5]
ERROR: Source/WebCore/platform/network/cocoa/WebCoreNSURLSession.mm:469:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
Total errors found: 2 in 12 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Jer Noble]

Created attachment 270454 [details]
Patch

[WebKit Commit Bot]

Attachment 270454 [details] did not pass style-queue:


ERROR: Tools/TestWebKitAPI/Configurations/FeatureDefines.xcconfig:0:  Patch introduces a new FeatureDefines.xcconfig, which check-webkit-style doesn't know about. Please add it to the list in featuredefines.py.  [featuredefines/new] [5]
ERROR: Source/WebCore/platform/network/cocoa/WebCoreNSURLSession.mm:469:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
Total errors found: 2 in 12 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Jer Noble]

Created attachment 270500 [details]
Patch

[WebKit Commit Bot]

Attachment 270500 [details] did not pass style-queue:


ERROR: Tools/TestWebKitAPI/Configurations/FeatureDefines.xcconfig:0:  Patch introduces a new FeatureDefines.xcconfig, which check-webkit-style doesn't know about. Please add it to the list in featuredefines.py.  [featuredefines/new] [5]
ERROR: Source/WebCore/platform/network/cocoa/WebCoreNSURLSession.mm:479:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
Total errors found: 2 in 12 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Build Bot]

Comment on attachment 270500 [details]
Patch

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

Number of test failures exceeded the failure limit.

[Build Bot]

Created attachment 270504 [details]
Archive of layout-test-results from ews116 for mac-yosemite

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

[Build Bot]

Comment on attachment 270500 [details]
Patch

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

Number of test failures exceeded the failure limit.

[Build Bot]

Created attachment 270505 [details]
Archive of layout-test-results from ews102 for mac-yosemite

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

[Build Bot]

Comment on attachment 270500 [details]
Patch

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

Number of test failures exceeded the failure limit.

[Build Bot]

Created attachment 270506 [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

[Jer Noble]

Created attachment 270509 [details]
Patch

[WebKit Commit Bot]

Attachment 270509 [details] did not pass style-queue:


ERROR: Tools/TestWebKitAPI/Configurations/FeatureDefines.xcconfig:0:  Patch introduces a new FeatureDefines.xcconfig, which check-webkit-style doesn't know about. Please add it to the list in featuredefines.py.  [featuredefines/new] [5]
ERROR: Source/WebCore/platform/network/cocoa/WebCoreNSURLSession.mm:486:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
Total errors found: 2 in 12 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Alex Christensen]

Comment on attachment 270509 [details]
Patch

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

> Source/WebCore/platform/graphics/PlatformMediaResourceLoader.h:59
> +        CacheData = 2 << 0,

This isn't really used in this patch.

> Source/WebCore/platform/network/cocoa/WebCoreNSURLSession.h:56
> +    Vector<RetainPtr<WebCoreNSURLSessionDataTask>> _dataTasks;

I think we should use a HashSet to avoid removing from the middle of a Vector.

> Source/WebCore/platform/network/cocoa/WebCoreNSURLSession.h:107
> +    int64_t _countOfBytesReceived;
> +    int64_t _countOfBytesSent;
> +    int64_t _countOfBytesExpectedToSend;
> +    int64_t _countOfBytesExpectedToReceive;

Do these really need to be signed?  Can they ever be <= 0?

> Source/WebCore/platform/network/cocoa/WebCoreNSURLSession.mm:110
> +    [self.delegateQueue addOperationWithBlock:[strongSelf] {

Sometimes you use _queue directly.  I think this should be consistent.

> Source/WebCore/platform/network/cocoa/WebCoreNSURLSession.mm:421
> +    callOnMainThread([strongSelf] {

Are these and the ASSERT(isMainThread())'s correct when the delegate queue is not the [NSOperationQueue mainQueue]?

> Source/WebCore/platform/network/cocoa/WebCoreNSURLSession.mm:489
> +            if (disposition == NSURLSessionResponseAllow)

else if

Also, what should be done if disposition is NSURLSessionResponseBecomeDownload or NSURLSessionResponseBecomeStream?

> Source/WebCore/platform/network/cocoa/WebCoreNSURLSession.mm:503
> +    // FIXME: try to avoid a copy, if possible.

This had better be possible.  Otherwise, we have done something terribly wrong and it will kill media performance.

> Source/WebCore/platform/spi/mac/AVFoundationSPI.h:78
> +#import <AVFoundation/AVAssetResourceLoader.h>

This isn't used in this patch.

> Tools/TestWebKitAPI/Tests/WebCore/WebCoreNSURLSession.mm:79
> +    didRecieveData = true;

It would be nice if we could check to make sure the data is actually what we expect it to be.

[Jer Noble]

Comment on attachment 270509 [details]
Patch

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

>> Source/WebCore/platform/graphics/PlatformMediaResourceLoader.h:59
>> +        CacheData = 2 << 0,
> 
> This isn't really used in this patch.

It's for a follow up patch; I'll remove it.

>> Source/WebCore/platform/network/cocoa/WebCoreNSURLSession.h:56
>> +    Vector<RetainPtr<WebCoreNSURLSessionDataTask>> _dataTasks;
> 
> I think we should use a HashSet to avoid removing from the middle of a Vector.

OK.

>> Source/WebCore/platform/network/cocoa/WebCoreNSURLSession.h:107
>> +    int64_t _countOfBytesExpectedToReceive;
> 
> Do these really need to be signed?  Can they ever be <= 0?

The protocol I'm re-implementing requires these to be int64_t.  We can implement the ivars as uint64_t and have the properties be uint64_t, but we could no longer use @synthesize, and it would require potentially confusing casts everywhere.

>> Source/WebCore/platform/network/cocoa/WebCoreNSURLSession.mm:110
>> +    [self.delegateQueue addOperationWithBlock:[strongSelf] {
> 
> Sometimes you use _queue directly.  I think this should be consistent.

Ok.

>> Source/WebCore/platform/network/cocoa/WebCoreNSURLSession.mm:421
>> +    callOnMainThread([strongSelf] {
> 
> Are these and the ASSERT(isMainThread())'s correct when the delegate queue is not the [NSOperationQueue mainQueue]?

Yes.

>> Source/WebCore/platform/network/cocoa/WebCoreNSURLSession.mm:489
>> +            if (disposition == NSURLSessionResponseAllow)
> 
> else if
> 
> Also, what should be done if disposition is NSURLSessionResponseBecomeDownload or NSURLSessionResponseBecomeStream?

Since we don't support Download tasks or Stream tasks, we don't handle those dispositions.  I suppose we could add an ASSERT_NOT_REACHED() to catch if any of our clients start requesting those tasks.

>> Source/WebCore/platform/network/cocoa/WebCoreNSURLSession.mm:503
>> +    // FIXME: try to avoid a copy, if possible.
> 
> This had better be possible.  Otherwise, we have done something terribly wrong and it will kill media performance.

It's not currently possible to get a reference-counted handle on the memory.  That's an improvement we have to look into, hence the FIXME.

>> Source/WebCore/platform/spi/mac/AVFoundationSPI.h:78
>> +#import <AVFoundation/AVAssetResourceLoader.h>
> 
> This isn't used in this patch.

I'll remove it.

>> Tools/TestWebKitAPI/Tests/WebCore/WebCoreNSURLSession.mm:79
>> +    didRecieveData = true;
> 
> It would be nice if we could check to make sure the data is actually what we expect it to be.

I'll add some logic to assert that the data returned matches the data in the file.

[Jer Noble]

Comment on attachment 270509 [details]
Patch

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

>> Source/WebCore/platform/graphics/PlatformMediaResourceLoader.h:59
>> +        CacheData = 2 << 0,
> 
> This isn't really used in this patch.

It's for a follow up patch; I'll remove it.

>> Source/WebCore/platform/network/cocoa/WebCoreNSURLSession.h:56
>> +    Vector<RetainPtr<WebCoreNSURLSessionDataTask>> _dataTasks;
> 
> I think we should use a HashSet to avoid removing from the middle of a Vector.

OK.

>> Source/WebCore/platform/network/cocoa/WebCoreNSURLSession.h:107
>> +    int64_t _countOfBytesExpectedToReceive;
> 
> Do these really need to be signed?  Can they ever be <= 0?

The protocol I'm re-implementing requires these to be int64_t.  We can implement the ivars as uint64_t and have the properties be uint64_t, but we could no longer use @synthesize, and it would require potentially confusing casts everywhere.

>> Source/WebCore/platform/network/cocoa/WebCoreNSURLSession.mm:110
>> +    [self.delegateQueue addOperationWithBlock:[strongSelf] {
> 
> Sometimes you use _queue directly.  I think this should be consistent.

Ok.

>> Source/WebCore/platform/network/cocoa/WebCoreNSURLSession.mm:421
>> +    callOnMainThread([strongSelf] {
> 
> Are these and the ASSERT(isMainThread())'s correct when the delegate queue is not the [NSOperationQueue mainQueue]?

Yes.

>> Source/WebCore/platform/network/cocoa/WebCoreNSURLSession.mm:489
>> +            if (disposition == NSURLSessionResponseAllow)
> 
> else if
> 
> Also, what should be done if disposition is NSURLSessionResponseBecomeDownload or NSURLSessionResponseBecomeStream?

Since we don't support Download tasks or Stream tasks, we don't handle those dispositions.  I suppose we could add an ASSERT_NOT_REACHED() to catch if any of our clients start requesting those tasks.

>> Source/WebCore/platform/network/cocoa/WebCoreNSURLSession.mm:503
>> +    // FIXME: try to avoid a copy, if possible.
> 
> This had better be possible.  Otherwise, we have done something terribly wrong and it will kill media performance.

It's not currently possible to get a reference-counted handle on the memory.  That's an improvement we have to look into, hence the FIXME.

>> Source/WebCore/platform/spi/mac/AVFoundationSPI.h:78
>> +#import <AVFoundation/AVAssetResourceLoader.h>
> 
> This isn't used in this patch.

I'll remove it.

>> Tools/TestWebKitAPI/Tests/WebCore/WebCoreNSURLSession.mm:79
>> +    didRecieveData = true;
> 
> It would be nice if we could check to make sure the data is actually what we expect it to be.

I'll add some logic to assert that the data returned matches the data in the file.

[Jer Noble]

Created attachment 270589 [details]
Patch

[WebKit Commit Bot]

Attachment 270589 [details] did not pass style-queue:


ERROR: Tools/TestWebKitAPI/Configurations/FeatureDefines.xcconfig:0:  Patch introduces a new FeatureDefines.xcconfig, which check-webkit-style doesn't know about. Please add it to the list in featuredefines.py.  [featuredefines/new] [5]
ERROR: Source/WebCore/platform/network/cocoa/WebCoreNSURLSession.mm:486:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
Total errors found: 2 in 9 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Alex Christensen]

Comment on attachment 270589 [details]
Patch

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

> Source/WebCore/platform/network/cocoa/WebCoreNSURLSession.mm:40
> +SOFT_LINK_CONSTANT_MAY_FAIL(Foundation, NSURLSessionTaskPriorityDefault, float)

We should put a comment saying this is soft linked to link successfully in Yosemite so we can remember to clean up once we drop support for Yosemite.

> Source/WebCore/platform/network/cocoa/WebCoreNSURLSession.mm:466
> +    // No-op.

Shouldn't we call didSendBodyData on the delegate here?

> Tools/TestWebKitAPI/Tests/WebCore/WebCoreNSURLSession.mm:128
> +    [session finishTasksAndInvalidate];

This does not test finishTasksAndInvalidate with _dataTasks not empty, which uses a different code path.  Please add such a test.

[Jer Noble]

(In reply to comment #30)
> Comment on attachment 270589 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=270589&action=review
> 
> > Source/WebCore/platform/network/cocoa/WebCoreNSURLSession.mm:40
> > +SOFT_LINK_CONSTANT_MAY_FAIL(Foundation, NSURLSessionTaskPriorityDefault, float)
> 
> We should put a comment saying this is soft linked to link successfully in
> Yosemite so we can remember to clean up once we drop support for Yosemite.

Ok.

> > Source/WebCore/platform/network/cocoa/WebCoreNSURLSession.mm:466
> > +    // No-op.
> 
> Shouldn't we call didSendBodyData on the delegate here?

No, we are not a Upload task.

> > Tools/TestWebKitAPI/Tests/WebCore/WebCoreNSURLSession.mm:128
> > +    [session finishTasksAndInvalidate];
> 
> This does not test finishTasksAndInvalidate with _dataTasks not empty, which
> uses a different code path.  Please add such a test.

Ok.

[Jer Noble]

Created attachment 270596 [details]
Patch for landing

[WebKit Commit Bot]

Attachment 270596 [details] did not pass style-queue:


ERROR: Tools/TestWebKitAPI/Configurations/FeatureDefines.xcconfig:0:  Patch introduces a new FeatureDefines.xcconfig, which check-webkit-style doesn't know about. Please add it to the list in featuredefines.py.  [featuredefines/new] [5]
ERROR: Source/WebCore/platform/network/cocoa/WebCoreNSURLSession.mm:487:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
Total errors found: 2 in 9 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Alex Christensen]

Comment on attachment 270596 [details]
Patch for landing

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

> Tools/TestWebKitAPI/Tests/WebCore/WebCoreNSURLSession.mm:151
> +TEST_F(WebCoreNSURLSessionTest, InvalidateEmpty)

I don't think this tests what I was thinking.  I think we need a test that calls dataTaskWithURL on the session and calls finishTasksAndInvalidate without waiting for the task to complete first.

[WebKit Commit Bot]

Comment on attachment 270596 [details]
Patch for landing

Clearing flags on attachment: 270596

Committed r196082: <http://trac.webkit.org/changeset/196082>

[Alex Christensen]

Comment on attachment 270596 [details]
Patch for landing

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

> Source/WebCore/platform/network/cocoa/WebCoreNSURLSession.mm:206
> +    WebCoreNSURLSessionDataTask *task = [[WebCoreNSURLSessionDataTask alloc] initWithSession:self identifier:_nextTaskIdentifier++ request:request];

Is the first task identifier 0?  I think the first task identifier of a real NSURLSession is 1, and that might cause some problems.  It might be worth using the prefix ++ instead.

[Jer Noble]

(In reply to comment #34)
> Comment on attachment 270596 [details]
> Patch for landing
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=270596&action=review
> 
> > Tools/TestWebKitAPI/Tests/WebCore/WebCoreNSURLSession.mm:151
> > +TEST_F(WebCoreNSURLSessionTest, InvalidateEmpty)
> 
> I don't think this tests what I was thinking.  I think we need a test that
> calls dataTaskWithURL on the session and calls finishTasksAndInvalidate
> without waiting for the task to complete first.

We have one of those, in BasicOperation:

+    didInvalidate = false;
+
+    task = [session dataTaskWithURL:resourceURL];
+    [task resume];
+    [session finishTasksAndInvalidate];
+
+    TestWebKitAPI::Util::run(&didInvalidate);

[Radar WebKit Bug Importer]

<rdar://problem/24588054>