Bug 155137 – Always call NSURLSession completion handlers

• Component: WebKit2
• Status: RESOLVED FIXED

[Alex Christensen]

Always call NSURLSession completion handlers

[Alex Christensen]

Created attachment 273219 [details]
Patch

[Brent Fulgham]

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?

[Brent Fulgham]

(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).

[Alex Christensen]

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

[Darin Adler]

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?

[Alex Christensen]

(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.

[WebKit Commit Bot]

Comment on attachment 273219 [details]
Patch

Clearing flags on attachment: 273219

Committed r197865: <http://trac.webkit.org/changeset/197865>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Alex Christensen]

removed an assertion in r197965

[Alex Christensen]

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