Bug 28832

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.

(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.

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.

<rdar://problem/7187383>

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

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

OK

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...

Created attachment 159380 [details] Patch

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 on attachment 159380 [details] Patch Attachment 159380 [details] did not pass win-ews (win): Output: http://queues.webkit.org/results/13536655

(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 on attachment 159380 [details] Patch Attachment 159380 [details] did not pass mac-ews (mac): Output: http://queues.webkit.org/results/13538623

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.

> 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

Created attachment 159416 [details] test for img.complete Simpler test that should work non-locally as well

Created attachment 159423 [details] Patch

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

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

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 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?

(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.

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.

Created attachment 161840 [details] Patch test mac implementation (no review please, I'll split this patch still)

Created attachment 161841 [details] Patch test mac implementation, take 2 (no review, I'll split this patch still)

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

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 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.

(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.

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!

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.