Bug 28832 - <img>.complete should return false for invalid images
Summary: <img>.complete should return false for invalid images
Status: NEW
Alias: None
Product: WebKit
Classification: Unclassified
Component: DOM (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Jussi Kukkonen (jku)
URL:
Keywords: InRadar
Depends on: 95440
Blocks: 46506
  Show dependency treegraph
 
Reported: 2009-08-30 00:54 PDT by Gavin Sharp
Modified: 2013-05-10 00:08 PDT (History)
10 users (show)

See Also:


Attachments
testcase (544 bytes, text/html)
2009-08-30 00:55 PDT, Gavin Sharp
no flags Details
Patch (5.15 KB, patch)
2012-08-20 02:35 PDT, Jussi Kukkonen (jku)
no flags Details | Formatted Diff | Diff
test for img.complete (943 bytes, text/html)
2012-08-20 04:47 PDT, Jussi Kukkonen (jku)
no flags Details
test for img.complete (1.40 KB, text/html)
2012-08-20 06:55 PDT, Jussi Kukkonen (jku)
no flags Details
Patch (13.48 KB, patch)
2012-08-20 07:40 PDT, Jussi Kukkonen (jku)
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from gce-cr-linux-01 (632.17 KB, application/zip)
2012-08-20 08:42 PDT, WebKit Review Bot
no flags Details
Patch (13.62 KB, patch)
2012-08-21 00:33 PDT, Jussi Kukkonen (jku)
no flags Details | Formatted Diff | Diff
Patch (14.95 KB, patch)
2012-09-02 04:39 PDT, Jussi Kukkonen (jku)
no flags Details | Formatted Diff | Diff
Patch (16.16 KB, patch)
2012-09-02 05:20 PDT, Jussi Kukkonen (jku)
no flags Details | Formatted Diff | Diff
Patch (10.95 KB, patch)
2012-09-03 01:09 PDT, Jussi Kukkonen (jku)
kenneth: review+
darin: commit-queue-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Gavin Sharp 2009-08-30 00:54:06 PDT
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.
Comment 1 Gavin Sharp 2009-08-30 00:55:06 PDT
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.
Comment 2 Gavin Sharp 2009-08-30 00:58:29 PDT
(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.
Comment 3 Alexey Proskuryakov 2009-09-01 10:44:33 PDT
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.
Comment 4 David Kilzer (:ddkilzer) 2009-09-01 11:17:09 PDT
<rdar://problem/7187383>
Comment 5 Gavin Sharp 2009-09-01 15:26:20 PDT
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
Comment 7 Alexey Proskuryakov 2009-09-08 11:41:16 PDT
OK
Comment 8 Jussi Kukkonen (jku) 2012-08-13 05:00:24 PDT
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...
Comment 9 Jussi Kukkonen (jku) 2012-08-20 02:35:40 PDT
Created attachment 159380 [details]
Patch
Comment 10 Jussi Kukkonen (jku) 2012-08-20 02:49:01 PDT
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
Comment 11 Build Bot 2012-08-20 03:37:23 PDT
Comment on attachment 159380 [details]
Patch

Attachment 159380 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/13536655
Comment 12 Jussi Kukkonen (jku) 2012-08-20 03:51:24 PDT
(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.
Comment 13 Build Bot 2012-08-20 04:12:22 PDT
Comment on attachment 159380 [details]
Patch

Attachment 159380 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/13538623
Comment 14 Jussi Kukkonen (jku) 2012-08-20 04:47:45 PDT
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.
Comment 15 Jussi Kukkonen (jku) 2012-08-20 05:02:42 PDT
> 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
Comment 16 Jussi Kukkonen (jku) 2012-08-20 06:55:10 PDT
Created attachment 159416 [details]
test for img.complete

Simpler test that should work non-locally as well
Comment 17 Jussi Kukkonen (jku) 2012-08-20 07:40:20 PDT
Created attachment 159423 [details]
Patch
Comment 18 WebKit Review Bot 2012-08-20 08:42:39 PDT
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
Comment 19 WebKit Review Bot 2012-08-20 08:42:45 PDT
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
Comment 20 Jussi Kukkonen (jku) 2012-08-21 00:33:53 PDT
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...
Comment 21 Kenneth Rohde Christiansen 2012-08-28 04:00:03 PDT
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?
Comment 22 Dominik Röttsches (drott) 2012-08-28 04:20:14 PDT
(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.
Comment 23 Jussi Kukkonen (jku) 2012-08-29 04:28:51 PDT
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.
Comment 24 Jussi Kukkonen (jku) 2012-09-02 04:39:44 PDT
Created attachment 161840 [details]
Patch

test mac implementation (no review please, I'll split this patch still)
Comment 25 Jussi Kukkonen (jku) 2012-09-02 05:20:56 PDT
Created attachment 161841 [details]
Patch

test mac implementation, take 2 (no review, I'll split this patch still)
Comment 26 WebKit Review Bot 2012-09-02 08:29:52 PDT
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
Comment 27 Jussi Kukkonen (jku) 2012-09-03 01:09:12 PDT
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().
Comment 28 Darin Adler 2013-05-08 09:24:18 PDT
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.
Comment 29 Jussi Kukkonen (jku) 2013-05-10 00:08:17 PDT
(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.