Bug 95440 – testing img.complete without waiting for decoding is wrong

• Component: Tools / Tests
• Status: NEW

[Jussi Kukkonen (jku)]

I was trying to fix HTMLImageElement.complete (so it would return false when the image was not decodable), and realised the tests that use it are also wrong.

img.complete may (and should) change while the script is running. If we want to check the "final" value we can't do it when script execution starts: the image may not be loaded/decoded yet at that time. We should check img.complete only after decoding should be complete.

Fixing this reliably wasn't as easy as I hoped:
 * it seems even for inlined uri-data images, the decoding is still done in parallel with script execution
 * same goes for img.onload: checking for img.complete from the onload handler gives flaky results
...at least these were my results with the WIP patch in bug 28832

A timeout is an inherently flaky solution but still the best I could think of... with small images (like the testrunner ones) the decoding seems to take only milliseconds. I'll upload a patch with a timeout but I'd be happy with a better solution if anyone has one.

[Jussi Kukkonen (jku)]

https://www.w3.org/Bugs/Public/show_bug.cgi?id=18742

[Jussi Kukkonen (jku)]

Created attachment 161873 [details]
Patch

Review would be appreciated.

[Kenneth Rohde Christiansen]

Comment on attachment 161873 [details]
Patch

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

> LayoutTests/ChangeLog:4
> +        Give image decoder a moment before testing img.complete
> +        https://bugs.webkit.org/show_bug.cgi?id=95440

Make it clear that this is about the tests.

> LayoutTests/ChangeLog:15
> +
> +        Checking for img.complete before the decoding is finished is
> +        incorrect. We don't have a good way to know when it is finished,
> +        so use a timeout.
> +
> +        Also fix the expected values of the tests, and remove "resource" class
> +        from the image ("resource" is invisible, meaning it's never actually
> +        decoded, see https://bugs.webkit.org/show_bug.cgi?id=28832)
> +

IT doesn't sounds like you are only talking about the tests here. You should make that more clear.

[Jussi Kukkonen (jku)]

Created attachment 163335 [details]
Patch

[Jussi Kukkonen (jku)]

(In reply to comment #3)
> 
> Make it clear that this is about the tests.
> 

Thanks. I tried to clarify the changelog, HTH.

[Kenneth Rohde Christiansen]

Comment on attachment 163335 [details]
Patch

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

> LayoutTests/ChangeLog:19
> +        Remove extra line from expected results.

You seem to be adding that instead

> LayoutTests/ChangeLog:24
> +        * canvas/philip/tests/2d.drawImage.broken-expected.txt:
> +        * canvas/philip/tests/2d.drawImage.broken.html:
> +        * canvas/philip/tests/2d.pattern.image.broken-expected.txt:
> +        * canvas/philip/tests/2d.pattern.image.broken.html:

explain why the tests were passing before? wrongly?

> LayoutTests/canvas/philip/tests/2d.drawImage.broken.html:20
> +// give decoder time to figure out the image is broken

comments start with capital and ends with a punctuation mark

> LayoutTests/canvas/philip/tests/2d.drawImage.broken.html:21
> +setTimeout(wrapFunction(function () {

Write in the changelog why this shouldnt result in flakyness

[Jussi Kukkonen (jku)]

Created attachment 163542 [details]
Patch

[James Robinson]

This doesn't make any sense to me.  img.complete should NOT change while script is running - in general nothing script-observable should change.

I don't know what you are referring to when talking about "the decoder may not have...".  The decoders in WebCore are synchronous, they can't spontaneously change state.

[James Robinson]

Comment on attachment 163542 [details]
Patch

This needs a better explanation at minimum.  It seems that this is just adding a setTimeout() to tests for no reason in its current incarnation.

[Jussi Kukkonen (jku)]

(In reply to comment #8)
> This doesn't make any sense to me.  img.complete should NOT change while script is running - in general nothing script-observable should change.

The spec clearly disagrees, but I realise that doesn't mean implementation needs to... the comment about synchronous decoders is interesting -- I'll look closer at what's really happening then and report back here

[Jussi Kukkonen (jku)]

Created attachment 163570 [details]
test for img.complete behaviour

Here's a test for img.complete behaviour, hopefully works straight from bugzilla.

Results on chromium (not webkit master but a release):
---
Log:

* Testing img.complete on script startup
  decodable-visible: image.complete == false
  decodable-hidden: image.complete == false
  broken-visible: image.complete == false
  broken-hidden: image.complete == false

* Testing img.complete after 5 secs...
  decodable-visible: image.complete == true
  decodable-hidden: image.complete == true
  broken-visible: image.complete == true
  broken-hidden: image.complete == true
---

So the results do change while script is running -- because imageloader is still doing it's job (so my comment in the test is not very accurate -- the timeout is waiting for decoding results, but the images probably aren't even loaded at that point).
The end results are also wrong, which is what bug 28832 is about: broken images should not be "complete".


EFL MiniBrowser with the 28832 patch gives:
---
Log:

* Testing img.complete on script startup
  decodable-visible: image.complete == false
  decodable-hidden: image.complete == false
  broken-visible: image.complete == false
  broken-hidden: image.complete == false

* Testing img.complete after 5 secs...
  decodable-visible: image.complete == true
  decodable-hidden: image.complete == true
  broken-visible: image.complete == false
  broken-hidden: image.complete == true
---

Broken visible images are now "not complete". It's still saying broken hidden image is "complete", I would argue that's according to spec and we definitely wouldn't want to start decoding just for this anyway...

[Jussi Kukkonen (jku)]

I was supposed to add info on what really happens after image loader finishes but I don't have that yet...

The test result (with patch from 28832) is that on the onload handler for a broken visible image, img.complete _is_ true. This changes to false (the correct value) just a bit later. My original assumption was that this was because in onload the image had been loaded (fulfilling the spec requirements for "complete", if I read it correctly) but we didn't yet know it is not decodable.

I may have assumed too much there or my patch could be wrong so I'll try to document what _really_ happens there -- James, if you have knowledge of this please share.

[Jussi Kukkonen (jku)]

Looking at the sequence of events more carefully, this is what I think happens:
* imageloader starts working
* script starts (image.complete is always false because image is not loaded)
* decoder starts getting data
* onload event may happen here (valid images are now complete, but broken image 
  may be incorrectly complete because decoding has not failed yet)
* decoder may set failed=True here (but may also be earlier)

It looks to me like my original reasoning is valid. I'll be happy if someone shows where I'm wrong though.

The flakiness could be minimized by waiting for the onload-events but that's just not enough, a timeout is still needed because decoder might might decide it has failed later than onload.