Bug 136259 – Prevent decoded images from being destroyed when they're in use.

• Component: WebCore Misc.
• Status: RESOLVED FIXED

[Daewoong Jang]

CachedImage shouldn't destroy its decoded image while it still has clients.

[Daewoong Jang]

Created attachment 237157 [details]
patch

[Darin Adler]

Comment on attachment 237157 [details]
patch

Bug fixes need to come with test cases. Please construct a test case that demonstrates the problem this is fixing.

[Daewoong Jang]

Darin,

The problem I try to fix here is a regression caused by patch r172790(https://bugs.webkit.org/show_bug.cgi?id=135939). Before the patch, CachedImage does not release its Image object if it still has clients and this behavior is done by a check to !hasClients(), which had been called by deleted isSafeToMakePurgeable().

The problem can be seen on WinCairo build with cURL enabled. And I'm afraid test cases can't be provided since original patch has no new tests because no functional changes.

Could you please see r172790 I mentioned above? I think it would be nice if Psolanski could take a look.

Thank you.

[Pratik Solanki]

isSafeToMakePurgeable() used to do

    return !hasClients() && !m_proxyResource && !m_resourceToRevalidate;

Should this patch be checking for the other two conditions as well?

[Darin Adler]

Please mention that you are fixing a regression in the change log and how you are restoring the old behavior; if you had done that, I might have just done a review+.

I think it might be OK to not add a regression test if you don’t know how to make one, but we really do want one if we can possibly do it. The whole point of the tests is to prevent this from happening again.

[Darin Adler]

(In reply to comment #3)
> And I'm afraid test cases can't be provided since original patch has no new tests because no functional changes.

That comment doesn’t make logical sense. If we can’t provide a test case, there must be some other reason. The presence or absence of test cases in the original patch that was meant to be a refactoring/removal of feature only tells us nothing about whether a test case can be created.

[Daewoong Jang]

Pratnik,

I don't think checking of other two conditions is needed since a proxy resource is not likely to hold an image object. How do you think?

Darin,

All right, I get what you meant. The thing I tried to tell you was it seemed to me that this bug fix doesn't need a test case since the original patch didn't need one.

>> The whole point of the tests is to prevent this from happening again.

I totally agree to what you've said. However I don't know how a test case can be provided for this kind of bug.

[Daewoong Jang]

And I will not forget to mention about the details next time. Thanks Darin.

[Pratik Solanki]

Patch seems reasonable to me. But can you add more information in the ChangeLog? Please reference the commit/bug that caused this issue.

[Daewoong Jang]

(In reply to comment #9)
> Patch seems reasonable to me. But can you add more information in the ChangeLog? Please reference the commit/bug that caused this issue.

OK. I'll update patch with the details. Thanks Pratik.

[Daewoong Jang]

Created attachment 237320 [details]
updated patch

[Darin Adler]

Comment on attachment 237320 [details]
updated patch

We should not give up on trying to cover this with tests. The reason we have the “add a test whenever we fix a bug” policy is so we don’t keep breaking the same things over and over again. I understand that it’s much harder to make a test here than it is to simply undo the regression we caused. But the discussion we’ve had so far about why it’s hard to make a test have not been satisfactory -- it’s simply logically incorrect to say that because Pratik didn’t add a test when he refactored and broke this by mistake that we don’t need a test when we fix it. I know it’s frustrating, but when we make a mistake like that it is our policy to add coverage after the fact so we don’t make the same mistake again. So it’s not the person who made the mistake, but the rest of us after the fact who have to try to figure out how to add test coverage.

I’m going to reluctantly say review+ here but I would like someone to make a test.

[Daewoong Jang]

(In reply to comment #12)
> (From update of attachment 237320 [details])
> We should not give up on trying to cover this with tests. The reason we have the “add a test whenever we fix a bug” policy is so we don’t keep breaking the same things over and over again. I understand that it’s much harder to make a test here than it is to simply undo the regression we caused. But the discussion we’ve had so far about why it’s hard to make a test have not been satisfactory -- it’s simply logically incorrect to say that because Pratik didn’t add a test when he refactored and broke this by mistake that we don’t need a test when we fix it. I know it’s frustrating, but when we make a mistake like that it is our policy to add coverage after the fact so we don’t make the same mistake again. So it’s not the person who made the mistake, but the rest of us after the fact who have to try to figure out how to add test coverage.
> 
> I’m going to reluctantly say review+ here but I would like someone to make a test.

Darin,

Thanks for the review and I'll try to add test for this bug soon. As you said, to prevent this from happening again is important.

[Daewoong Jang]

Created attachment 237374 [details]
test case

[Daewoong Jang]

I've done the test case, but where should I put this? Please take a look and give me some advice.

[Brian Burg]

(In reply to comment #15)
> I've done the test case, but where should I put this? Please take a look and give me some advice.

I would put it in LayoutTests/loader/.

[Daewoong Jang]

(In reply to comment #16)
> (In reply to comment #15)
> > I've done the test case, but where should I put this? Please take a look and give me some advice.
> 
> I would put it in LayoutTests/loader/.

I've already uploaded: https://bugs.webkit.org/show_bug.cgi?id=136417. If you could, please check this out.

I'll move the test case to LayoutTests/loader. Thanks!

[WebKit Commit Bot]

Comment on attachment 237320 [details]
updated patch

Clearing flags on attachment: 237320

Committed r173172: <http://trac.webkit.org/changeset/173172>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.