Bug 22191 - logic error in CachedImage.cpp
Summary: logic error in CachedImage.cpp
Status: RESOLVED INVALID
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC OS X 10.5
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2008-11-11 17:07 PST by Ojan Vafai
Modified: 2008-12-01 13:12 PST (History)
0 users

See Also:


Attachments
Swaps lines. (1000 bytes, patch)
2008-11-11 17:33 PST, Ojan Vafai
darin: review-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Ojan Vafai 2008-11-11 17:07:35 PST
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();
}
Comment 1 Ojan Vafai 2008-11-11 17:33:31 PST
Created attachment 25081 [details]
Swaps lines.
Comment 2 Darin Adler 2008-11-12 08:59:11 PST
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.
Comment 3 Ojan Vafai 2008-12-01 13:12:10 PST
Now I can't reproduce the crash. Not sure what changed, but no need to keep the bug open.