Bug 155137 - Always call NSURLSession completion handlers
Summary: Always call NSURLSession completion handlers
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit2 (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Alex Christensen
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2016-03-07 14:41 PST by Alex Christensen
Modified: 2016-03-10 17:15 PST (History)
2 users (show)

See Also:


Attachments
Patch (5.56 KB, patch)
2016-03-07 15:01 PST, Alex Christensen
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Alex Christensen 2016-03-07 14:41:11 PST
Always call NSURLSession completion handlers
Comment 1 Alex Christensen 2016-03-07 15:01:34 PST
Created attachment 273219 [details]
Patch
Comment 2 Brent Fulgham 2016-03-08 08:51:26 PST
Comment on attachment 273219 [details]
Patch

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

This seems fine. r=me.

> Source/WebKit2/ChangeLog:10
> +        There are also a few release asserts that do not need to crash release builds.

So is this patch just about protecting against a possible problem we could encounter?
Comment 3 Brent Fulgham 2016-03-08 08:52:06 PST
(In reply to comment #2)
> Comment on attachment 273219 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=273219&action=review
> 
> This seems fine. r=me.

I mean, NOT r=me. You need a WK2 owner. Sorry! (But it seems fine).
Comment 4 Alex Christensen 2016-03-08 10:18:07 PST
Comment on attachment 273219 [details]
Patch

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

>> Source/WebKit2/ChangeLog:10
>> +        There are also a few release asserts that do not need to crash release builds.
> 
> So is this patch just about protecting against a possible problem we could encounter?

yep
Comment 5 Darin Adler 2016-03-08 18:08:59 PST
Comment on attachment 273219 [details]
Patch

I do not understand the status of these ASSERT_NOT_REACHED. Will we ever reach these? If so, why?
Comment 6 Alex Christensen 2016-03-09 10:17:26 PST
(In reply to comment #5)
> Comment on attachment 273219 [details]
> Patch
> 
> I do not understand the status of these ASSERT_NOT_REACHED. Will we ever
> reach these? If so, why?
We should not reach any of these assertions because once we are doing loading, a session with the given SessionID has already been made in the SessionTracker.  If something goes horribly wrong and we are, for example, finishing loading after we have destroyed a session, then these do not need to crash release builds because the only effect might be that we do some loads without credentials.
Comment 7 WebKit Commit Bot 2016-03-09 11:03:56 PST
Comment on attachment 273219 [details]
Patch

Clearing flags on attachment: 273219

Committed r197865: <http://trac.webkit.org/changeset/197865>
Comment 8 WebKit Commit Bot 2016-03-09 11:03:59 PST
All reviewed patches have been landed.  Closing bug.
Comment 9 Alex Christensen 2016-03-10 15:54:56 PST
removed an assertion in r197965
Comment 10 Alex Christensen 2016-03-10 17:15:21 PST
When a load is redirected to a url that we do not like, we call the willPerformHTTPRedirection completion handler with nil in NetworkLoad::continueWillSendRequest just after the NetworkLoad and NetworkDataTask are destroyed by the call to didFail.  When this happens, we get a didReceiveResponse delegate callback for the url that was redirected from, which no longer corresponds to a NetworkDataTask that exists any more in case the 301 response had content that we wanted, but in WebKit we never use that content, so we are correctly calling the completion handler with NSURLSessionResponseCancel.

This happens in several tests, including http/tests/xmlhttprequest/xmlhttprequest-unsafe-redirect.html