|Summary:||REGRESSION (r17682): Assertion failure in IconLoader::didFinishLoading() when the icon request returns 404 or 403|
|OS:||OS X 10.4|
Description mitz 2006-11-09 08:56:14 PST
The assertion in IconLoader::didFinishLoading() is hit when the icon URL request returns a result code outside the 200-299 range. The assertion fails because in this case, finishLoading() is called first from didReceiveResponse().
Comment 1 Brady Eidson 2006-11-09 09:40:38 PST
I'll look into this soon... definitely a regression from before some work Darin and I did a couple of days ago
Comment 2 Brady Eidson 2006-11-09 16:12:51 PST
I thought the problem here was due to refactoring Darin and I did earlier causing a preexisting ASSERT to be hit - I see now what maciej changed in 17682. Looking at it now
Comment 3 Brady Eidson 2006-11-09 17:30:40 PST
Maciej's addition of didFailWithError changed what we should be checking in this method - very small change, at that. Patch is coming
Comment 4 Brady Eidson 2006-11-09 17:35:31 PST
Created attachment 11449 [details] Proposed fix Maciej's didFailWithError addition brought this one about - simple fix.
Comment 5 Brady Eidson 2006-11-09 20:27:51 PST
Discussed this with Maciej on IRC, I need to explore a little more - he claims didFailWithError should be terminal for a ResourceHandler, and didFinishLoading should not be called after.
Comment 6 mitz 2006-11-09 23:12:35 PST
(In reply to comment #5) > Discussed this with Maciej on IRC, I need to explore a little more - he claims > didFailWithError should be terminal for a ResourceHandler, and didFinishLoading > should not be called after. > To clarify my initial description, didFailWithError is never called in this case, only didReceiveResponse is. I think perhaps the way to deal with this situation is to change the behavior of didReceiveResponse for statuses outside the 200-299 range, so that it will only set an "invalid icon data" flag, and later in finishLoading to examine this flag, and if it is set, skip the icon processing and go straight to clearLoadingState. The flag could also be used to save the work of copying of data in didReceiveData.
Comment 7 Brady Eidson 2006-11-09 23:57:58 PST
Yah, I totally jumped to an assumption when I was debugging and saw m_handle == 0 - I assumed it could only happen in two places, but can really happen in 3. Addressing your suggestion - Darin and I actually went through great lengths to remove as moving flags and vars as possible - it would be a shame to put one back in. With this greater, complete understand of what's happening, I think my original patch becomes valid again, in a slightly expanded form. Stay tuned...
Comment 8 Brady Eidson 2006-11-10 00:37:53 PST
Created attachment 11456 [details] This is a more complete fix to this bug, and another bug too!
Comment 9 mitz 2006-11-10 00:47:07 PST
Comment on attachment 11456 [details] This is a more complete fix to this bug, and another bug too! r=me