Follow-up (r213833): write a layout test for 169199

• Component: Images
• Status: RESOLVED FIXED

[Said Abou-Hallawa]

A new internal API is needed to force the ImageDecoder to be deleted. A function like this needs to be called from the internal API.

void BitmapImage::clearDecoder()
{
    m_source.clear(data());
}

When new an image frame is requested and without r213833, a crash should occur. To expose a BitmapImage function through Internals, have look at Internals::imageFrameIndex(). And to write a test for animated images, have a look fast/images/animated-image-loop-count.html.

[Radar WebKit Bug Importer]

<rdar://problem/31024766>

[Miguel Gomez]

I'm working on this.

I have the test that deletes the decoder and works fine. But without r213833 it doesn't necessarily cause a crash, as it's a race condition.

With the implementation of BitmapImage::clearDecoder() you suggest, ImageSource::clear() calls setData() just after setting the ImageFrameCache decoder to nullptr, and that setData() creates a new decoder that the decoding thread will use without crashing.

The crash happens only when the decoding thread is using the decoder exactly between its destruction and the creation of the new one, which is pretty hard to reproduce. I could force it though, using an implementation of BitmapImage::clearDecoder() that just deletes the decoder and doesn't cause setData() to be called.

Should I do that? Or should I use the implementation you suggest, even if the crash is not easily reproducible without r213833?

[Said Abou-Hallawa]

(In reply to comment #2)
> I'm working on this.
> 
> I have the test that deletes the decoder and works fine. But without r213833
> it doesn't necessarily cause a crash, as it's a race condition.
> 
> With the implementation of BitmapImage::clearDecoder() you suggest,
> ImageSource::clear() calls setData() just after setting the ImageFrameCache
> decoder to nullptr, and that setData() creates a new decoder that the
> decoding thread will use without crashing.
> 
> The crash happens only when the decoding thread is using the decoder exactly
> between its destruction and the creation of the new one, which is pretty
> hard to reproduce. I could force it though, using an implementation of
> BitmapImage::clearDecoder() that just deletes the decoder and doesn't cause
> setData() to be called.
> 
> Should I do that? Or should I use the implementation you suggest, even if
> the crash is not easily reproducible without r213833?

I think you are right. You can instead add a setting flag which clears the decoder immediately  after requesting a new frame in BitmapImage::internalStartAnimation(). You can do something similar to Internals::setImageFrameDecodingDuration().

You can also try run-webkit-tests with --repeat-each=100 for example. This might cause the crash to happen with scenarios like this.

[Miguel Gomez]

Created attachment 304630 [details]
Patch

[Said Abou-Hallawa]

Comment on attachment 304630 [details]
Patch

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

I assume you tried this test without r213833 and it crashed.

> Source/WebCore/ChangeLog:10
> +        requesting de async decoding of a frame, and implement that behaviour in BitmapImage.

de => an (maybe)

> Source/WebCore/testing/Internals.cpp:747
> +void Internals::clearDecoderAfterAsyncFrameRequestForTesting(HTMLImageElement& element)

This name does not reveal what this function is actually doing. Since it is just setting a flag, I would suggest the renaming it to be setClearDecoderAfterAsyncFrameRequestForTesting() or enableClearDecoderAfterAsyncFrameRequestForTesting(). I would suggest also adding a bool flag to enable/disable m_clearDecoderAfterAsyncFrameRequestForTesting instead of enabling it only.

[Miguel Gomez]

> I assume you tried this test without r213833 and it crashed.

I tried, and it crashes most of the times. There's still some probability of it not crashing, but seems to be quite small with this approach.

As long as ImageSource::clear is used to clear the decoder (which destroys the decoder and instantly creates a new one due to the call to ImageSource::setData), the crash cannot be guaranteed 100% of the times.

In order to guarantee it we would need to clear the decoder in ImageSource without calling ImageSource::setData. But this would mean adding a new function to do so and create a situation that cannot happen in the normal execution of the code. I didn't do so because I'm not sure that's expected from a test, but I'll be happy to do it if you prefer it that way.

[Said Abou-Hallawa]

(In reply to comment #6)
> > I assume you tried this test without r213833 and it crashed.
> 
> I tried, and it crashes most of the times. There's still some probability of
> it not crashing, but seems to be quite small with this approach.
> 
> As long as ImageSource::clear is used to clear the decoder (which destroys
> the decoder and instantly creates a new one due to the call to
> ImageSource::setData), the crash cannot be guaranteed 100% of the times.
> 
> In order to guarantee it we would need to clear the decoder in ImageSource
> without calling ImageSource::setData. But this would mean adding a new
> function to do so and create a situation that cannot happen in the normal
> execution of the code. I didn't do so because I'm not sure that's expected
> from a test, but I'll be happy to do it if you prefer it that way.

If you run-webkit-tests --repeat-each=100 for your test and without r213833 and the test crashes only once that would be enough.

[Miguel Gomez]

> If you run-webkit-tests --repeat-each=100 for your test and without r213833
> and the test crashes only once that would be enough.

Sure. It crashes more than 50% of the times.

[Miguel Gomez]

Created attachment 304770 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 304770 [details]
Patch

Clearing flags on attachment: 304770

Committed r214103: <http://trac.webkit.org/changeset/214103>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.