Bug 169576 - Follow-up (r213833): write a layout test for 169199
Summary: Follow-up (r213833): write a layout test for 169199
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Images (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Miguel Gomez
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2017-03-13 17:01 PDT by Said Abou-Hallawa
Modified: 2017-03-17 05:22 PDT (History)
6 users (show)

See Also:


Attachments
Patch (8.07 KB, patch)
2017-03-16 04:17 PDT, Miguel Gomez
no flags Details | Formatted Diff | Diff
Patch (8.19 KB, patch)
2017-03-17 03:24 PDT, Miguel Gomez
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Said Abou-Hallawa 2017-03-13 17:01:46 PDT
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.
Comment 1 Radar WebKit Bug Importer 2017-03-13 17:03:49 PDT
<rdar://problem/31024766>
Comment 2 Miguel Gomez 2017-03-14 10:31:11 PDT
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?
Comment 3 Said Abou-Hallawa 2017-03-14 10:45:59 PDT
(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.
Comment 4 Miguel Gomez 2017-03-16 04:17:39 PDT
Created attachment 304630 [details]
Patch
Comment 5 Said Abou-Hallawa 2017-03-16 08:53:28 PDT
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.
Comment 6 Miguel Gomez 2017-03-16 09:13:56 PDT
> 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.
Comment 7 Said Abou-Hallawa 2017-03-16 10:10:47 PDT
(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.
Comment 8 Miguel Gomez 2017-03-17 03:21:12 PDT
> 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.
Comment 9 Miguel Gomez 2017-03-17 03:24:41 PDT
Created attachment 304770 [details]
Patch
Comment 10 WebKit Commit Bot 2017-03-17 05:22:14 PDT
Comment on attachment 304770 [details]
Patch

Clearing flags on attachment: 304770

Committed r214103: <http://trac.webkit.org/changeset/214103>
Comment 11 WebKit Commit Bot 2017-03-17 05:22:22 PDT
All reviewed patches have been landed.  Closing bug.