Closed Bug 1316629 Opened 3 years ago Closed 3 years ago

dom/html/test/test_img_complete.html fails to run >1 time in a browser session

Categories

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

defect

Tracking

()

RESOLVED FIXED
mozilla53
Tracking Status
firefox52 --- fixed
firefox53 --- fixed

People

(Reporter: jmaher, Assigned: jmaher)

References

(Blocks 1 open bug)

Details

Attachments

(1 file, 1 obsolete file)

in experimenting with a possible new test verification job, I examined existing tests and realized that many do not run >1 time in the same browser session:
./mach mochitest dom/html/test/test_img_complete.html --repeat 1

I get this error at the end of my mach command:
26 INFO TEST-UNEXPECTED-FAIL | dom/html/test/test_img_complete.html | Now that we're loading we should no longer be complete


doing something like this fixes the issue:
diff --git a/dom/html/test/test_img_complete.html b/dom/html/test/test_img_complete.html
--- a/dom/html/test/test_img_complete.html
+++ b/dom/html/test/test_img_complete.html
@@ -19,17 +19,18 @@ https://bugzilla.mozilla.org/show_bug.cg
 <div id="content" style="display: none">

 </div>
 <pre id="test">
   <script type="application/javascript">
     addLoadEvent(function() {
       var img = document.querySelector("img");
       ok(img.complete, "By onload, image should have loaded");
-      img.src = "image.png?cachebust";
+      var salt = Math.random();
+      img.src = "image.png?cachebust" + salt;
       ok(!img.complete, "Now that we're loading we should no longer be complete");
       img.onload = function() {
         ok(img.complete, "The new thing should have loaded.");
         SimpleTest.finish();
       }
     });
   </script>
 </pre>



the problem is that seems super hacky- I would rather at the end of the test remove the item from the cache or find a way to ensure we always load a fresh image.png.

:bz, I see you wrote this test originally last year, any thoughts on what you would prefer we do to solve this bug?
Flags: needinfo?(bzbarsky)
The right thing here is:

  img.src = `image.png?${Math.random()}`; // Make sure we miss the sync image cache.

and not overthinking it past that.

This test should really move into a web platform test, if there isn't one already, where this approach will work but "remove it from the cache" certainly will not.
Flags: needinfo?(bzbarsky)
Oh, and I can move this to a web platform test myself if you'd rather not spend time on it.  Just let me know.
it looks like there is an existing wpt test that covers this:
https://dxr.mozilla.org/mozilla-central/source/testing/web-platform/tests/html/semantics/embedded-content/the-img-element/img.complete.html?q=img.complete.html&redirect_type=direct

:bz, can you verify this exists and I will happily delete this mochitest one.
That test doesn't test all the same things as our existing test does.  In particular, it doesn't check that if you start with an already-loaded image that is complete then setting a new src will immediately set complete to false....
Priority: -- → P3
Attached file migrate to web-platform-test (obsolete) —
ok, my first attempt at a wpt test, this is pretty much a blind copy of the existing test.  I tested this locally and will push to try if this seems good.
Assignee: nobody → jmaher
Status: NEW → ASSIGNED
Attachment #8810551 - Flags: review?(james)
You need to do an async_test here, finishing it when the async work is done....
Comment on attachment 8810551 [details]
migrate to web-platform-test

thanks :bz, I will use async_test :)
Attachment #8810551 - Flags: review?(james)
Actually you don't have to; without test() or async_test() anywhere on the page it's interpreted as a single page test. But you are calling done() at the wrong time; the test isn't complete until the second image load event fires.
I looked at the test, initially I wanted to call done() in the img.onload handler, but what if the img doesn't load, then the test times out and we fail slowly?  Possibly that is the best route for displaying an error- :jgraham, how does wpt handle timeouts if it doesn't receive a done() message?


as a note, I run the test locally with:
./mach web-platform-tests testing/web-platform/tests/html/semantics/embedded-content/the-img-element/update-src-complete.html

and moving done() to the img.onload handler works as expected
Flags: needinfo?(james)
If the image doesn't load we will indeed time out slowly; after 10s (in default settings) the harness will get status TIMEOUT which will be displayed to the user (or recorded in the test results in CI). In general I don't think you should worry too much about this case because it's pretty unlikely that we totally break image loading.
Flags: needinfo?(james)
thanks for your help so far :jgraham, I know this is a lot of back and forth, learning the small details of wpt tests.  I see :bz is still busy, so asking you for review.
Attachment #8810551 - Attachment is obsolete: true
Attachment #8810945 - Flags: review?(james)
Comment on attachment 8810945 [details] [diff] [review]
migrate to web-platform-test

Review of attachment 8810945 [details] [diff] [review]:
-----------------------------------------------------------------

::: testing/web-platform/tests/html/semantics/embedded-content/the-img-element/update-src-complete.html
@@ +15,5 @@
> +            done();
> +        }
> +    }
> +
> +    onload = function () {

fwiw I would just have written 
onload = function() {
  // body of check goes here
}

but this way works too.
Attachment #8810945 - Flags: review?(james) → review+
Pushed by jmaher@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/55fe392732ac
migrate dom/html/test/test_img_complete.html to web-platform-test. r=jgraham
https://hg.mozilla.org/mozilla-central/rev/55fe392732ac
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla53
You need to log in before you can comment on or make changes to this bug.