We got failure as followed if the fix of bug 1193394 is included: TEST-UNEXPECTED-FAIL | dom/tests/mochitest/general/test_contentViewer_overrideDPPX.html | devicePixelRatio has the original value. - got 0.5, expected 1 TEST-UNEXPECTED-FAIL | dom/tests/mochitest/general/test_contentViewer_overrideDPPX.html | fullZoom has the original value. - got 0.5, expected 1 TEST-UNEXPECTED-FAIL | dom/tests/mochitest/general/test_contentViewer_overrideDPPX.html | devicePixelRatio has the original value. - got 0.5, expected 1 TEST-UNEXPECTED-FAIL | dom/tests/mochitest/general/test_contentViewer_overrideDPPX.html | fullZoom has the original value. - got 0.5, expected 1 TEST-UNEXPECTED-FAIL | dom/tests/mochitest/general/test_contentViewer_overrideDPPX.html | devicePixelRatio has the original value. - got 0.5, expected 1 TEST-UNEXPECTED-FAIL | dom/tests/mochitest/general/test_contentViewer_overrideDPPX.html | fullZoom has the original value. - got 0.5, expected 1 TEST-UNEXPECTED-FAIL | dom/tests/mochitest/general/test_contentViewer_overrideDPPX.html | devicePixelRatio has the original value. - got 0.5, expected 1 TEST-UNEXPECTED-FAIL | dom/tests/mochitest/general/test_contentViewer_overrideDPPX.html | fullZoom has the original value. - got 0.5, expected 1 After further investigation, the problem is that we should ensure the call flow of setOverrideDPPX/setFullZoom in "test OverrideDPPX with MediaQueryList and fullZoom" [1] in correct order as followed in both new/old Promise scheduling to meet the test criteria: 1. setOverrideDPPX(dppx); 2. setFullZoom(zoom); 3. setOverrideDPPX(0); 4. setFullZoom(originalZoom) [1] https://searchfox.org/mozilla-central/rev/c633ffa4c4611f202ca11270dcddb7b29edddff8/dom/tests/mochitest/general/test_contentViewer_overrideDPPX.html#292-299

The difference of the Promise scheduling is explained in bug 1193394 comment 48. In short, a promise callback could be performed earlier at the return of outermost JS callback if possible rather than at the end of current task.

Hi Matteo, This patch is to fix the problem found if we apply the new Promise scheduling approach as explained in bug 1193394 comment 48: A promise callback shall be performed earlier at the return of the outermost JS callback when possible rather than at the end of current task according to the HTML spec. [1] In this test, we need to make sure that the sequence of setOverrideDPPX()/setFullZoom() calls in "test OverrideDPPX with MediaQueryList and fullZoom" [2] in correct order as followed in both new/old Promise scheduling to meet the test criteria: 1. setOverrideDPPX(dppx); 2. setFullZoom(zoom); 3. setOverrideDPPX(0); 4. setFullZoom(originalZoom); (The order will be 1 -> 3 -> 4 -> 2 without this fix in new promise scheduling.) I'd like to ask for your review if reasonable because I found that this test was created by you, thanks! [1] https://html.spec.whatwg.org/multipage/webappapis.html#clean-up-after-running-script [2] https://searchfox.org/mozilla-central/rev/c633ffa4c4611f202ca11270dcddb7b29edddff8/dom/tests/mochitest/general/test_contentViewer_overrideDPPX.html#292-299

Comment on attachment 8930357 [details] [diff] [review] (v1) Patch: Fix failure of test_contentViewer_overrideDPPX.html relying on non-comformant Promise handling. Review of attachment 8930357 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/tests/mochitest/general/test_contentViewer_overrideDPPX.html @@ +270,4 @@ > let mql = window.matchMedia(`(resolution: ${dppx}dppx)`); > > mql.addListener(function listener() { > + ok(true, "MediaQueryList's listener for dppx invoked."); This is to make the assert name printed properly in the console instead of "undefined assertion name", btw.

Comment on attachment 8930357 [details] [diff] [review] (v1) Patch: Fix failure of test_contentViewer_overrideDPPX.html relying on non-comformant Promise handling. Review of attachment 8930357 [details] [diff] [review]: ----------------------------------------------------------------- The review is pending for too long. I'd like to ask another person to review it.

Comment on attachment 8930357 [details] [diff] [review] (v1) Patch: Fix failure of test_contentViewer_overrideDPPX.html relying on non-comformant Promise handling. Review of attachment 8930357 [details] [diff] [review]: ----------------------------------------------------------------- Thanks for working on this, and I am sorry you've been waiting! (I believe Matteo is no longer working for Mozilla, so he may not respond.) ::: dom/tests/mochitest/general/test_contentViewer_overrideDPPX.html @@ +290,4 @@ > ]; > > promises[0] > + .then(() => new Promise(resolve => { This change feels a bit mysterious to me unless you know all the details of this promise scheduling change. Does it work to move the `.then` chain below the first two steps, so that they happen in order like before? As in: setOverrideDPPX(dppx); setFullZoom(zoom); promises[0] .then(() => setOverrideDPPX(0)) .then(promises[1]) .then(() => setFullZoom(originalZoom)) .then(done, e => {throw e});

(In reply to J. Ryan Stinnett [:jryans] (use ni?) from comment #6) > Thanks for working on this, and I am sorry you've been waiting! (I believe > Matteo is no longer working for Mozilla, so he may not respond.) Oops, I still see him from the phonebook. :p > This change feels a bit mysterious to me unless you know all the details of > this promise scheduling change. > > Does it work to move the `.then` chain below the first two steps, so that > they happen in order like before? As in: > > setOverrideDPPX(dppx); > setFullZoom(zoom); > > promises[0] > .then(() => setOverrideDPPX(0)) > .then(promises[1]) > .then(() => setFullZoom(originalZoom)) > .then(done, e => {throw e}); Yes, this makes more sense and it works in both old/new scheduling! I'll update the patch for the review again, thanks.

Address the suggestion in comment 6.

Comment on attachment 8934426 [details] [diff] [review] (v2) Patch: Fix failure of test_contentViewer_overrideDPPX.html relying on non-comformant Promise handling. Review of attachment 8934426 [details] [diff] [review]: ----------------------------------------------------------------- Great, looks good to me! :) Feel free to also include the assertion (`ok(true, ...)`) fix from your previous version if you like.

Include the assertion (`ok(true, ...)`) fix for better reading from the log.

Pushed by btseng@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/ac5dec6f0893 Fix failure of test_contentViewer_overrideDPPX.html relying on non-comformant Promise handling. r=jryans