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.
m_errorOccurred = true;
m_loading = false;
m_image = 0;
if (m_image && !m_errorOccurred)
Created attachment 25081 [details]
Comment on attachment 25081 [details]
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.
Now I can't reproduce the crash. Not sure what changed, but no need to keep the bug open.