Bug 11554 – REGRESSION (r17682): Assertion failure in IconLoader::didFinishLoading() when the icon request returns 404 or 403

• Status: CLOSED
• Resolution: FIXED
• Component: Page Loading

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

[Brady Eidson]

I'll look into this soon...  definitely a regression from before some work Darin and I did a couple of days ago

[Brady Eidson]

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

[Brady Eidson]

Maciej's addition of didFailWithError changed what we should be checking in this method - very small change, at that.  Patch is coming

[Brady Eidson]

Created attachment 11449 [details]
Proposed fix

Maciej's didFailWithError addition brought this one about - simple fix.

[Brady Eidson]

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.

[Brady Eidson]

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

[Brady Eidson]

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

[Brady Eidson]

Committed in r17702, please verify