Bug 22191 – logic error in CachedImage.cpp

• Status: RESOLVED
• Resolution: INVALID
• Component: WebCore Misc.

[Ojan Vafai]

I don't have a testcase for this other than to say that is causes a crash in the Chromium build when loading an image resource fails, but the logic seems obviously wrong.

error() calls clear(), which calls destroyDecodedData(). destoryDecodedData() checks m_errorOccurred, which is set to true *after* the clear() call in error(). Seems like those two lines just need to be swapped.

void CachedImage::error()
{
    clear();
    m_errorOccurred = true;
    notifyObservers();
    m_loading = false;
    checkNotify();
}

void CachedImage::clear()
{
    destroyDecodedData();
    m_image = 0;
    setEncodedSize(0);
}

void CachedImage::destroyDecodedData()
{
    if (m_image && !m_errorOccurred)
        m_image->destroyDecodedData();
}

[Ojan Vafai]

Created attachment 25081 [details]
Swaps lines.

[Darin Adler]

Comment on attachment 25081 [details]
Swaps lines.

You say that the mistake in this function is obvious. But it's not obvious to me. It seems to me that the clear() function needs to be called before m_errorOccurred, because we want it to actually discard any decoded data in the partially created image.

In fact, I'm not sure why the check for m_errorOccurred in the destroyDecodedData is helpful. It seems that we do want to destroy the decoded data even if an error occurred.

You say that this causes a crash in Chromium, but you don't supply any details.

I'm not convinced this is the correct fix.

> +	Fixes logic of error() to first set
> +	m_errorOccurred, then call clear(),
> +	which depends on m_errorOccurred having the
> +	correct value.

Please don't use tabs in ChangeLog -- our repository is configured so it doesn't allow tabs.

I think it's likely that if this is really a bug there's some way to reproduce a crash in ports other than Chromium and therefore we can create a regression test.

If you remain convinced that this patch is 1) correct and 2) not testable in other ports, then feel free to submit the patch again, but I don't think it's obvious that this is a correct change.

[Ojan Vafai]

Now I can't reproduce the crash. Not sure what changed, but no need to keep the bug open.