Bug 136259

Summary: Prevent decoded images from being destroyed when they're in use.
Product: WebKit Reporter: Daewoong Jang <daewoong.jang>
Component: WebCore Misc.Assignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: burg, commit-queue, darin, dbates, ggaren, japhet, kling, msaboff, psolanki, simon.fraser, svillar
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
patch
none
updated patch
none
test case none

Description Daewoong Jang 2014-08-26 10:48:57 PDT
CachedImage shouldn't destroy its decoded image while it still has clients.
Comment 1 Daewoong Jang 2014-08-26 10:55:09 PDT
Created attachment 237157 [details]
patch
Comment 2 Darin Adler 2014-08-27 09:38:17 PDT
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.
Comment 3 Daewoong Jang 2014-08-27 11:41:22 PDT
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.
Comment 4 Pratik Solanki 2014-08-27 11:57:45 PDT
isSafeToMakePurgeable() used to do

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

Should this patch be checking for the other two conditions as well?
Comment 5 Darin Adler 2014-08-27 11:58:25 PDT
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.
Comment 6 Darin Adler 2014-08-27 11:59:34 PDT
(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.
Comment 7 Daewoong Jang 2014-08-27 17:58:15 PDT
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.
Comment 8 Daewoong Jang 2014-08-27 18:34:32 PDT
And I will not forget to mention about the details next time. Thanks Darin.
Comment 9 Pratik Solanki 2014-08-27 23:16:56 PDT
Patch seems reasonable to me. But can you add more information in the ChangeLog? Please reference the commit/bug that caused this issue.
Comment 10 Daewoong Jang 2014-08-27 23:30:28 PDT
(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.
Comment 11 Daewoong Jang 2014-08-28 11:41:33 PDT
Created attachment 237320 [details]
updated patch
Comment 12 Darin Adler 2014-08-29 09:23:08 PDT
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.
Comment 13 Daewoong Jang 2014-08-29 10:27:46 PDT
(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.
Comment 14 Daewoong Jang 2014-08-29 13:58:22 PDT
Created attachment 237374 [details]
test case
Comment 15 Daewoong Jang 2014-08-29 14:07:42 PDT
I've done the test case, but where should I put this? Please take a look and give me some advice.
Comment 16 Brian Burg 2014-09-01 15:26:41 PDT
(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/.
Comment 17 Daewoong Jang 2014-09-01 17:16:54 PDT
(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!
Comment 18 WebKit Commit Bot 2014-09-02 08:54:42 PDT
Comment on attachment 237320 [details]
updated patch

Clearing flags on attachment: 237320

Committed r173172: <http://trac.webkit.org/changeset/173172>
Comment 19 WebKit Commit Bot 2014-09-02 08:54:47 PDT
All reviewed patches have been landed.  Closing bug.