Bug 153669 - [Mac] Wrap a resource and resource loader in a NSURLSession-like object for use by lower level frameworks
Summary: [Mac] Wrap a resource and resource loader in a NSURLSession-like object for u...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Jer Noble
URL:
Keywords: InRadar
Depends on:
Blocks: 189742
  Show dependency treegraph
 
Reported: 2016-01-29 13:34 PST by Jer Noble
Modified: 2018-09-19 03:14 PDT (History)
7 users (show)

See Also:


Attachments
Patch (57.86 KB, patch)
2016-01-29 14:56 PST, Jer Noble
no flags Details | Formatted Diff | Diff
Patch (61.43 KB, patch)
2016-02-01 13:58 PST, Jer Noble
no flags Details | Formatted Diff | Diff
Patch (61.58 KB, patch)
2016-02-01 14:37 PST, Jer Noble
no flags Details | Formatted Diff | Diff
Patch (62.66 KB, patch)
2016-02-01 15:19 PST, Jer Noble
no flags Details | Formatted Diff | Diff
Patch (62.79 KB, patch)
2016-02-01 16:12 PST, Jer Noble
no flags Details | Formatted Diff | Diff
Patch (62.79 KB, patch)
2016-02-01 16:39 PST, Jer Noble
no flags Details | Formatted Diff | Diff
Patch (62.80 KB, patch)
2016-02-01 16:48 PST, Jer Noble
no flags Details | Formatted Diff | Diff
Patch (63.00 KB, patch)
2016-02-02 11:34 PST, Jer Noble
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews116 for mac-yosemite (264.93 KB, application/zip)
2016-02-02 12:09 PST, Build Bot
no flags Details
Archive of layout-test-results from ews102 for mac-yosemite (335.49 KB, application/zip)
2016-02-02 12:10 PST, Build Bot
no flags Details
Archive of layout-test-results from ews106 for mac-yosemite-wk2 (371.07 KB, application/zip)
2016-02-02 12:12 PST, Build Bot
no flags Details
Patch (63.35 KB, patch)
2016-02-02 13:05 PST, Jer Noble
no flags Details | Formatted Diff | Diff
Patch (60.19 KB, patch)
2016-02-03 11:05 PST, Jer Noble
achristensen: review+
Details | Formatted Diff | Diff
Patch for landing (61.14 KB, patch)
2016-02-03 13:00 PST, 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 Jer Noble 2016-01-29 13:34:37 PST
[Mac] Wrap a resource and resource loader in a NSURLSession-like object for use by lower level frameworks
Comment 1 Jer Noble 2016-01-29 14:56:30 PST
Created attachment 270257 [details]
Patch
Comment 2 WebKit Commit Bot 2016-01-29 14:57:16 PST
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.
Comment 3 Jer Noble 2016-02-01 13:58:37 PST
Created attachment 270423 [details]
Patch
Comment 4 WebKit Commit Bot 2016-02-01 14:01:07 PST
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.
Comment 5 Jer Noble 2016-02-01 14:37:32 PST
Created attachment 270427 [details]
Patch
Comment 6 WebKit Commit Bot 2016-02-01 14:39:09 PST
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.
Comment 7 Jer Noble 2016-02-01 15:19:43 PST
Created attachment 270434 [details]
Patch
Comment 8 WebKit Commit Bot 2016-02-01 15:20:28 PST
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.
Comment 9 Jer Noble 2016-02-01 16:12:29 PST
Created attachment 270447 [details]
Patch
Comment 10 WebKit Commit Bot 2016-02-01 16:13:28 PST
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.
Comment 11 Jer Noble 2016-02-01 16:39:29 PST
Created attachment 270452 [details]
Patch
Comment 12 WebKit Commit Bot 2016-02-01 16:40:57 PST
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.
Comment 13 Jer Noble 2016-02-01 16:48:26 PST
Created attachment 270454 [details]
Patch
Comment 14 WebKit Commit Bot 2016-02-01 16:50:04 PST
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.
Comment 15 Jer Noble 2016-02-02 11:34:22 PST
Created attachment 270500 [details]
Patch
Comment 16 WebKit Commit Bot 2016-02-02 11:37:11 PST
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.
Comment 17 Build Bot 2016-02-02 12:08:57 PST
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.
Comment 18 Build Bot 2016-02-02 12:09:00 PST
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
Comment 19 Build Bot 2016-02-02 12:10:38 PST
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.
Comment 20 Build Bot 2016-02-02 12:10:41 PST
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
Comment 21 Build Bot 2016-02-02 12:12:23 PST
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.
Comment 22 Build Bot 2016-02-02 12:12:26 PST
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
Comment 23 Jer Noble 2016-02-02 13:05:22 PST
Created attachment 270509 [details]
Patch
Comment 24 WebKit Commit Bot 2016-02-02 13:06:37 PST
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.
Comment 25 Alex Christensen 2016-02-02 15:00:42 PST
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.
Comment 26 Jer Noble 2016-02-03 09:48:22 PST
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.
Comment 27 Jer Noble 2016-02-03 09:48:25 PST
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.
Comment 28 Jer Noble 2016-02-03 11:05:39 PST
Created attachment 270589 [details]
Patch
Comment 29 WebKit Commit Bot 2016-02-03 11:08:18 PST
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.
Comment 30 Alex Christensen 2016-02-03 12:01:44 PST
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.
Comment 31 Jer Noble 2016-02-03 13:00:05 PST
(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.
Comment 32 Jer Noble 2016-02-03 13:00:30 PST
Created attachment 270596 [details]
Patch for landing
Comment 33 WebKit Commit Bot 2016-02-03 13:26:32 PST
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.
Comment 34 Alex Christensen 2016-02-03 14:50:31 PST
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.
Comment 35 WebKit Commit Bot 2016-02-03 14:57:50 PST
Comment on attachment 270596 [details]
Patch for landing

Clearing flags on attachment: 270596

Committed r196082: <http://trac.webkit.org/changeset/196082>
Comment 36 Alex Christensen 2016-02-03 16:49:27 PST
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.
Comment 37 Jer Noble 2016-02-04 13:36:32 PST
(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);
Comment 38 Radar WebKit Bug Importer 2016-02-10 08:50:12 PST
<rdar://problem/24588054>