1126038 - Finish decoding off-main-thread

Status: RESOLVED FIXED

(Core :: Graphics: ImageLib, defect)

Description

[Seth Fowler [:seth] [:s2h]]

Right now, Decoder::Finish only runs on the main thread.

This is problematic for two reasons:

1. It wastes main thread resources, because much of the work in Decoder::Finish can actually be done off-main-thread.

2. More seriously, it can cause deadlock, because for some image formats we don't call Decoder::PostFrameStop (which is responsible for calling imgFrame::Finish) until we call Decoder::Finish, and if the main thread is blocked in imgFrame::WaitUntilComplete, waiting for that imgFrame::Finish call on the same frame... kaboom.

So let's fix this.

I actually think the whole decoder shutdown sequence needs a complete refactoring (we have Decoder::Finish, Decoder::FinishInternal, RasterImage::OnDecodingComplete, RasterImage::FinalizeDecoder... too much, and in this bug I'll make it worse) and I intend to file a bug to do that soon, but I think this particular issue should be fixed separately. For one thing, it's blocking the landing of bug 1125490, but it's also generally important and I don't want it to get backed out if I make a mistake in the subsequent refactoring.

Comment 1

[Seth Fowler [:seth] [:s2h]]

Attachment: Finish decoding off-main-thread

Here's the patch.

Comment 2

[Seth Fowler [:seth] [:s2h]]

Try job here:

https://tbpl.mozilla.org/?tree=Try&rev=f218fdbcd699

Comment 3

[Seth Fowler [:seth] [:s2h]]

Attachment: Finish decoding off-main-thread

The tests there look good except for a devtools failure which is caused by us
failing to print a error to the console for a corrupt image. This seems to be
because the code in CompleteDecode() considers the image usable and calls
PostFrameStop(), PostDecodeDone(), etc., so we don't enter the similar |if|
branch in Finish(). This version of the patch is the same except that I've added
a flag to remember that we should report an error to the console in Finish().

New Linux-only try job here, to verify that this fixes the problem:

https://tbpl.mozilla.org/?tree=Try&rev=016dd3de7eaf

Comment 4

[Seth Fowler [:seth] [:s2h]]

Thanks for the review! Pushed:

https://hg.mozilla.org/integration/mozilla-inbound/rev/00f4c6c6f553

Comment 5

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/00f4c6c6f553

Comment 6

[Seth Fowler [:seth] [:s2h]]

Comment on attachment 8554953 [details] [diff] [review]
Finish decoding off-main-thread

Approval Request Comment
[Feature/regressing bug #]: Part of fix for bug 1125272 and bug 1119938.
[User impact if declined]: Already approved.
[Describe test coverage new/current, TreeHerder]: In 38 for a long time.
[Risks and why]: Low risk; in 38 for a long time.
[String/UUID change made/needed]: None.

Comment 7

[Seth Fowler [:seth] [:s2h]]

Pushed to Aurora:

https://hg.mozilla.org/releases/mozilla-aurora/rev/6dc06329002f

Comment 8

[Lawrence Mandel [:lmandel] (use needinfo)]

Comment on attachment 8554953 [details] [diff] [review]
Finish decoding off-main-thread

Aurora approval previously granted for a collection of ImageLib related bugs that Seth worked on and QE verified before uplift.