1079627 - Make RasterImage support multiple decoders at once

Status: RESOLVED FIXED

(Core :: Graphics: ImageLib, defect)

Description

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

This is a metabug with the goal of supporting multiple decoders at once in RasterImage, which is needed to support downscale-during-decode and should generally increase the sanity of the RasterImage/Decoder code.

Comment 1

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

Attachment: (Part 1) - Make image decoders hold a strong reference to their image

OK, here's part 1. In a later part we're going to stop holding a reference to decoders on RasterImage, so we need to instead do the reverse and have decoders keep their parent RasterImage alive.

This may well cause leaks without the other patches in this bug, but I think it's worth separating out anyway, because it touches a lot of files. There's nothing complicated in here, though. (The only substantial addition, the code to release Decoder::mImage on the main thread if the Decoder is destroyed off-main-thread, is copied directly from DecodePool, where it's been in use for a long time.)

Comment 2

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

Attachment: (Part 2) - Add SourceBuffer

This part adds SourceBuffer, a single producer multiple consumer data structure.
It's designed to allow the producer to write to the buffer without blocking the
consumers, and for the consumers to be able to read without blocking each other.

Hopefully the general idea is clear from the comments. Let me know if there's
anything that could use further explanation.

I mentioned it in a comment in the code as well, but there's one way in which
this is inferior to the current design, and that's that if we don't get a
Content-Length header (and thus don't call SourceBuffer::ExpectLength) then the
buffer will end up with multiple chunks and some slack space at the end, and
there's currently no way to compact it. I do plan to implement support for
compacting the buffer, but I think it's better to do it in a followup since it
adds complexity, and landing it separately reduces risk on the critical path to
downscale-during-decode.

Comment 3

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

Attachment: (Part 2) - Add SourceBuffer

Whoops, I realized that the version I just uploaded had a bunch of printf calls in it. Removed the printfs.

Comment 4

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

Attachment: (Part 3) - Support multiple decoders for a single RasterImage

OK, here's the main event. Even though it doesn't touch that many files, it's a
pretty big patch.

The basic idea here is: RasterImage does not communicate directly with decoders
at all once they've been created and sent off to the decode pool. It doesn't
even keep a reference to them - all of the state and code in RasterImage that
had to do with that has been removed. Instead, decoders communicate with a
SourceBuffer (which provides their input and manages synchronization issues on
that end) and the SurfaceCache (which stores their output and implicitly aborts
unneeded decoders). RasterImage's Necko callbacks write to the SourceBuffer, and
RasterImage reads imgFrame objects via LookupFrame, which accesses the
SurfaceCache. No direct communication with decoders is necessary.

This mostly just works! The one area which is a little more subtle is sync
decoding. A high level issue first: there are actually two types of sync decodes
we support in ImageLib right now. The first is kind is a *true* sync decode,
where we block the main thread until an image is decoded; this is indicated by
the FLAG_SYNC_DECODE Draw flag and is used for things like |canvas.drawImage|.
The other kind of sync decode is a heuristic sync decode that's designed to
ensure small images decode right away; we run this kind of sync decode when
StartDecoding is called. Previously, the heuristic sync decode ran for a certain
amount of *time*. However, this patch makes the heuristic sync decode instead
check if the image's source data is under a certain number of bytes. I did this
to simplify the implementation of SourceBuffer, DecodePool, and Decoder, since
all that code is simplified by not supporting the periodic yielding that a time
limit would require. I think this is actually an improvement on the status quo,
because the existing time limit approach means that we waste time sync decoding
images on the main thread that we don't end up completely decoding. However, I
don't think we should stick with this approach either; my favored approach in
the long term continues to be to remember how long it took us to decode an image
the first time, and use that information to decide whether to sync decode in the
future. That will have to wait for a followup, though.

So we implement sync decoding in two ways, depending on the situation.

1. If an async decode was already running for the size and flags we need, we
just block using imgFrame::WaitUntilComplete. (We actually added the code for
this in bug 1118087, but we continue to rely on it here.)

2. If there is no async decode currently running for the size and flags we need,
we go ahead and perform the sync decode on the main thread via
DecodePool::SyncDecodeIfPossible.

There is a third case, though:

3. We could start sync decoding after we've started a async decoder for the same
size and flags, but before that async decoder starts running. In that situation,
the async decoder will actually run, but as soon as it parses the header it will
try to insert a frame into the SurfaceCache and discover that a frame with the
same size and flags is already present. In that situation, the async decoder
marks itself aborted and stops decoding. An aborted decoder is considered to be
a transient error, so we don't notify the image's observers or call
RasterImage::DoError; we just drop the decoder and move on. We may also arrive
at this situation if the async decoder starts running slightly before the sync
decoder, in which case the sync decoder will get aborted, and we'll end up
falling back to case 1 above. It's also possible to hit this if two async
decoders get started for the same image in very quick succession.

The sync/async races in case 3 are difficult to avoid, but I think we can handle
them more efficiently than we do in this patch, because in some cases we could
catch them before we do any unnecessary work at all. The async/async races are
totally avoidable.  Both of these issues can fixed by keeping track of the
SurfaceKey hashes of decoders that are running, and checking or updating that
list as appropriate. I'm going to write that code but I think it's appropriate
for a followup, because doing this does add some additional complexity, and it
could mask bugs in this patch which I'd rather know about.

That said, other than suboptimal efficiency in this third case (which should not
be common), this patch should cover all the bases of enabling multiple decoders
for a single RasterImage. Let me know if there's anything that's unclear.
Hopefully the new code is mostly simpler and easier to understand than the old
code it replaces. Some of the changes that were necessary, like splitting apart
FinishedSomeDecoding into NotifyProgress and FinalizeDecoder, ended up producing
code that strikes me as cleaner and more reusable as well.

Comment 5

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

I mentioned some of them above, but I want to record here a list of followup
bugs that will continue the work in this patch. (I'll actually file these soon,
but I want to make sure they don't get lost.)

- Adding compacting support to SourceBuffer.

- Track the SurfaceKey hashes (or some similar value) of running decoders to
  improve our behavior with respect to comment 4's "case 3".

- Rename or otherwise rethink LookupFrame’s aShouldSyncNotify argument. See the
  comments in RasterImage::RequestDecode and RasterImage::StartDecoding.

- Target Necko notifications to a different thread (not in the DecodePool thread
  pool) so that they are guaranteed to run in parallel with the decoders.

- Remove CONTAINER_ENSURE_SUCCESS and friends from RasterImage.cpp, since they
  aren't called anymore. (I've actually already written the patch for this one.)

Comment 6

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

Attachment: (Part 3) - Support multiple decoders for a single RasterImage

I noticed a bug today; we should do the synchronous size decode in OnImageDataComplete after we call SourceBuffer::Complete. I've updated the patch.

Comment 7

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

Attachment: (Part 3) - Support multiple decoders for a single RasterImage

Wow, having APZ on is really good for flushing out bugs! I also noticed that
it's normal, and expected, for us to have not set the size on the image even if
the decoder has a size if |RasterImage::mError| is true, because in that case
RasterImage::SetSize refuses to do anything. So the assertion at the beginning
of FinalizeDecoder needed to check mError too.

Comment 8

[Timothy Nikkel (:tnikkel)]

Comment on attachment 8544428 [details] [diff] [review]
(Part 2) - Add SourceBuffer

Say AdvanceOrScheduleResume put the consumer in the waiting consumers list then returns. The consumer then cleans up (which from the next patch looks like it just returns), but whats to prevent the SourceBuffer from calling Resume before the consumer has cleaned up?

Comment 9

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

(In reply to Timothy Nikkel (:tn) from comment #8)
> Say AdvanceOrScheduleResume put the consumer in the waiting consumers list
> then returns. The consumer then cleans up (which from the next patch looks
> like it just returns), but whats to prevent the SourceBuffer from calling
> Resume before the consumer has cleaned up?

Absolutely nothing. The consumer must be written to handle that correctly. (And Decoder::Decode() does.) Of course, the consumer can make Resume() do whatever they want, so if they need locking or something they can handle it themselves.

Comment 10

[Timothy Nikkel (:tnikkel)]

Comment on attachment 8544974 [details] [diff] [review]
(Part 3) - Support multiple decoders for a single RasterImage

>@@ -937,71 +896,68 @@ RasterImage::OnAddedFrame(uint32_t aNewF
>+      if (aNewFrameCount > 1) {
>+        mAnim->UnionFirstFrameRefreshArea(aNewRefreshArea);
>+      }
>     }
>   }
>-
>-  if (aNewFrameCount > 1) {
>-    mAnim->UnionFirstFrameRefreshArea(aNewRefreshArea);
>-  }
> }

This makes us do the UnionFirstFrameRefreshArea call only when aNewFrameCount == 2, that doesn't seem right.

Comment 11

[Timothy Nikkel (:tnikkel)]

Comment on attachment 8544974 [details] [diff] [review]
(Part 3) - Support multiple decoders for a single RasterImage

>@@ -2015,46 +1650,41 @@ RasterImage::Draw(gfxContext* aContext,
>     return NS_ERROR_FAILURE;
> 
>   NS_ENSURE_ARG_POINTER(aContext);
> 
>   if (IsUnlocked() && mProgressTracker) {
>     mProgressTracker->OnUnlockedDraw();
>   }
> 
>-  // We use !mDecoded && mHasSourceData to mean discarded.
>-  if (!mDecoded && mHasSourceData) {
>-    mDrawStartTime = TimeStamp::Now();
>-  }
>-
>-  // If a synchronous draw is requested, flush anything that might be sitting around
>-  if (aFlags & FLAG_SYNC_DECODE) {
>-    nsresult rv = SyncDecode();
>-    NS_ENSURE_SUCCESS(rv, rv);
>-  }
>-
>   // XXX(seth): For now, we deliberately don't look up a frame of size aSize
>   // (though DrawWithPreDownscaleIfNeeded will do so later). It doesn't make
>   // sense to do so until we support downscale-during-decode. Right now we need
>   // to make sure that we always touch an mSize-sized frame so that we have
>   // something to HQ scale.
>-  DrawableFrameRef ref = LookupFrame(GetRequestedFrameIndex(aWhichFrame),
>-                                     mSize, aFlags);
>+  DrawableFrameRef ref =
>+    LookupFrame(GetRequestedFrameIndex(aWhichFrame), mSize, aFlags);
>   if (!ref) {
>     // Getting the frame (above) touches the image and kicks off decoding.
>+    if (mDrawStartTime.IsNull()) {
>+      mDrawStartTime = TimeStamp::Now();
>+    }
>     return NS_OK;
>   }
> 
>+  bool shouldRecordTelemetry = !mDrawStartTime.IsNull() &&
>+                               ref->IsImageComplete();
>+
>   DrawWithPreDownscaleIfNeeded(Move(ref), aContext, aSize,
>                                aRegion, aFilter, aFlags);
> 
>-  if (mDecoded && !mDrawStartTime.IsNull()) {
>+  if (shouldRecordTelemetry) {
>       TimeDuration drawLatency = TimeStamp::Now() - mDrawStartTime;
>-      Telemetry::Accumulate(Telemetry::IMAGE_DECODE_ON_DRAW_LATENCY, int32_t(drawLatency.ToMicroseconds()));
>-      // clear the value of mDrawStartTime
>+      Telemetry::Accumulate(Telemetry::IMAGE_DECODE_ON_DRAW_LATENCY,
>+                            int32_t(drawLatency.ToMicroseconds()));
>       mDrawStartTime = TimeStamp();
>   }
> 
>   return NS_OK;
> }

Isn't calling LookupFrame going to do some decoding in some cases? So initializing mDrawStartTime after the LookupFrame call is going to miss some decoding time in the measurement?

Comment 12

[Timothy Nikkel (:tnikkel)]

(In reply to Seth Fowler [:seth] from comment #9)
> (In reply to Timothy Nikkel (:tn) from comment #8)
> > Say AdvanceOrScheduleResume put the consumer in the waiting consumers list
> > then returns. The consumer then cleans up (which from the next patch looks
> > like it just returns), but whats to prevent the SourceBuffer from calling
> > Resume before the consumer has cleaned up?
> 
> Absolutely nothing. The consumer must be written to handle that correctly.
> (And Decoder::Decode() does.) Of course, the consumer can make Resume() do
> whatever they want, so if they need locking or something they can handle it
> themselves.

Say that the AdvanceOrScheduleResume call in Decoder::Decode has scheduled a resume and returned control to Decoder::Decode on thread A, then the source buffer gets more data and resumes the waiting consumers (on some other thread), we resume the decoder on thread B this time and get to Decoder::Decode and it calls AdvanceOrScheduleResume, which sets the source iterator to ready. Now thread A proceeds and checks mIterator->IsReady() and finds that it is ready so it continues decoding, and thread B does the same. Or is that not possible?

Comment 13

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

Attachment: (Part 2) - Add SourceBuffer

That totally is possible. Thanks for pointing it out!

I've fixed it in this version of the patch by returning the new state from
AdvanceOrScheduleResume, instead of having the caller check a getter. I removed
both HasMore() and IsReady() from the public API since they are both
error-prone; now callers should exclusively rely on the returned new state
value. I've updated the documentation as well.

Comment 14

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

Attachment: (Part 3) - Support multiple decoders for a single RasterImage

Updated part 3 to use the new API.

Comment 15

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

Attachment: (Part 2) - Add SourceBuffer

This new version of part 2 fixes a few bugs:

- We manually clear the nsTArray's instead of relying on their destructor, since
  they do not seem to call the destructors of their elements.

- There's a new heuristic to make sure that allocations aren't unreasonably
  large before we try to make them.

- We forbid copying of SourceBufferIterators. This caused unnecessary refcount
  bouncing.

Comment 16

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

Attachment: (Part 3) - Support multiple decoders for a single RasterImage

This version of the patch includes some minor fixes, most importantly:

- A rebase against the new part 2.

- A fix for an issue pointed out by the static analyzer in DecodePool.

- Some gfxPrefs that were uselessly "Live" have been made "Once".

Also, I disabled some tests that were already intermittent oranges and became
more frequent after this patch. This series of patches is doing a lot to improve
the reproducibility of intermittent oranges, which is both a good thing and a
bad thing. =)

I also disabled one other test,
|toolkit/content/tests/chrome/test_preferences.xul|, on most platforms. I'm
having a hard time understanding why this patch should cause a timeout in this
test. I'm not even sure that this test is really the cause. I did quite a bit of
binary searching through the mochitests in this directory and was unable to
determine exactly where the problem lies, and could make the failure appear or
disappear by disabling other tests, not just this one. This is the one that
shows up on TBPL, though, and disabling this test seems to fix it reliably
locally. (I left it on on OS X so we have some coverage; the problem doesn't
happen there.)

I fully intend to come back to the |test_preferences.xul| issue, and will file a
followup bug tomorrow. I think disabling it is the best course of action right
now, though, because it looks like the type of thing that is going to take quite
a while to debug.

Comment 17

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

I went ahead and pushed since the tree is orange right now and I think these patches will fix it. If not, I apologize in advance for the number of backouts that are going to have to happen. =(

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/3dd5401f359c
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/4d93dcf74f01
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/460c36a4666a
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/a8044fd506db

Comment 18

[Tom S [:evilpie]]

Backout
https://hg.mozilla.org/integration/mozilla-inbound/rev/8f3cc2c90893

Comment 19

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

Attachment: (Part 4) - Support multiple decoders in the presence of frame preallocation

Timothy, as we discussed on IRC the problematic patch seems confirmed to be bug
1117607. Since you've already reviewed the other patches in this bug, I didn't
want to make any big changes in them, so this is a new part that rebases the
preceding patches to work without bug 1117607.

The issues that are solved here are all related to
imgFrame::WaitUntilComplete(), which we rely on much more heavily after this bug
lands.

The main difficulty is that if we preallocate frames, then up to now
we've had to replace the preallocated imgFrame for images which have padding in
the first frame, because we created that imgFrame with the wrong parameters. But
sync decoding callers may have already retrieved that preallocated imgFrame from
the SurfaceCache and have called WaitUntilComplete() on it. If we just destroy
the preallocated one, those callers draw the wrong imgFrame - they think
decoding got aborted!

I solved that problem by adding imgFrame::ReinitForDecoder, which makes to
possible to reinitialize an imgFrame with new parameters as long as it has never
been written to and Finish() and Abort() have not been called. I wrote it *very*
conservatively; I could probably have avoided resetting most of those member
variables, but I wanted to be very careful that the state after calling
ReinitForDecoder is the same as the state after creating a fresh decoder and
calling InitForDecoder.

There was also a second bug, which is actually a bug in a previous patch (the
one for bug 1118087) where didn't always notify correctly in imgFrame::Abort. If
I can't land this patch, I'll pull this piece out separately and push it as a
followup. I didn't notice the issue because it's very hard to hit without this
patch.

Comment 20

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

Thanks for the review!

My try job here looks good:

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

So I went ahead and pushed:

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/c86c43915254
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/44b622a479b6
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/e7add8446221
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/14cc155b0d6e

Comment 21

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

It looks like on Windows XP this was periodically causing an OOM in |caps/tests/mochitest/test_bug995943.xul|, despite taking great pains to handle memory allocation failure, because UniquePtr's MakeUnique uses an infallible allocator. I switched to using nsTArray's fallible allocator in this followup:

https://hg.mozilla.org/integration/mozilla-inbound/rev/e8ddeaeb82ee

I also disabled |image/test/reftest/gif/webcam.html|, which became a frequent intermittent orange on E10S after pushing this. (I disabled it everywhere because I'm not sure whether it happens anywhere besides E10S, and I'm getting too tired to stay up.)

This stuff can get tweaked further tomorrow.

Comment 22

[Ryan VanderMeulen [:RyanVM][PTO June 24-28]]

Backed out for causing at least bug 1120448. The trees are such a mess at the moment that I'm not sure if there are others lingering around from this too.
https://hg.mozilla.org/integration/mozilla-inbound/rev/cb26891d69e9

Comment 23

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

Attachment: (Part 2) - Add SourceBuffer

OK, given that this got backed out anyway, I am going to create cleaner fixes
for the issues in comment 21. First up: this new version of SourceBuffer
allocates memory using a fallible allocator *correctly* - I consulted khuey for
the correct API to use.

Comment 24

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

Attachment: (Part 3) - Support multiple decoders for a single RasterImage

We no longer need to disable test_preferences.xul, so I removed that from this patch.

Comment 25

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

Attachment: (Part 5) - Allow recursive notifications in NotifyProgress

OK, this change both simplifies the code and eliminates the need to disable
|test_preferences.xul|, which was failing because modal dialogs can run a new
event loop with RasterImage::NotifyProgress lower on the stack.

Comment 26

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

Attachment: (Part 6) - Lock the image during decoding

This patch makes us lock the image during decoding, which ensures that we don't
end up discarding the first frame of the image before we realize that it's
animated.

Comment 27

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

Attachment: (Part 7) - Disable some questionable tests

This patch disables some questionable tests. We really need to rewrite a lot of
the ImageLib tests, unfortunately.

- delaytest.html just waits a fixed number of milliseconds before taking the
  reftest snapshot. It's really no surprise that this fails on the B2G
  emulators, as that's totally unsafe there because the emulator timing is
  weird. (It's not great anywhere, really.) This type of test just can't be a
  reftest.

- gif/webcam.html, which tests GIFs in a webcam simulation, cannot possibly be
  reliable before bug 1113465 lands, because we currently send a load event for
  every frame in a multipart image. After bug 1113465 lands, it will be
  irrelevant, because we won't support GIFs in multipart images anymore. (And
  indeed, this test is removed in that bug.)

Comment 28

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

Here's a test run with all of these patches applied, which looks green everywhere:

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

Comment 29

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

Thanks for the reviews! This should be ready to push.

Comment 30

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

Pushed:

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/3460bb4b7090
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/7487bc892f0f
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/d5462de77e9f
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/5286fe9dfc72
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/2a500e92e1ae
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/f8846665998a
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/daf8243cd190

Comment 31

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

Pushed a followup:

https://hg.mozilla.org/integration/mozilla-inbound/rev/0c2393315416

To prevent a backout due to occasional failures of test-image-layers-multiple-displayitem.html with "failed reftest-no-paint". Example:

https://treeherder.mozilla.org/logviewer.html#?job_id=5531444&repo=mozilla-inbound

This can be fixed in a followup.

Comment 32

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/3460bb4b7090
https://hg.mozilla.org/mozilla-central/rev/7487bc892f0f
https://hg.mozilla.org/mozilla-central/rev/d5462de77e9f
https://hg.mozilla.org/mozilla-central/rev/5286fe9dfc72
https://hg.mozilla.org/mozilla-central/rev/2a500e92e1ae
https://hg.mozilla.org/mozilla-central/rev/f8846665998a
https://hg.mozilla.org/mozilla-central/rev/daf8243cd190
https://hg.mozilla.org/mozilla-central/rev/0c2393315416

Comment 33

[Alice0775 White]

@:seth

Could you uplift this to Aurora37.0a2 ?
Bug 1119938(Bug 1120036, Bug 1124931) is still existing in Aurora37.0a2.

Comment 34

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

(In reply to Alice0775 White from comment #33)
> @:seth
> 
> Could you uplift this to Aurora37.0a2 ?
> Bug 1119938(Bug 1120036, Bug 1124931) is still existing in Aurora37.0a2.

We cannot uplift this yet because of bug 1122704. I hope to reach a resolution for that, or at least an agreement about what to do, soon.

Comment 35

[Emanuel Hoogeveen [:ehoogeveen]]

I'll be honest, if it were up to me I'd take the memory regression over a massively slower browser, which is what I believe people on the FDE/Aurora channel are experiencing right now (at least on Windows?). It's not ideal either way, of course; hopefully it won't matter in a few days.

Comment 36

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

For people who are waiting for this to be uplifted to Aurora: it's ready now, but unfortunately I'm going on vacation for a week, and the job is complex enough that I can't rush through doing it tonight. Other folks may take it over from me and do it before I get back, but in the worst case, I intend to uplift it in a week when I return.

Comment 37

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

Comment on attachment 8544336 [details] [diff] [review]
(Part 1) - Make image decoders hold a strong reference to their image

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 38

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

Pushed to Aurora:

https://hg.mozilla.org/releases/mozilla-aurora/pushloghtml?changeset=21a292304e03

Comment 39

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

Comment on attachment 8544336 [details] [diff] [review]
(Part 1) - Make image decoders hold a strong reference to their image

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

Comment 40

[Benoit Girard (:BenWa)]

[Tracking Requested - why for this release]:

Bug 1128138 points to this also impacting beta. I'm not sure if we want to uplift or not. I'm just nominating this bug so someone who knows this better can make this call taking into consideration bug 1128138.

Comment 42

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

(In reply to Benoit Girard (:BenWa) from comment #40)
> [Tracking Requested - why for this release]:
> 
> Bug 1128138 points to this also impacting beta. I'm not sure if we want to
> uplift or not. I'm just nominating this bug so someone who knows this better
> can make this call taking into consideration bug 1128138.

This got uplifted to 37, which is beta now. It's too late for it to get into 36, alas.