Invalid URL causes crash #744

Closed
patr1ck opened this Issue May 15, 2012 · 11 comments

Projects

None yet

4 participants

@patr1ck
patr1ck commented May 15, 2012

Similar to #628 but not quite the same.

The problem here, which is not caught by the testThatLoadingInvalidURLDoesNotCrashApp test in RKResponseTest, is that in my app code, I am releasing the request object in the request delegate's request:(RKRequest *)request didFailLoadWithError:(NSError *)error method. This seems like the most appropriate place to put that code, but I could be wrong.

Anyways, when I do release the request there, the request goes off and tries to send a few notifications in it's didFailLoadWithError:(NSError*)error method. The last of which fails, since it's expecting to still have a retain count of 1 at that point, when in fact it is zero.

The fix mentioned by @echamberlain in #628 works here also, but I thought I'd open this issue for discussion, since I'm not positive that's the appropriate fix.

@k1th
k1th commented May 15, 2012

Blake's fix didn't work for me either (1f98cf4)
I still crash in
- (void)connection:(NSURLConnection *)connection didFailWithError:(NSError *)error;

I am using @echamberlain's workaround for now, but added retain/release around the other invalidates in


- (void)connection:(NSURLConnection *)connection didReceiveData:(NSData *)data;

- (void)connection:(NSURLConnection *)connection didSendBodyData:(NSInteger)bytesWritten totalBytesWritten:(NSInteger)totalBytesWritten totalBytesExpectedToWrite:(NSInteger)totalBytesExpectedToWrite;

@blakewatters
Member

Acknowledged. Will get a fix in.

@k1th
k1th commented May 16, 2012

Regarding the other issue in #628 I did some digging and think you should go back to removeLoadingRequest intead of removeRequest.

in RKResponse:
- (void)connection:(NSURLConnection *)connection didFailWithError:(NSError *)error;

calls didFailLoadWithError on the Request. Current retain count is 2

in RKObjectLoader = the Request:

- (void)didFailLoadWithError:(NSError *)error;

the notification is posted:
...postNotificationName:RKRequestDidFailWithErrorNotification...

The notification is then processed immediately in RKRequestQueue:
- (void)processRequestDidFailWithErrorNotification:(NSNotification *)notification;

retain count is 3 (the notification has an additional retain) at the beginning.

With the change in 1f98cf4:
[self removeRequest:request] is called and itself then does:

[self removeLoadingRequest:request] - retain count 2
then
`[_requests removeObject:request]`` - retain count 1.

now `processRequestDidFailWithErrorNotification``goes out of scope and the retain from the notification is gone as well.

While still in
- (void)didFailLoadWithError:(NSError *)error;

[self informDelegateOfError:error];
will then crash because self is no more – as in this case the RKObjectLoader is the request.

I hope I didn't miss something here :)

@blakewatters
Member

@k1th Thanks for the analysis here -- very helpful. Will re-evaluate this today and try to find a better path.

@sprynmr
Contributor
sprynmr commented May 16, 2012

Just noticed this as well. Thanks for the work guys.

@patr1ck
patr1ck commented May 16, 2012

I forgot to add that the reason testThatLoadingInvalidURLDoesNotCrashApp doesn't catch this is because on certain machines / connections, OpenDNS (or in some cases, the ISP) will catch the invalid URL request and forward it to a server that will respond, usually with a domain search page or something similar.

Using http://localhost:<random closed port>/ would be a better solution.

I mentioned this to Blake on IRC so he knows, but it's worth documenting here too.

@blakewatters
Member

@patr1ck I updated this test but it still passes without any issue. Drilling in further.

@blakewatters blakewatters added a commit that closed this issue May 17, 2012
@blakewatters blakewatters Fixes to ensure failed requests are not resent. refs #628, fixes #744,…
… closes #720

Additional changes and test coverage to handle the case of loading an invalid URL
or otherwise immediately encountering an error condition:

* Restores use of removeLoadingRequest: within the queue
* Updates fragile tests to ensure better coverage for error cases
* Sets isLoaded to YES during error callbacks to prevent duplicated dispatching
938304d
@blakewatters
Member

@patr1ck @k1th Can you guys give the latest development bits a whirl to see if my latest attempts squashes the bug you are seeing?

@patr1ck
patr1ck commented May 17, 2012

@blakewatters Things look good now. Thank you!

@k1th
k1th commented May 22, 2012

looks good here, too.
What was the relevant change, you did? Obviously, the added retain/release we did in the meantime was a little bit too clunky :)

@blakewatters
Member

The underlying issue had to do with the behavior of the isLoaded flag. Previously on error isLoading would flip to NO, but isLoaded would also remain as NO. This would cause isUnsent to evaluate to YES and the request to be resent, but the error callback will have decremented the retain count. So now we consider requests that failed with an error to be loaded, which cleared up the mess. Arguably considering a failed request loaded may be incorrect and it may be more appropriate to add an isError flag and incorporate that state into the isUnsent logic.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment