1263092 - Unable to run reftest locally with e10s enabled (if screen resolution is too low)

Status: RESOLVED FIXED

(Testing :: Reftest, defect)

Description

[Ting-Yu Lin [:TYLin] (PDT, UTC-7)]

Attachment: log-reftest-e10s.txt

Running my own debug build without optimization on Mac OS with "./mach reftest layout/reftests/details-summary/" and every test has an error like

REFTEST ERROR | file:///Users/tlin/Projects/gecko-dev/layout/reftests/details-summary/disabled-single-summary-ref.html | can't drawWindow remote content

Full reftest log is attached.

Running "./mach reftest --disable-e10s layout/reftests/details-summary/" is OK.

Comment 1

[Astley Chen (inactive)]

we'll need this bug fixed as soon as possible so that we can ensure following commits can pass reftest on e10s.

Comment 2

[Daniel Holbert [:dholbert]]

I know of two newish Gecko developers who have been tripped up by this issue in recent weeks -- ./mach reftest failing to do anything useful, by default, unless they were able to infer that they needed to add "--disable-e10s".

It's extremely bad that local reftest runs are *broken by default* (at least in these developers' configurations; not in my configuration. I don't know what the key difference is offhand).

Tagging ahal for needinfo, since I think he may have been working on getting our test harnesses e10s-enabled -- ahal, do you know who might work on this & if we can prioritize this? Is there any useful information/logging (beyond the log that's already attached) that affected developers can provide to help out here?  And is this "can't drawWindow remote content" behavior currently expected or unexpected?

Comment 3

[Matt Woodrow (:mattwoodrow)]

Reftests require the window size to be 800x1000 (and so need a screen resolution of at least this, possibly more if you have the dock permanently in view).

The main error is 'REFTEST ERROR | WARNING: USE_WIDGET_LAYERS disabled' with the following line showing that you were only able to get a height of 724 (I assume your screen is 1440x900?).

This error means that the window we have in the compositor isn't big enough to include the entire test, so doing readback from the GPU would be missing content.

Without e10s we just redraw the entire page in software during the DrawWindow call and get somewhat valid results, but these aren't always useful (nor the same as the results you'd get on tinderbox).

With e10s, we can't do this as the DrawWindow call happens in the parent process, but the content we need to redraw is in the child process. We don't have the plumbing to access this, so we just fail instead.

The biggest issue here is probably that the USE_WIDGET_LAYERS disabled error message is very easy to miss.

Comment 4

[Xidorn Quan [:xidorn] UTC+11]

1440x900 sounds like a very common resolution, especially on laptops. Some laptops have an even smaller resolution, e.g. 1440x810 (my ThinkPad has 2880x1620 with 2dppx, which basically means 1440x810). I think we should support those machines.

Could we scale the view somehow so that it always fits in? That may hide some fuzzy... but probably better than doing nothing at all... Alternatively, is it possible to split the window into multiple parts on the screen?

On macOS, though, I guess changing layout.css.devPixelsPerPx to 1.0 for reftest would workaround this issue. But it may not always that case.

Comment 5

[Matt Woodrow (:mattwoodrow)]

Attachment: Improve error logging

Comment 6

[Matt Woodrow (:mattwoodrow)]

(In reply to Xidorn Quan [:xidorn] (UTC+10) from comment #4)
> 1440x900 sounds like a very common resolution, especially on laptops. Some
> laptops have an even smaller resolution, e.g. 1440x810 (my ThinkPad has
> 2880x1620 with 2dppx, which basically means 1440x810). I think we should
> support those machines.
> 
> Could we scale the view somehow so that it always fits in? That may hide
> some fuzzy... but probably better than doing nothing at all...
> Alternatively, is it possible to split the window into multiple parts on the
> screen?
> 
> On macOS, though, I guess changing layout.css.devPixelsPerPx to 1.0 for
> reftest would workaround this issue. But it may not always that case.

The best fix would probably be going through the test suite and making sure all tests fit their important content within a smaller area. That's a lot of work though.

Scaling and changing the devPixelsPerPx ratio would both work, but then they run the risk of local results not matching tbpl, and results varying per-machine. I'm worried that these sort of bugs will cause intermittent pain for developers that will be much harder to track down.

Unconditionally changing devPixelsPerPx would lose test coverage of HighDPI.

Making it obvious that the current screen resolution doesn't support reftests properly, so that the user knows to try change it seems like the best compromise to me.

Comment 7

[Daniel Holbert [:dholbert]]

Comment on attachment 8768599 [details] [diff] [review]
Improve error logging

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

(I seem to recall dbaron wanting to review all/most reftest harness changes; this change is small enough that I think we don't need to bother him, but he's CC'd on this bug and can chime in if he likes.)

r=me with one nit:

::: layout/tools/reftest/reftest.jsm
@@ +1389,5 @@
>      logger.suiteEnd(extra={'results': gTestResults});
>      logger.info("Slowest test took " + gSlowestTestTime + "ms (" + gSlowestTestURL + ")");
>      logger.info("Total canvas count = " + gRecycledCanvases.length);
> +    if (gFailedUseWidgetLayers) {
> +        logger.error("USE_WIDGET_LAYERS disabled because the screen resolution is too low. This error is fatal with e10s enabled.");

Looks like you've got this error message copypasted twice here.  That's probably sub-optimal, since we might tweak one and forget to tweak the other as well.

Could you pull the string out into a global const variable, or into a helper-function?

Comment 8

[Xidorn Quan [:xidorn] UTC+11]

(In reply to Matt Woodrow (:mattwoodrow) from comment #6)
> Scaling and changing the devPixelsPerPx ratio would both work, but then they
> run the risk of local results not matching tbpl, and results varying
> per-machine. I'm worried that these sort of bugs will cause intermittent
> pain for developers that will be much harder to track down.
> 
> Unconditionally changing devPixelsPerPx would lose test coverage of HighDPI.

Do we have HiDPI coverage on our test infra? It doesn't seem to me so, and thus I don't think it makes much sense to keep that locally at this moment.

Also I guess it doesn't need to be changed unconditionally. It could be set dynamically when HiDPI and too-small resolution is detected. devPixelsPerPx isn't something request a restart.

> Making it obvious that the current screen resolution doesn't support
> reftests properly, so that the user knows to try change it seems like the
> best compromise to me.

Even that way, we should probably provide some advice on possible workaround, otherwise people would still get confused and need to ask others for help. But again, I think the workaround can be integrated into the test harness directly, and when some workaround is applied automatically, we can show a warning with the summary so that people can notice that.

Comment 9

[Andrew Halberstadt [:ahal]]

Comment on attachment 8768599 [details] [diff] [review]
Improve error logging

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

Thanks for looking into this! Aside from this patch, it's not really clear what else we can do. I think it's important e10s remains default and I'd be against any sort of "automatic fallback" to non-e10s. As far as changing the resolution requirements go, we have looked into that before and it would be a massive task updating all the tests. Plus, it is very hard to guarantee a test's correctness after we make that change (the test may look like it's passing, but in reality isn't testing what it was designed to test).

::: layout/tools/reftest/reftest.jsm
@@ +1389,5 @@
>      logger.suiteEnd(extra={'results': gTestResults});
>      logger.info("Slowest test took " + gSlowestTestTime + "ms (" + gSlowestTestURL + ")");
>      logger.info("Total canvas count = " + gRecycledCanvases.length);
> +    if (gFailedUseWidgetLayers) {
> +        logger.error("USE_WIDGET_LAYERS disabled because the screen resolution is too low. This error is fatal with e10s enabled.");

Maybe also mention --disable-e10s here?

Comment 10

[Andrew Halberstadt [:ahal]]

I should also mention if the developers who ran into this want, they can alias --disable-e10s in ~/.mozbuild/machrc:

[alias]
reftest = reftest --disable-e10s

Comment 11

[Daniel Holbert [:dholbert]]

+1 to comment 9 - we should make it more obvious how to quickly work around this. We should add something like:
   "Consider increasing your screen resolution, or adding '--disable-e10s' to your './mach reftest' command"
...to the end of the logger.error() string.

Comment 12

[Jonathan Chan [:jyc]]

Today I ran into a problem on macOS where when I ran without --disable-e10s (and the log shows the reftests to be using e10s), every test seems to pass. I'm running on 303456. Is this possibly related?

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/60fdac16b458
Improve error logging when reftests fail due to the screen resolution being too low. r=dholbert

Comment 14

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/60fdac16b458

Comment 15

[Andrew Halberstadt [:ahal]]

(In reply to Jonathan Chan [:jyc] from comment #12)
> Today I ran into a problem on macOS where when I ran without --disable-e10s
> (and the log shows the reftests to be using e10s), every test seems to pass.
> I'm running on 303456. Is this possibly related?

Could you provide some more information? Are you expecting the tests to fail?

Comment 16

[Daniel Holbert [:dholbert]]

(I suspect Jonathan's issue is unrelated to what was going on here, in any case. Jonathan, consider filing a new bug in this component for your issue [if you're still hitting it] with more details - thanks!)

Comment 17

[Astley Chen (inactive)]

(In reply to Carsten Book [:Tomcat] - PTO-back Sept 4th from comment #14)
> https://hg.mozilla.org/mozilla-central/rev/60fdac16b458

I don't this bug was fixed by just adding additional logs to the code. Let's keep it open and figure out the root cause. Aliasing or changing pref of device pixel mapping is not making sense to me and other developers.

Comment 18

[Jonathan Watt [:jwatt]]

(In reply to Astley Chen [:astley] (UTC+8) from comment #17)
> I don't this bug was fixed by just adding additional logs to the code. Let's
> keep it open and figure out the root cause. Aliasing or changing pref of
> device pixel mapping is not making sense to me and other developers.

The only reliable fix is to increasing your screen resolution as suggested by the added logging...or to audit and fix-up the tests to fit within a shorter window. I don't see what other option there is here.

Comment 19

[Xidorn Quan [:xidorn] UTC+11]

FWIW, running
> ./mach reftest --setpref=layout.css.devPixelsPerPx=1.0
can be a workaround on macOS.

Comment 20

[BugBot [:suhaib / :marco/ :calixte]]

The leave-open keyword is there and there is no activity for 6 months.
:mattwoodrow, maybe it's time to close this bug?