Bug 175595 – Remove the ImageSource from the class hierarchy that connects BitmapImage to ImageFrame

• Component: Images
• Status: RESOLVED FIXED

[Said Abou-Hallawa]

Currently we have this hierarchy: BitmapImage -> ImageSource -> ImageFrameCache -> ImageDecoder + ImageFrame. This hierarchy structure has been troublesome. Two reasons for that:

1) Unclear purpose of ImageSource: The initial plan was to have ImageSource be responsible of retrieving and caching the image metadata. And have ImageFrameCache be responsible of retrieving and caching the image frames. This plan changed gradually by having ImageFrameCache be responsible for both. ImageSource mainly became a bridge connecting BitmapImage to ImageFrameCache. Other than that, it has few APIs which set the encoded data in the ImageDecoder. Some other APIs ensure the decoder is created and the rest are just helper APIs. All these APIs can fit better in either BitmapImage or ImageFrameCache.

2) The life cycle of the decoder has become problematic and hard to be understood: This is because it is shared among
   -- ImageSource (for setting the encoded data)
   -- ImageFrameCache (for retrieving the image metadata and decoding the image frames synchronously)
   -- The decoding thread (for decoding the image frames asynchronously)

The way it shared is the following. The ImageSource creates the ImageDecoder and passes a RefPtr to ImageDecoder to the ImageFrameCache. The ImageFrameCache passes a RefPtr to ImageDecoder to the decoding thread. We saw few crashes because the ImageDecoder was not a RefPtr at the beginning. Then we saw other crashes because the ImageDecoder was not protected by the decoding thread.

To fix this issue, here is the plan:
-- Get rid of ImageSource by moving its APIs to ImageFrameCache and BitmapImage.
-- Have BitmapImage::m_source a RefPtr of type ImageFrameCache
-- In followup patch rename ImageFrameCache to be ImageSource

[Said Abou-Hallawa]

Created attachment 318163 [details]
Patch

[Said Abou-Hallawa]

Created attachment 318172 [details]
Patch

[Said Abou-Hallawa]

Created attachment 318175 [details]
Patch

[Simon Fraser (smfr)]

Comment on attachment 318175 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=318175&action=review

> Source/WebCore/WebCore.xcodeproj/project.pbxproj:-29487
> -				5182C23F1F313A090059BA7C /* ServiceWorker.h in Headers */,
>  				51F1755F1F3EBC8300C74950 /* ServiceWorkerContainer.h in Headers */,
> -				5182C2411F313A090059BA7C /* ServiceWorkerContainer.h in Headers */,
>  				51F175611F3EBC8300C74950 /* ServiceWorkerGlobalScope.h in Headers */,
> -				5182C2431F313A090059BA7C /* ServiceWorkerGlobalScope.h in Headers */,
>  				51F175631F3EBC8300C74950 /* ServiceWorkerJob.h in Headers */,
> -				511CA67E1F3905A60019E074 /* ServiceWorkerJob.h in Headers */,
>  				51F175641F3EBC8300C74950 /* ServiceWorkerJobClient.h in Headers */,
> -				511CA6801F39331F0019E074 /* ServiceWorkerJobClient.h in Headers */,
> -				511CA67A1F3904B10019E074 /* ServiceWorkerProvider.h in Headers */,
>  				51F175661F3EBC8300C74950 /* ServiceWorkerProvider.h in Headers */,
>  				51F175681F3EBC8300C74950 /* ServiceWorkerRegistration.h in Headers */,
> -				5182C2451F313A090059BA7C /* ServiceWorkerRegistration.h in Headers */,
>  				51F175691F3EBC8300C74950 /* ServiceWorkerRegistrationOptions.h in Headers */,
>  				51F1756B1F3EBC8300C74950 /* ServiceWorkerRegistrationParameters.h in Headers */,
>  				51F1756C1F3EBC8300C74950 /* ServiceWorkerUpdateViaCache.h in Headers */,
> -				51F174FF1F35899700C74950 /* ServiceWorkerUpdateViaCache.h in Headers */,

I think you may have done something bad to the project here.

> Source/WebCore/platform/graphics/BitmapImage.h:206
> -    mutable ImageSource m_source;
> +    mutable Ref<ImageFrameCache> m_source;

Is there a reason ImageFrameCache can't just be by value?

[Said Abou-Hallawa]

Created attachment 318286 [details]
Patch

[Said Abou-Hallawa]

Comment on attachment 318175 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=318175&action=review

>> Source/WebCore/WebCore.xcodeproj/project.pbxproj:-29487
>> -				51F174FF1F35899700C74950 /* ServiceWorkerUpdateViaCache.h in Headers */,
> 
> I think you may have done something bad to the project here.

I just deleted ImageSource.cpp and ImageSource.h from the WebKit xcode project. When I applied this patch locally I did not see these weird deletions in project.pbxproj. I guess it is an issue with xcode itself. The new patch does not have this weirdness.

>> Source/WebCore/platform/graphics/BitmapImage.h:206
>> +    mutable Ref<ImageFrameCache> m_source;
> 
> Is there a reason ImageFrameCache can't just be by value?

I think we can't do that. ImageFrameCache inherits from ThreadSafeRefCounted<ImageFrameCache>. The decoding thread calls makeRef(*this) to have a protected instance of ImageFrameCache. Both BitmapImage and the decoding thread have to have two RefPtrs encapsulating the same ImageFrameCache object. The destructor of these RefPtrs will decide when the ImageFrameCache object itself is safe to be deleted.

If I make BitmapImage::m_source be of type ImageFrameCache, the destructor of BitmapImage will destroy it immediately even if the protected thread holds a RefPtr to the same  ImageFrameCache object.

[Said Abou-Hallawa]

Created attachment 327891 [details]
Patch

[Said Abou-Hallawa]

Created attachment 327899 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 327899 [details]
Patch

Clearing flags on attachment: 327899

Committed r225300: <https://trac.webkit.org/changeset/225300>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/35759071>