Bug 28832 – <img>.complete should return false for invalid images

• Status: RESOLVED
• Resolution: CONFIGURATION CHANGED
• Component: DOM

[Gavin Sharp]

This is the same bug as https://bugzilla.mozilla.org/show_bug.cgi?id=513541 .

Quoting http://lists.whatwg.org/pipermail/whatwg-whatwg.org/2009-August/022452.html :

> The current HTML 5 draft says:
> 
> "The DOM attribute complete must return true if the user agent has 
> downloaded the image specified in the src attribute, and it is a valid 
> image, and false otherwise."

> The "and it is a valid image" part seems to contradict the current 
> behavior of Safari 3.0.2

(I've confirmed that this is still the case with WebKit nightlies.)

And from Hixie:

> My findings match yours. I have left the spec as is, for compatibility 
> with IE, and because it seems the most logical.

[Gavin Sharp]

Created attachment 38785 [details]
testcase

Expected: Image.complete is false
Actual (WebKit trunk): Image.complete is true

This testcase also tests error events - I'll file a separate bug on that.

[Gavin Sharp]

(In reply to comment #1)
> This testcase also tests error events - I'll file a separate bug on that.

Actually, I was misremebering the issue. I was referring to this:

> The fact that Safari fires an error event but still returns "true" for 
> <img>.complete seems like a bug.

But that will be addressed by a fix for this bug.

[Alexey Proskuryakov]

Marking as security for now.

Isn't it information disclosure to report whether an arbitrary URL has a valid image? Perhaps we should stop firing the error event instead.

[David Kilzer (:ddkilzer)]

<rdar://problem/7187383>

[Gavin Sharp]

I don't think this bug is a security problem. It's possible to detect the presence of remote images with or without this change, by observing their effect on layout.

See:
http://lists.whatwg.org/pipermail/whatwg-whatwg.org/2009-August/022476.html
http://lists.whatwg.org/pipermail/whatwg-whatwg.org/2009-August/022481.html

[Gavin Sharp]

Also:
http://lists.whatwg.org/pipermail/whatwg-whatwg.org/2009-September/022589.html

[Alexey Proskuryakov]

OK

[Jussi Kukkonen (jku)]

I tried fixing this (was probably being naive) to fix CanvasRenderingContext2D.createPattern(). Things quickly turned difficult,  without help I probably won't get this done.

For reference for anyone else, currently the spec says this:
complete must return true if any of the following conditions is true:
   * The src attribute is omitted.
   * The src attribute's value is the empty string.
   * The final task that is queued by the networking task source once the 
     resource has been fetched has been queued, but has not yet been run,
     and the img element is not in the broken state.
   * The img element is completely available. 

We fail first three cases currently. First two are easy fixes and fixes don't seem to break anything but the rest seems difficult... My first idea was to do this:
    Image* imageForRenderer = cachedImage()->imageForRenderer(renderer());
    return imageForRenderer && imageForRenderer->decodedSize();

This will fail at least for "visibility:hidden" images: I assume those aren't decoded until they're shown so decodedSize isn't what I want...

[Jussi Kukkonen (jku)]

Created attachment 159380 [details]
Patch

[Jussi Kukkonen (jku)]

Comment on attachment 159380 [details]
Patch

Removing r?, this is not complete. Patch is still a work in progress, but if I could get a confirmation from someone more experienced (that this is a good direction to go and has a chance of being accepted) that would be awesome...

The patch does this:
 - Return true if no src or it is empty string
 - Return false if image is not loaded yet or is not decodable
 - Otherwise return true (image is loaded and decoded _or_
   image is loaded but no decode has been attempted)

The gotcha is that the completeness state now changes more over the lifetime of the image. So the worst case scenario would be an image that is first assigned a source url with a borken image and is then set visible.
1. image has no src -> complete=true
2. script sets src, but is not fetched yet -> complete=false
3. image is loaded (but is still not visible) -> complete=true
4. script sets image visible, decoding fails ->
 complete = false

Some things I already know to be missing:
* missing support for some graphics ports
* tests need to be updated

[Build Bot]

Comment on attachment 159380 [details]
Patch

Attachment 159380 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/13536655

[Jussi Kukkonen (jku)]

(In reply to comment #10)
> * tests need to be updated

Outlining this a bit: I think there's no specific img.complete test but several canvas tests do test it with broken or missing images. These have two basic problems:

1. the broken image is marked visibility:hidden, but the test expects complete to be false. I'm not smart enough to say if that is according to spec, but I think that we cannot expect that result in practice: if the image is invisible we do not want to decode it, so we can't know it's broken. I would suggest for testing broken images we set them viisble

2. the tests expect the broken image to be incomplete immediately. This may be a valid expectation, but does not currently happen. This fails in philip tests:
    var img = document.getElementById('broken.png'); 
    _assertSame(img.complete, false, "img.complete", "false");
because img.complete is true for a very short time, probably until the decoder has decided that the image can't be decoded.

[Build Bot]

Comment on attachment 159380 [details]
Patch

Attachment 159380 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/13538623

[Jussi Kukkonen (jku)]

Created attachment 159395 [details]
test for img.complete

This test shows values for img.complete for various images for 0.5 seconds. The results currently look like this:

---
* Testing decodable-visible
  image is complete? false
  image is complete? true
  image is complete? true
  image is complete? true
  image is complete? true

* Testing decodable-hidden
  image is complete? true
  image is complete? true
  image is complete? true
  image is complete? true
  image is complete? true

* Testing broken-visible
  image is complete? false
  image is complete? false
  image is complete? false
  image is complete? false
  image is complete? false

* Testing broken-hidden
  image is complete? true
  image is complete? true
  image is complete? true
  image is complete? true
  image is complete? true
---

Things to note:
1. first result for decodable & visible is "not complete". Would be nice to get that fixed
2. broken and hidden images return "complete", this is not optimal but I think the best we can do.

[Jussi Kukkonen (jku)]

> Things to note:
> 1. first result for decodable & visible is "not complete". Would be nice to get that fixed

Ah, actually it is working as intended: the first image just takes a longer to load and the imageLoader is still sayin the image is not yet complete at that time. So the test should not expect the image to be complete immediately

[Jussi Kukkonen (jku)]

Created attachment 159416 [details]
test for img.complete

Simpler test that should work non-locally as well

[Jussi Kukkonen (jku)]

Created attachment 159423 [details]
Patch

[WebKit Review Bot]

Comment on attachment 159423 [details]
Patch

Attachment 159423 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/13547007

New failing tests:
canvas/philip/tests/2d.pattern.image.broken.html
canvas/philip/tests/2d.drawImage.broken.html

[WebKit Review Bot]

Created attachment 159437 [details]
Archive of layout-test-results from gce-cr-linux-01

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: gce-cr-linux-01  Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'>  Platform: Linux-2.6.39-gcg-201203291735-x86_64-with-Ubuntu-10.04-lucid

[Jussi Kukkonen (jku)]

Created attachment 159631 [details]
Patch

Add changelogs. Setting r? flag now, I could really use some comments on this now. I've not added support for graphics ports I can't test but they should keep working as they have been so far...

[Kenneth Rohde Christiansen]

Comment on attachment 159631 [details]
Patch

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

> Source/WebCore/ChangeLog:8
> +        HTMLImageElement.complete should follow spec more closely

shouldn't it not just follow it? :-)

> Source/WebCore/ChangeLog:9
> +        - Return true if no src or it is empty string

no src is present? or if it is equal to an empty string

Could be written better

> Source/WebCore/platform/graphics/BitmapImage.h:137
> +    virtual bool decodeFailed() const { return m_source.decodeFailed(); }

why not decodingFailed? or decodeDidFail(). I am not sure what is most correct.

> Source/WebCore/platform/graphics/cg/ImageSourceCG.cpp:239
> +bool ImageSource::decodeFailed() const
> +{
> +    // TODO: implement this (with kCGImageStatus?)
> +    return false;
> +}

file bug report then, add link

> LayoutTests/ChangeLog:6
> +        Tests that deal with img.complete with broken images are

I dont understand this line

> LayoutTests/canvas/philip/tests/2d.drawImage.broken.html:25
> +deferTest();
> +
> +// wait a moment so imageloader has time to load the image
> +setTimeout(wrapFunction(function () {
> +  var img = document.getElementById('broken.png');
> +  _assertSame(img.complete, false, "img.complete", "false");
> +  ctx.drawImage(img, 0, 0);
> +}), 100);

This often leads to flaky tests. How do you know 100 is enough? anyway to avoid depending on time?

[Dominik Röttsches (drott)]

(In reply to comment #21)
> (From update of attachment 159631 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=159631&action=review

> > LayoutTests/canvas/philip/tests/2d.drawImage.broken.html:25
> > +deferTest();
> > +
> > +// wait a moment so imageloader has time to load the image
> > +setTimeout(wrapFunction(function () {
> > +  var img = document.getElementById('broken.png');
> > +  _assertSame(img.complete, false, "img.complete", "false");
> > +  ctx.drawImage(img, 0, 0);
> > +}), 100);
> 
> This often leads to flaky tests. How do you know 100 is enough? anyway to avoid depending on time?

Maybe encode the image as a data-URI and place it inline, then put a script tag below that triggers starting the test? (Since image progress events as proposed in bug 76102 haven't been accepted yet.) I'd suggest to file a W3C Test Cases bug if you plan to change the philip test case here.

[Jussi Kukkonen (jku)]

Thanks for the comments, much appreciated.

(In reply to comment #21)
> > Source/WebCore/ChangeLog:9
> > +        - Return true if no src or it is empty string
> 
> no src is present? or if it is equal to an empty string
> 
> Could be written better


Agreed, will improve.

> > Source/WebCore/platform/graphics/BitmapImage.h:137
> > +    virtual bool decodeFailed() const { return m_source.decodeFailed(); }
> 
> why not decodingFailed? or decodeDidFail(). I am not sure what is most correct.

Agree that decodeFailed does not sound entirely correct, I copied it from the underlying classes that actually implement the functionality...

> > Source/WebCore/platform/graphics/cg/ImageSourceCG.cpp:239
> > +bool ImageSource::decodeFailed() const
> > +{
> > +    // TODO: implement this (with kCGImageStatus?)
> > +    return false;
> > +}
> 
> file bug report then, add link

Ok

> > LayoutTests/ChangeLog:6
> > +        Tests that deal with img.complete with broken images are
> 
> I dont understand this line


I don't blame you... Actually "Tests that rely on img.complete are ..." would be correct -- it's just that we notice the problem using broken images.


> > LayoutTests/canvas/philip/tests/2d.drawImage.broken.html:25
> > +deferTest();
> > +
> > +// wait a moment so imageloader has time to load the image
> > +setTimeout(wrapFunction(function () {
> > +  var img = document.getElementById('broken.png');
> > +  _assertSame(img.complete, false, "img.complete", "false");
> > +  ctx.drawImage(img, 0, 0);
> > +}), 100);
> 
> This often leads to flaky tests. How do you know 100 is enough? anyway to avoid depending on time?

Hmm, I'll think about this.


(In reply to comment #22)
> Maybe encode the image as a data-URI and place it inline, then put a script tag below that triggers starting the test? (Since image progress events as proposed in bug 76102 haven't been accepted yet.) I'd suggest to file a W3C Test Cases bug if you plan to change the philip test case here.

The test case bug sounds like a good idea, I'll do this.

[Jussi Kukkonen (jku)]

Created attachment 161840 [details]
Patch

test mac implementation (no review please, I'll split this patch still)

[Jussi Kukkonen (jku)]

Created attachment 161841 [details]
Patch

test mac implementation, take 2 (no review, I'll split this patch still)

[WebKit Review Bot]

Comment on attachment 161841 [details]
Patch

Attachment 161841 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/13724819

New failing tests:
canvas/philip/tests/2d.drawImage.broken.html

[Jussi Kukkonen (jku)]

Created attachment 161876 [details]
Patch

Review would be appreciated, but note that this is now blocked by bug 95440. This version should fix all Kenneths comments and includes the mac implementation for ImageSource::decodingFailed().

[Darin Adler]

Comment on attachment 161876 [details]
Patch

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

This patch is good, but I’d like to see the changes I mentioned in the comments. Not sure if Jussi Kukkonen is still around to finish working on this or not.

> Source/WebCore/html/HTMLImageElement.cpp:375
> +    if (!hasAttribute(srcAttr) || getAttribute(srcAttr).isEmpty())
> +        return true;

The check of hasAttribute here is unnecessary; the isEmpty check will cover that case too. This should use fastGetAttribute.

> Source/WebCore/html/HTMLImageElement.cpp:380
> +    return (!cachedImage()->imageForRenderer(renderer())->decodingFailed());

There are extra parentheses here that should be removed.

> Source/WebCore/platform/graphics/BitmapImage.h:140
> +    virtual bool decodingFailed() const { return m_source.decodingFailed(); }

This should be marked OVERRIDE. Also, the override should be private rather than public since we’d prefer that people not call this virtual function if they have a BitmapImage pointer. Or we can use FINAL to achieve the same thing.

> Source/WebCore/platform/graphics/Image.h:117
> +    virtual bool decodingFailed() const { return false; }

The function name here is ambiguous. It could be a function to call to tell the image that decoding failed or to ask the image is decoding failed. That’s way we normally try to name boolean functions in ways that complete sentences like “image <x>”. In this case I would probably say suggest the name hasFailedToDecode.

> Source/WebCore/platform/graphics/ImageSource.cpp:135
> +    if (!m_decoder)
> +        return false;
> +
> +    return m_decoder->failed();

I think this reads better as:

    return m_decoder && m_decoder->failed().

> Source/WebCore/platform/graphics/ImageSource.h:135
> +    bool decodingFailed() const;

As above, I suggest naming this hasFailedToDecode.

> Source/WebCore/platform/graphics/cg/ImageSourceCG.cpp:241
> +    return (status == kCGImageStatusUnexpectedEOF
> +            || status == kCGImageStatusInvalidData
> +            || status == kCGImageStatusUnknownType);

Please remove the unneeded parentheses and use normal WebKit coding style indenting, 4 spaces, not lining up with open parentheses.

[Jussi Kukkonen (jku)]

(In reply to comment #28)
> (From update of attachment 161876 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=161876&action=review
> 
> This patch is good, but I’d like to see the changes I mentioned in the comments. Not sure if Jussi Kukkonen is still around to finish working on this or not.

I don't have a git clone available right now but I promise to take a look next week: I feel I have a special connection to this bug :)

Thanks for review. If you want to take a look at the test modifications in bug 95440 as well, that would be grand as jamesr and I just didn't get on the same page about them.

[Ahmad Saleem]

Created attachment 459862 [details]
Safari 15.5 behaves same as other browsers

Based on test case from Comment 16, all browsers listed below works same (as shown in the attachment).

Safari 15.5 - all true across four tests
Chrome Canary 104 - same as above
Firefox Nightly 102 - same as above

But from original test case, Safari and Chrome behavior is aligned and it shows "image.complete is true" while Firefox shows "false". [Versions are same as above listed]

Just wanted to share the updated results based on latest browsers. Thanks!

[Alexey Proskuryakov]

The spec now says:

The IDL attribute complete must return true if any of the following conditions is true:

* Both the src attribute and the srcset attribute are omitted.
* The srcset attribute is omitted and the src attribute's value is the empty string.
* The img element's current request's state is completely available and its pending request is null.
* The img element's current request's state is broken and its pending request is null.