190561 - [FIXr]Image.complete should be set to true on error

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect, P2)

Description

[Mitchell Stoltz (not reading bugmail)]

When an IMG tag points to a nonexistent image, its |complete| property never
gets set to true. According to the Rhino Book, the Image.complete property
should be set to true on error. Fixing this is one step in preventing the
detection of remote image existence, a minor security issue.

Comment 1

[Boris Zbarsky [:bzbarsky]]

DOM bug.

Comment 2

[Johnny Stenback (:jst)]

Mass-reassigning bugs to [e-mail]

Comment 3

[Boris Zbarsky [:bzbarsky]]

Attachment: error == complete, presto.

Comment 4

[Boris Zbarsky [:bzbarsky]]

taking

Comment 5

[Johnny Stenback (:jst)]

Comment on attachment 121391 [details] [diff] [review]
error == complete, presto.

sr=jst

Comment 6

[Mitchell Stoltz (not reading bugmail)]

Comment on attachment 121391 [details] [diff] [review]
error == complete, presto.

r=mstoltz. Thanks, Boris.

Comment 7

[Boris Zbarsky [:bzbarsky]]

Mitch, let me know whether you want this for 1.4final (or just set the approval
request flag yourself).

Comment 8

[Mitchell Stoltz (not reading bugmail)]

Yes, I would, if you can. You've got the reviews, let's get this checked in.

Comment 9

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 121391 [details] [diff] [review]
error == complete, presto.

Requesting approval for 1.4b per Mitch's request.

Comment 10

[(not reading, please use [e-mail] instead)]

Comment on attachment 121391 [details] [diff] [review]
error == complete, presto.

a=sspitzer

Comment 11

[Boris Zbarsky [:bzbarsky]]

Fixed.

Comment 12

[Mitra Ardron]

I'm just noting that current behavior of img.complete is different from in Chrome. When src is empty it appears to mark the image as complete immediately, which is not the expected behavior if the intention is to set 'src' from some other code.

This caused a nasty incompatibility for us, since the resizing code fired based on complete=true and left us with a 0x0 box to put the image in !

Comment 13

[Boris Zbarsky [:bzbarsky]]

Mitra, I don't know what your exact code looks like, but assuming it's something like this:

  var img = new Image();
  alert(img.complete);

then I see that show true in both Firefox and Chrome, and that's the right behavior per spec: see the first bullet point at https://html.spec.whatwg.org/multipage/embedded-content.html#dom-img-complete. It's worth filing a separate bug on whatever your situation is and ccing me so I can take a look, though it sounds like Firefox is generally following the spec here, given the description in comment 12.

Comment 14

[Mitra Ardron]

Its hard to pin down exactly the difference - in this case its images being created by React, then src being updated (via React's setState on the parent) and a componentDidUpdate method being triggered that behaves differently for Firefox and Chrome because in FF img.complete is true and in Chrome its false. This might be a timing issue. I saw a similar problem with the react-media library where in FF setting src didn't get the image to resize - but I think that code has changed in more recent versions.

I've not filed a bug because
a) there seems some debate as to whether complete should be set true, so it might be that FF is correct, and Chrome seems to get to true eventually anyway
b) I haven't been able to replicate with simpler tests so chance of getting someone to look at it is low.

So I was just trying to flag here (which is what came up for me on a search) that there do appear to be differences between FF and Chrome in how they handle it.

I currently have an example page: https://dweb.archive.org/details/mbid-ac2b87af-2774-4575-a72a-db31c8865264?transport=HTTP which fails in FF and works in Chrome because of this exact problem (another method decides the page is complete before its src is set), but I plan in the next few days to push a workaround with browser-specific code :-(

Comment 15

[Boris Zbarsky [:bzbarsky]]

The main reason to file a separate bug for this discussion is that it's not really related to this bug and lots of people are getting mail about this who probably don't care. ;)

I am explicitly offering to take a look at non-simple testcases, though obviously a simple one would make it easier to figure out what's going on.

I'll take a look at the example page tomorrow morning; thank you for the link.

Comment 16

[Boris Zbarsky [:bzbarsky]]

For what it's worth, I looked a bit at the testcase. It's complex, yes, but even worse from a debugging point of view all the code is minified and obfuscated. A testcase without minified code would be extremely helpful. Again, please file a separate bug on this...

Comment 17

[Mitra Ardron]

Sorry - I forgot that code is minimized, and I can't regenerate it because the repo (github.com/interntarchive/dweb-archive) has moved on and includes a workaround for this browser difference in v0.1.92 (I'd been holding off pushing it live to give you a chance to look at it, but that doesn't sound much use on minimized code). I'm not sure what to post in a separate bug since I don't have STR, just an example failure case which is going to disappear any day now :-(

Comment 18

[Boris Zbarsky [:bzbarsky]]

Is it possible to regenerate from an older repo revision? It would be really helpful to pin this down, because while you're willing to work around it others probably are not and that will cause us web compat issues.

If it's really not possible to create something with unminimized code, I will try debugging what I have to work with, but I likely can't get to it until Tuesday, because of the holiday on Monday. Is it possible to keep the example up until then?

Comment 19

[Boris Zbarsky [:bzbarsky]]

I filed bug 1577991 for further discussion about this. That lets me pull in other people who might be able to help debug, etc...