Closed Bug 1270252 Opened 8 years ago Closed 8 years ago

test_gfxBlacklist_No_Comparison.js fails when run in debug mode on windows 7 VM

Categories

(Core :: Graphics, defect)

Product:
Core
Component:
Graphics
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla49
Tracking Flags:
Tracking Status
firefox49 --- fixed

People

(Reporter: jmaher, Assigned: milan)

References

Details

(Whiteboard: [gfx-noted])

Attachments

(1 file, 1 obsolete file)

MozReview Request: Bug 1270252: Skip a blocklisting test when everything is blocked because of a missing driver version. Clean up some returns from blocklisting, where the status and return value were missing or inconsistent. r?benwa
58 bytes, text/x-review-board-request
BenWa
: review+
Details 
we are working on getting windows 7 running tests in AWS.  I found that the xpcshell test:
mozapps/extensions/test/xpcshell/test_gfxBlacklist_No_Comparison.js

fails on a machine without a GPU and in debug only (example log: http://archive.mozilla.org/pub/firefox/try-builds/jmaher@mozilla.com-c81e54baacd264dbf66d456a8a02972079241070/try-win32-debug/try_win7_vm-debug_test-xpcshell-bm128-tests1-windows-build9.txt.gz):
09:33:13     INFO -  TEST-PASS | xpcshell.ini:toolkit/mozapps/extensions/test/xpcshell/test_gfxBlacklist_No_Comparison.js | checkBlacklist - [checkBlacklist : 71] 4 == 4
09:33:13  WARNING -  TEST-UNEXPECTED-FAIL | xpcshell.ini:toolkit/mozapps/extensions/test/xpcshell/test_gfxBlacklist_No_Comparison.js | checkBlacklist - NS_ERROR_FAILURE: Component returned failure code: 0x80004005 (NS_ERROR_FAILURE) [nsIGfxInfo.getFeatureStatus]
09:33:13     INFO -  checkBlacklist@c:/slave/test/build/tests/xpcshell/tests/toolkit/mozapps/extensions/test/xpcshell/test_gfxBlacklist_No_Comparison.js:74:14
09:33:13     INFO -  do_execute_soon/<.run@c:\slave\test\build\tests\xpcshell\head.js:692:9
09:33:13     INFO -  _do_main@c:\slave\test\build\tests\xpcshell\head.js:209:5
09:33:13     INFO -  _execute_test@c:\slave\test\build\tests\xpcshell\head.js:533:5
09:33:13     INFO -  @-e:1:1
09:33:13     INFO -  exiting test
09:33:13     INFO -  (xpcshell/head.js) | test checkBlacklist finished (2)


I assume this is failing only because we are running on a non GPU instance- odd why this doesn't fail in opt mode (I verified it ran ok).  

I am working in bug 1269872 to move all tests which require a GPU into the same job.  This is the only xpcshell test that appears to require a GPU- I am wondering if we can edit the test or make it smarter.
:milan, I saw you modified this specific test about 9 months ago, is there a chance you could help me figure out a resolution or point me to someone who could help out?
Flags: needinfo?(milan)
Sure thing, I'll look at it today.
Joel, does this help?  If it does, I'll get a "real" patch and get it reviewed.
Flags: needinfo?(milan) → needinfo?(jmaher)
oh, this looks cool- I will run this through the magical try server tonight on my next iteration and see if this works.  Thanks for making a simple fix so quickly!
Flags: needinfo?(jmaher)
this didn't seem to work:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=fbb8eb36c7796b1f62165e628d00944b3769f58a&selectedJob=20431453&filter-searchStr=xpcshell

as a note, this is debug only, possibly we don't need this running in debug as we will have coverage in opt mode?  then again, this fails in debug, maybe it is important to be running there.
These tests only run in debug mode, so it's important they do so.  I'm having trouble building Firefox these days, because of the recent VC 2013 incompatibilities; I'll have to get to this next week, hopefully I'll be able to build by then.
Flags: needinfo?(milan)
ok great- if you want to get some printf's in place, or try a few other things, I can do that over the weekend and see if we can get to an answer by monday!
If you need something going, and this is the only test failing, you can disable it on windows.  The first failure is addressed with the patch above; there is now a problem in the test itself.  We basically assume that we should get the acceleration unless there is blocklisting - when you're running the tests in a VM without any graphics acceleration, the tests will fail.

This one can't be the only one that's failing, can it?
Flags: needinfo?(milan) → needinfo?(jmaher)
So, let me summarize:

The blocklisting tests are only doing the work in debug.  In opt they trivially succeed.
These tests are assuming there is acceleration unless blocklisting stops it.  As such, they will start failing on non-GPU instances, and are useless there.  Assuming we will keep running the GPU instances as well, we can disable all of the failing tests on Windows non-GPU.
this isn't urgent to the hour,  Ican wait until Monday- just thought I would save you some time on Monday if possible.

this is the only test in xpcshell that fails in opt/debug on non gpu vms.
Flags: needinfo?(jmaher)
Whiteboard: [gfx-noted]
checking in here on any status or ideas to try.
Yeah, I've been stuck not being able to build on Windows (VC2013 hasn't been building for about a week now), sorry about that.
Depends on: 1271389
ah, got it- I believe on trunk we only support vs2015- not sure of the full story there.
OK, got the build, reproduced this locally.  There are a few pieces of code that should be cleaned up, could even call them bugs, and I'll do that.  However, this test will keep failing.  It tests that particular acceleration is available if it isn't blocked; however, it doesn't exist in the first place, so it won't be available, with or without the blocking.  We don't have the API to tell the difference.
ok, looks like we will have to go to the drawing board maybe?  Is it possible to run this test in a different framework like mochitest?
Attachment #8749217 - Attachment is obsolete: true
(In reply to Joel Maher (:jmaher) from comment #15)
> ok, looks like we will have to go to the drawing board maybe?  Is it
> possible to run this test in a different framework like mochitest?

The patch above should let the test pass (as a no-op) on "there is no graphics" setups, but still run and test things when we do have graphics.  So, as long as we test on some platforms that have graphics, we should be OK.
Assignee: nobody → milan
thanks Milan!  This will run on a lot of different platforms like it currently does.  as it stands, we are just moving win7, but plan to do the same with win10- ideally we will only have a few jobs that need to run on gpu hardware, and we should make sure as we port more tests to the cloud that we don't forget this.
Attachment #8751318 - Flags: review?(bgirard) → review+
Comment on attachment 8751318 [details]
MozReview Request: Bug 1270252: Skip a blocklisting test when everything is blocked because of a missing driver version. Clean up some returns from blocklisting, where the status and return value were missing or inconsistent. r?benwa

https://reviewboard.mozilla.org/r/51955/#review48809

::: toolkit/mozapps/extensions/test/xpcshell/test_gfxBlacklist_No_Comparison.js:72
(Diff revision 1)
>  
>    function checkBlacklist()
>    {
> +    var driverVersion = gfxInfo.adapterDriverVersion;
> +    if (driverVersion) {
> -    var status = gfxInfo.getFeatureStatus(Ci.nsIGfxInfo.FEATURE_DIRECT2D);
> +      var status = gfxInfo.getFeatureStatus(Ci.nsIGfxInfo.FEATURE_DIRECT2D);

Unrelated but these test still assume that we run our test on configurations that support these graphics feature by default. Would be best if, by default, our test passed on all machines unless you force tests to run a given feature.

::: widget/GfxInfoBase.cpp:831
(Diff revision 1)
>    if (NS_FAILED(GetAdapterVendorID(adapterVendorID)) ||
>        NS_FAILED(GetAdapterDeviceID(adapterDeviceID)) ||
>        NS_FAILED(GetAdapterDriverVersion(adapterDriverVersionString)))
>    {
>      aFailureId = "FEATURE_FAILURE_CANT_RESOLVE_ADAPTER";
> +		*aStatus = FEATURE_BLOCKED_DEVICE;

Tabs instead of spaces.
Comment on attachment 8751318 [details]
MozReview Request: Bug 1270252: Skip a blocklisting test when everything is blocked because of a missing driver version. Clean up some returns from blocklisting, where the status and return value were missing or inconsistent. r?benwa

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/51955/diff/1-2/
(In reply to Benoit Girard (:BenWa) from comment #19)
> Comment on attachment 8751318 [details]
> MozReview Request: Bug 1270252: Skip a blocklisting test when everything is
> blocked because of a missing driver version. Clean up some returns from
> blocklisting, where the status and return value were missing or
> inconsistent. r?benwa
> 
> https://reviewboard.mozilla.org/r/51955/#review48809
> 
> :::
> toolkit/mozapps/extensions/test/xpcshell/test_gfxBlacklist_No_Comparison.js:
> 72
> (Diff revision 1)
> >  
> >    function checkBlacklist()
> >    {
> > +    var driverVersion = gfxInfo.adapterDriverVersion;
> > +    if (driverVersion) {
> > -    var status = gfxInfo.getFeatureStatus(Ci.nsIGfxInfo.FEATURE_DIRECT2D);
> > +      var status = gfxInfo.getFeatureStatus(Ci.nsIGfxInfo.FEATURE_DIRECT2D);
> 
> Unrelated but these test still assume that we run our test on configurations
> that support these graphics feature by default. Would be best if, by
> default, our test passed on all machines unless you force tests to run a
> given feature.

The way it works (and if we want to change that, we can), is that platforms that don't understand a feature never block it.  So, OS X will happily say it isn't blocklisting D2D, because, well, it isn't.  What we were running into in this test (and I'm still unclear why we weren't running into it in other tests) was that we were failing *all* feature because we couldn't get the driver version.
Ohh I see. In my other patch in bug I'm working towards reporting a better status (FEATURE_FAILURE_NOT_SUPPORTED) for features that aren't available on the platform. I wonder how much of our code/tests make this assumption.
(In reply to Benoit Girard (:BenWa) from comment #22)
> Ohh I see. In my other patch in bug I'm working towards reporting a better
> status (FEATURE_FAILURE_NOT_SUPPORTED) for features that aren't available on
> the platform. I wonder how much of our code/tests make this assumption.

Right - in the "real code", I wouldn't expect OS X to ask if D2D is blocked, for example.  The blocklist itself, however, is not intelligent or platform specific - it takes whatever blocks we explicitly give it, and that's what it reports as being blocked.  There are no implicit blocklist items (D2D is blocked on everything but Windows), and I think that's OK.  If we wanted to, we could explicitly add those in (e.g., have GfxInfo.mm block D2D and D3D), but I'm not sure of the value.
https://hg.mozilla.org/mozilla-central/rev/e2729ae757f0
Status: NEW → RESOLVED
Closed: 8 years ago
status-firefox49: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla49
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: