Closed Bug 1162282 Opened 9 years ago Closed 9 years ago

When DrawImage is called on a corrupt image with an available size, it should not throw

Categories

(Core :: Graphics: Canvas2D, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla40
Tracking Status
firefox40 --- fixed

People

(Reporter: seth, Assigned: seth)

References

Details

Attachments

(1 file)

According to the spec (https://html.spec.whatwg.org/multipage/scripting.html#dom-context-2d-drawimage), the first step drawImage must take is as follows:

> Check the usability of the image argument. If this returns aborted, then an
> exception has been thrown and the method doesn't return anything; abort these
> steps. If it returns bad, then abort these steps without drawing anything.
> Otherwise it returns good; continue with these steps.

The algorithm for checking the usability of the image argument states:

> If the image argument is an HTMLImageElement object that is in the broken
> state, then throw an InvalidStateError exception, return aborted, and abort
> these steps.
> If the image argument is an HTMLImageElement object that is not fully
> decodable, or if the image argument is an HTMLVideoElement object whose
> readyState attribute is either HAVE_NOTHING or HAVE_METADATA, then return bad
> and abort these steps.

So the crucial point here is that we only throw when the image is in the broken state. When is it in the broken state? Well, the broken state is defined this way:

> The user agent has obtained all of the image data that it can, but it cannot
> even decode the image enough to get the image dimensions (e.g. the image is
> corrupted, or the format is not supported, or no data could be obtained).

Crucially, if we've decoded the image enough to get the image dimensions, we're not in the broken state.

This is nothing new; this is exactly how things work in other parts of the browser. But our implementation of canvas.drawImage doesn't perform this check correctly, and as a result, we're currently not in compliance with the spec.

There's a test that checks for this issue:

testing/web-platform/tests/2dcontext/drawing-images-to-the-canvas/2d.drawImage.broken.html

We currently pass this test through a quirk of timing, and not because our implementation is actually correct. Bug 1155332 is going to change that timing, which will break the test unless we fix our implementation. So in this bug, we'll do just that.
Here's the patch. We determine whether the image has a valid size in
SurfaceFromElement and perform the appropriate check before throwing in
DrawImage.
Attachment #8602371 - Flags: review?(gwright)
Comment on attachment 8602371 [details] [diff] [review]
When canvas.drawImage is called on a corrupt image that's not in the broken state, don't throw

Review of attachment 8602371 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/canvas/CanvasRenderingContext2D.cpp
@@ +4320,5 @@
> +      // The spec says to silently do nothing in the following cases:
> +      //   - The element is still loading.
> +      //   - The image is bad, but it's not in the broken state (i.e., we could
> +      //     decode the headers and get the size).
> +      if (!res.mIsStillLoading && !res.mHasSize) {

Is having the size information a robust indicator of "brokenness"?
(In reply to Seth Fowler [:seth] from comment #0)
> Crucially, if we've decoded the image enough to get the image dimensions,
> we're not in the broken state.

I guess what I'm asking is, is it possible to get corrupted image data where the dimensions aren't corrupted, but the rest of it is, and so we're in a broken state?
(In reply to George Wright (:gw280) from comment #4)
> I guess what I'm asking is, is it possible to get corrupted image data where
> the dimensions aren't corrupted, but the rest of it is, and so we're in a
> broken state?

Essentially if we did a successful decode of the headers - which is what STATUS_SIZE_AVAILABLE indicates - then we're not in the broken state. The image may be corrupt, which may lead to us not drawing anything when told to draw it, but it's not broken and we shouldn't throw in that case.

It's counter-intuitive, I know. =\ But if it didn't work this way, it'd have performance consequences, because it'd force us to fully decode images to decode whether they're broken or not, which is something we don't have to do today.
Comment on attachment 8602371 [details] [diff] [review]
When canvas.drawImage is called on a corrupt image that's not in the broken state, don't throw

Review of attachment 8602371 [details] [diff] [review]:
-----------------------------------------------------------------

I guess that makes sense then.
Attachment #8602371 - Flags: review?(gwright) → review+
Thanks again for the quick review on this, George!

No try job in this bug but the newest try job in bug 1155332 includes this patch.
https://hg.mozilla.org/mozilla-central/rev/b6db7c0824d6
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla40
I duped bug 1163102 against this, because another situation where imgIContainer::GetFrame could fail is if memory is so low that we cannot decode the image.
You need to log in before you can comment on or make changes to this bug.