| Summary: | REGRESSION (r17682): Assertion failure in IconLoader::didFinishLoading() when the icon request returns 404 or 403 | ||||||||
|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | mitz | ||||||
| Component: | Page Loading | Assignee: | mitz | ||||||
| Status: | CLOSED FIXED | ||||||||
| Severity: | Normal | CC: | beidson | ||||||
| Priority: | P1 | Keywords: | Regression | ||||||
| Version: | 420+ | ||||||||
| Hardware: | Mac | ||||||||
| OS: | OS X 10.4 | ||||||||
| URL: | data:text/html,<link%20rel="shortcut%20icon"%20href="http://www.apple.com/filenotfound"> | ||||||||
| Attachments: |
|
||||||||
|
Description
mitz
2006-11-09 08:56:14 PST
I'll look into this soon... definitely a regression from before some work Darin and I did a couple of days ago 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 Maciej's addition of didFailWithError changed what we should be checking in this method - very small change, at that. Patch is coming Created attachment 11449 [details]
Proposed fix
Maciej's didFailWithError addition brought this one about - simple fix.
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. (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. 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... Created attachment 11456 [details]
This is a more complete fix to this bug, and another bug too!
Comment on attachment 11456 [details]
This is a more complete fix to this bug, and another bug too!
r=me
Committed in r17702, please verify |