Closed Bug 1373739 Opened 7 years ago Closed 7 years ago

Run Web Platform Tests in Headless Mode

Categories

(Firefox :: Headless, enhancement)

Product:
Firefox
Component:
Headless
Type:
enhancement
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
Firefox 57
Tracking Flags:
Tracking Status
firefox57 --- fixed

People

(Reporter: bdahl, Assigned: mismith)

References

(Regressed 1 open bug)

Details

Attachments

(13 files, 1 obsolete file)

Bug 1373739 - Set up automated test running for WPT in headless mode.
(deleted), text/x-review-board-request
jgraham
: review+
Details
Bug 1373739 - Disable WebGL in headless mode.
(deleted), text/x-review-board-request
bdahl
: review+
jgilbert
: review+
Details
Bug 1373739 - Use ClientLayerManager in headless mode.
(deleted), text/x-review-board-request
jrmuizel
: review+
Details
Bug 1373739 - Constrain widget size to screen size in headless mode.
(deleted), text/x-review-board-request
jrmuizel
: review+
Details
Bug 1373739 - Apply thumb styles directly to their orientation variants.
(deleted), text/x-review-board-request
mossop
: review+
Details
Bug 1373739 - Simulate window activation events in headless mode.
(deleted), text/x-review-board-request
jrmuizel
: review+
Details
Bug 1373739 - Re-enable chrome-switching tests on headless mode.
(deleted), text/x-review-board-request
bdahl
: review+
Details
Bug 1373739 - Headless: use native look-and-feel on Win, hardcoded on Linux.
(deleted), text/x-review-board-request
bdahl
: review+
Details
Bug 1373739 - Enable Marionette "screenshot" tests in headless.
(deleted), text/x-review-board-request
bdahl
: review+
Details
Bug 1373739 - Make headless compositing Windows-compatible, in addition to Linux.
(deleted), text/x-review-board-request
dvander
: review+
Details
Bug 1373739 - Make headless widgets hidden by default, matching other platforms.
(deleted), text/x-review-board-request
bdahl
: review+
Details
Bug 1373739 - Hook HeadlessSound and HeadlessScreenHelper into Windows widgets.
(deleted), text/x-review-board-request
bdahl
: review+
Details
Bug 1373739 - Make macOS compatible with the headless WPT changes.
(deleted), text/x-review-board-request
bdahl
: review+
Details
James Graham kicked of a try run in: https://treeherder.mozilla.org/#/jobs?repo=try&revision=e4282575210badf4ab3a072d5ceab51ee2384e11&filter-searchStr=linux I think most of these will be fixed in part of bug 1355150.
Assignee: nobody → lists
Comment on attachment 8881605 [details] Bug 1373739 - Apply thumb styles directly to their orientation variants. https://reviewboard.mozilla.org/r/152766/#review158092
Attachment #8881605 - Flags: review?(dtownsend) → review+
Comment on attachment 8881604 [details] Bug 1373739 - Constrain widget size to screen size in headless mode. https://reviewboard.mozilla.org/r/152764/#review158196
Attachment #8881604 - Flags: review?(jmuizelaar) → review+
Comment on attachment 8881603 [details] Bug 1373739 - Use ClientLayerManager in headless mode. Removing this patch because I don't think it's the right approach after working on this more.
Attachment #8881603 - Attachment is obsolete: true
Attachment #8881603 - Flags: review?(jmuizelaar)
With these patches I'm seeing green for all of the Marionette and Web Platform Tests all pass under headless, in both e10s and non-e10s mode. https://treeherder.mozilla.org/#/jobs?repo=try&revision=3944c38af3be5a6b35b06dcf4d45bac3b2a1fd79
We're now matching the behavior of non-headless mode on Windows with the Web Platform Tests. There are a few non-green chunks that are also broken in the same way in mozilla-central on Windows: wpt2 and wpt12 in all configurations, due to https://bugzilla.mozilla.org/show_bug.cgi?id=1376430, and wpt4 in debug mode with e10s disabled.
Attachment #8881601 - Flags: review?(bdahl) → review?(dburns)
Comment on attachment 8883798 [details] Bug 1373739 - Headless: use native look-and-feel on Win, hardcoded on Linux. https://reviewboard.mozilla.org/r/154750/#review161690 r+ with the one change ::: widget/headless/HeadlessLookAndFeel.cpp:242 (Diff revision 3) > + NS_WARNING("Someone asked nsILookAndFeel for a color I don't know about"); > +#ifdef DEBUG > + printf_stderr("HeadlessLookAndFeel::NativeGetColor, unknown aID = %" PRIu8 "\n", aID); > +#endif Let's reduce this just to the NS_WARNING, but use the message from your printf.
Attachment #8883798 - Flags: review?(bdahl) → review+
Comment on attachment 8881607 [details] Bug 1373739 - Re-enable chrome-switching tests on headless mode. https://reviewboard.mozilla.org/r/152770/#review161700
Attachment #8881607 - Flags: review?(bdahl) → review+
Comment on attachment 8885447 [details] Bug 1373739 - Enable Marionette "screenshot" tests in headless. https://reviewboard.mozilla.org/r/156304/#review161702
Attachment #8885447 - Flags: review?(bdahl) → review+
Attachment #8885449 - Flags: review?(bdahl) → review?(dburns)
Comment on attachment 8885450 [details] Bug 1373739 - Make headless widgets hidden by default, matching other platforms. https://reviewboard.mozilla.org/r/156310/#review161708
Attachment #8885450 - Flags: review?(bdahl) → review+
Comment on attachment 8885451 [details] Bug 1373739 - Hook HeadlessSound and HeadlessScreenHelper into Windows widgets. https://reviewboard.mozilla.org/r/156312/#review161710
Attachment #8885451 - Flags: review?(bdahl) → review+
Comment on attachment 8881602 [details] Bug 1373739 - Disable WebGL in headless mode. https://reviewboard.mozilla.org/r/152760/#review161718 ::: testing/web-platform/tests/WebIDL/current-realm.html:113 (Diff revision 2) > > ;["2d", "webgl"].forEach(function(val) { > test(function() { > var c = self[0].document.createElement("canvas"), > obj = c.getContext(val) > + // WebGL might not be enabled in this environment How about doing an early return once instead of guarding the assertions?
Comment on attachment 8883798 [details] Bug 1373739 - Headless: use native look-and-feel on Win, hardcoded on Linux. https://reviewboard.mozilla.org/r/154750/#review161690 > Let's reduce this just to the NS_WARNING, but use the message from your printf. Okay, but NS_WARNING takes static text only unfortunately so it won't be able to report which ID was unknown.
Comment on attachment 8881606 [details] Bug 1373739 - Simulate window activation events in headless mode. https://reviewboard.mozilla.org/r/152768/#review162194
Attachment #8881606 - Flags: review?(jmuizelaar) → review+
Comment on attachment 8881603 [details] Bug 1373739 - Use ClientLayerManager in headless mode. https://reviewboard.mozilla.org/r/152762/#review162200 This looks fine. If you want an extra pair of eyes, dvander would be a good choice.
Attachment #8881603 - Flags: review?(jmuizelaar) → review+
Comment on attachment 8885448 [details] Bug 1373739 - Make headless compositing Windows-compatible, in addition to Linux. I'm going to punt this review to dvander
Attachment #8885448 - Flags: review?(jmuizelaar) → review?(dvander)
Attachment #8881601 - Flags: review?(dburns) → review?(bdahl)
Attachment #8881602 - Flags: review?(bdahl)
Attachment #8881603 - Flags: review?(dvander)
Attachment #8885448 - Flags: review?(dvander) → review?(jmuizelaar)
Attachment #8885449 - Flags: review?(dburns) → review?(bdahl)
Attachment #8885448 - Flags: review?(jmuizelaar) → review?(dvander)
Comment on attachment 8881603 [details] Bug 1373739 - Use ClientLayerManager in headless mode. https://reviewboard.mozilla.org/r/152762/#review162606
Attachment #8881603 - Flags: review?(dvander) → review+
Comment on attachment 8885448 [details] Bug 1373739 - Make headless compositing Windows-compatible, in addition to Linux. https://reviewboard.mozilla.org/r/156306/#review162610 This patch overall looks okay, but I'd like to make sure the static_cast comment gets resolved before r+'ing. ::: widget/gtk/nsWindow.cpp:4161 (Diff revision 1) > > #ifdef MOZ_X11 > // Notify the X11CompositorWidget of a ClientSizeChange > // This is different than OnSizeAllocate to catch initial sizing > if (mCompositorWidgetDelegate) { > - mCompositorWidgetDelegate->NotifyClientSizeChanged(GetClientSize()); > + static_cast<X11CompositorWidgetDelegate*>(mCompositorWidgetDelegate)-> We should not have new static_casts like this if we can avoid it, since they're not safe. If these will never be headless widgets, it should be okay to just change the type in nsWindow for both GTK and Windows. (If that works for you, it's okay to move it out of nsBaseWidget.h.)
Attachment #8885448 - Flags: review?(dvander) → review-
Attachment #8885449 - Attachment is obsolete: true
Attachment #8885449 - Flags: review?(bdahl)
Comment on attachment 8885448 [details] Bug 1373739 - Make headless compositing Windows-compatible, in addition to Linux. https://reviewboard.mozilla.org/r/156306/#review162610 > We should not have new static_casts like this if we can avoid it, since they're not safe. If these will never be headless widgets, it should be okay to just change the type in nsWindow for both GTK and Windows. (If that works for you, it's okay to move it out of nsBaseWidget.h.) How's this? I can't entirely pull the CompositorWidgetDelegate into the GTK- and Windows-specific files, as surrounding code such as CompositorSession uses it to pass CompositorWidget-like types around as opaque pointers.
Attachment #8885448 - Flags: review?(jmuizelaar) → review?(dvander)
Comment on attachment 8881602 [details] Bug 1373739 - Disable WebGL in headless mode. https://reviewboard.mozilla.org/r/152760/#review164278
Attachment #8881602 - Flags: review?(bdahl) → review+
Attachment #8885448 - Flags: review?(dvander)
Attachment #8883798 - Flags: review?(bdahl)
Comment on attachment 8885448 [details] Bug 1373739 - Make headless compositing Windows-compatible, in addition to Linux. Sorry for the spam, ReviewBoard cleared some of the review flags I'd set.
Attachment #8885448 - Flags: review?(dvander)
Attachment #8885448 - Flags: review?(dvander)
Attachment #8883798 - Flags: review?(bdahl)
Attachment #8881601 - Flags: review?(dburns) → review?(james)
Attachment #8883798 - Flags: review?(bdahl) → review+
One thing to note is that it might be a really good idea to speak to RelEng/Taskcluster about the added load especially after we have had a few tree closures lately due to backlog.
Flags: needinfo?(lists)
(In reply to Michael Smith [:mismith] from comment #58) > Comment on attachment 8885448 [details] > Bug 1373739 - Make headless compositing Windows-compatible, in addition to > Linux. > > https://reviewboard.mozilla.org/r/156306/#review162610 > > > We should not have new static_casts like this if we can avoid it, since they're not safe. If these will never be headless widgets, it should be okay to just change the type in nsWindow for both GTK and Windows. (If that works for you, it's okay to move it out of nsBaseWidget.h.) > > How's this? I can't entirely pull the CompositorWidgetDelegate into the GTK- > and Windows-specific files, as surrounding code such as CompositorSession > uses it to pass CompositorWidget-like types around as opaque pointers. I mean, does it not work for GTK's nsWindow to have an X11CompositorWidget* mCompositorWidgetDelegate member? And similarly for Windows' nsWindow to have a WinCompositorWidget* mCompositorWidgetDelegate? To assign it from CreateCompositor (and from nsBaseWidget::DestroyCompositor) we could add a virtual setter. Then we don't need all the new casts.
Comment on attachment 8881601 [details] Bug 1373739 - Set up automated test running for WPT in headless mode. https://reviewboard.mozilla.org/r/152758/#review164812 Generally this looks good, but it does need to be rebased and I'm keen to see approval on the additional load before giving r+. ::: taskcluster/ci/test/tests.yml:1568 (Diff revision 3) > + instance-size: xlarge > + docker-image: {"in-tree": "desktop1604-test"} > + checkout: true > + run-on-projects: > + by-test-platform: > + windows10.*: ['mozilla-central', 'try'] This seems like a huge amount of resource usage to test this. Please get sign off from someone in releng that adding this amount of extra load is acceptable. My suspicion is that it makes more sense to enable this on e.g. linux64-e10s only at first. ::: taskcluster/ci/test/tests.yml:1632 (Diff revision 3) > - web_platform_tests/prod_config.py > - remove_executables.py > extra-options: > - --test-type=reftest > > +web-platform-tests-reftests-headless: Once you rebase on inbound this will be wrong because we landed the CSS testsuite there and enabled it on Linux. wpt reftests were rechunked as a result. You'll probably also have to update metadata. ::: testing/web-platform/tests/tools/wptrunner/wptrunner/browsers/firefox.py:131 (Diff revision 3) > + "headless": "MOZ_HEADLESS" in os.environ} > > > def update_properties(): > return (["debug", "stylo", "e10s", "os", "version", "processor", "bits"], > {"debug", "e10s", "stylo"}) If we expect a non-trivial number of headless/not headless differences, then the headless variable should be added here too so that the auto update script can use it.
Attachment #8881601 - Flags: review?(james) → review-
(In reply to David Anderson [:dvander] from comment #62) > (In reply to Michael Smith [:mismith] from comment #58) > > Comment on attachment 8885448 [details] > > Bug 1373739 - Make headless compositing Windows-compatible, in addition to > > Linux. > > > > https://reviewboard.mozilla.org/r/156306/#review162610 > > > > > We should not have new static_casts like this if we can avoid it, since they're not safe. If these will never be headless widgets, it should be okay to just change the type in nsWindow for both GTK and Windows. (If that works for you, it's okay to move it out of nsBaseWidget.h.) > > > > How's this? I can't entirely pull the CompositorWidgetDelegate into the GTK- > > and Windows-specific files, as surrounding code such as CompositorSession > > uses it to pass CompositorWidget-like types around as opaque pointers. > > I mean, does it not work for GTK's nsWindow to have an X11CompositorWidget* > mCompositorWidgetDelegate member? And similarly for Windows' nsWindow to > have a WinCompositorWidget* mCompositorWidgetDelegate? To assign it from > CreateCompositor (and from nsBaseWidget::DestroyCompositor) we could add a > virtual setter. Then we don't need all the new casts. I think I understand what you mean now. We can't have `{X11,Win}CompositorWidget* mCompositorWidgetDelegate` as the CompositorChild classes for GTK and Windows also implement CompositorWidgetDelegate. But we could have `PlatformCompositorWidgetDelegate* mCompositorWidgetDelegate` moved into the versions of nsWindow for GTK and Windows. Then nsBaseWidget::CreateCompositor would use a virtual setter of the form `virtual void SetCompositorWidgetDelegate(CompositorWidgetDelegate* delegate) {}`, which GTK and Windows would override to unwrap the CompositorWidgetDelegate* into a PlatformCompositorWidgetDelegate* and headless would use to unwrap it into a HeadlessCompositorWidget*. However, if you want to get rid of even that initial, one-time cast from the generic CompositorWidgetDelegate type during nsBaseWidget::CreateCompositor, I don't know what the right approach would be.
Flags: needinfo?(lists)
Comment on attachment 8881601 [details] Bug 1373739 - Set up automated test running for WPT in headless mode. https://reviewboard.mozilla.org/r/152758/#review164812 > This seems like a huge amount of resource usage to test this. Please get sign off from someone in releng that adding this amount of extra load is acceptable. My suspicion is that it makes more sense to enable this on e.g. linux64-e10s only at first. Thanks for catching this! This was a mistake owing to my basing the -headless variant configurations on the original ones for the web-platform-tests sets. I've emptied the run-on-projects list for the new -headless configurations (and double-checked the rest of the configuration) so that they shouldn't be set to run automatically yet. My intention was always to coordinate with someone in releng after these changes land to figure out where it makes sense to enable the tests. > Once you rebase on inbound this will be wrong because we landed the CSS testsuite there and enabled it on Linux. wpt reftests were rechunked as a result. You'll probably also have to update metadata. Rebased and updated. > If we expect a non-trivial number of headless/not headless differences, then the headless variable should be added here too so that the auto update script can use it. We actually want as few headless/non-headless differences as possible, and almost any that show up should be considered a bug. Am I understanding correctly that "auto update script" autogenerates expected results for test set configurations without human review? If so I'm wary of making this change.
Flags: needinfo?(dvander)
(In reply to Michael Smith [:mismith] from comment #76) > > I think I understand what you mean now. We can't have > `{X11,Win}CompositorWidget* mCompositorWidgetDelegate` as the > CompositorChild classes for GTK and Windows also implement > CompositorWidgetDelegate. But we could have > `PlatformCompositorWidgetDelegate* mCompositorWidgetDelegate` moved into the > versions of nsWindow for GTK and Windows. Then > nsBaseWidget::CreateCompositor would use a virtual setter of the form > `virtual void SetCompositorWidgetDelegate(CompositorWidgetDelegate* > delegate) {}`, which GTK and Windows would override to unwrap the > CompositorWidgetDelegate* into a PlatformCompositorWidgetDelegate* and > headless would use to unwrap it into a HeadlessCompositorWidget*. > > However, if you want to get rid of even that initial, one-time cast from the > generic CompositorWidgetDelegate type during nsBaseWidget::CreateCompositor, > I don't know what the right approach would be. Yeah, exactly. One-time cast is okay, I was just hoping to avoid it everywhere it's used in nsWindow.cpp.
Flags: needinfo?(dvander)
(In reply to David Anderson [:dvander] from comment #78) > (In reply to Michael Smith [:mismith] from comment #76) > > > > I think I understand what you mean now. We can't have > > `{X11,Win}CompositorWidget* mCompositorWidgetDelegate` as the > > CompositorChild classes for GTK and Windows also implement > > CompositorWidgetDelegate. But we could have > > `PlatformCompositorWidgetDelegate* mCompositorWidgetDelegate` moved into the > > versions of nsWindow for GTK and Windows. Then > > nsBaseWidget::CreateCompositor would use a virtual setter of the form > > `virtual void SetCompositorWidgetDelegate(CompositorWidgetDelegate* > > delegate) {}`, which GTK and Windows would override to unwrap the > > CompositorWidgetDelegate* into a PlatformCompositorWidgetDelegate* and > > headless would use to unwrap it into a HeadlessCompositorWidget*. > > > > However, if you want to get rid of even that initial, one-time cast from the > > generic CompositorWidgetDelegate type during nsBaseWidget::CreateCompositor, > > I don't know what the right approach would be. > > Yeah, exactly. One-time cast is okay, I was just hoping to avoid it > everywhere it's used in nsWindow.cpp. Okay, I've updated the patch accordingly.
Comment on attachment 8885448 [details] Bug 1373739 - Make headless compositing Windows-compatible, in addition to Linux. https://reviewboard.mozilla.org/r/156306/#review164800 ::: widget/CompositorWidget.h:50 (Diff revision 2) > -class CompositorWidgetDelegate; > +class PlatformCompositorWidgetDelegate; > + > +// Headless mode uses its own, singular CompositorWidget implementation. > +class HeadlessCompositorWidget; > + > +class CompositorWidgetDelegate { nit: brace on new line ::: widget/nsBaseWidget.cpp:283 (Diff revision 4) > // When CompositorSession::Shutdown returns, we assume the compositor is gone > // or will be gone very soon. > if (mCompositorSession) { > ReleaseContentController(); > mAPZC = nullptr; > - mCompositorWidgetDelegate = nullptr; > + SetCompositorWidgetDelegate(nullptr); I just realized a problem, it is not legal to call a virtual function from here, since we could be called from DestroyLayerManager, from the nsBaseWidget destructor. The derived class won't exist anymore in that case. I can't think of a good way to fix this, other than moving the declaration back into nsWindow and having a platform-specific #ifdef or two. I'm okay with that or reverting back to the previous patch - whichever is easiest for you. I don't want to hold this up any longer :) it looks good otherwise.
Attachment #8885448 - Flags: review?(dvander) → review+
Comment on attachment 8885448 [details] Bug 1373739 - Make headless compositing Windows-compatible, in addition to Linux. https://reviewboard.mozilla.org/r/156306/#review164800 > I just realized a problem, it is not legal to call a virtual function from here, since we could be called from DestroyLayerManager, from the nsBaseWidget destructor. The derived class won't exist anymore in that case. I can't think of a good way to fix this, other than moving the declaration back into nsWindow and having a platform-specific #ifdef or two. I'm okay with that or reverting back to the previous patch - whichever is easiest for you. I don't want to hold this up any longer :) it looks good otherwise. If DestroyCompositor is being called indirectly from ~nsBaseWidget, then the call to SetCompositorWidgetDelegate should resolve to nsBaseWidget::SetCompositorWidgetDelegate, as per http://eel.is/c++draft/class.cdtor#3 (test case: http://ideone.com/nqu2bD ). nsBaseWidget::SetCompositorWidgetDelegate is a no-op, so it should be safe to call.
Whoops you're right, it is not pure virtual.
Comment on attachment 8881601 [details] Bug 1373739 - Set up automated test running for WPT in headless mode. https://reviewboard.mozilla.org/r/152758/#review168698 ::: taskcluster/ci/test/tests.yml:1788 (Diff revision 7) > + macosx.*: true > + default: both > + max-run-time: 7200 > + instance-size: xlarge > + docker-image: {"in-tree": "desktop1604-test"} > + run-on-projects: [] I think this needs a comment
Attachment #8881601 - Flags: review?(james) → review+
Comment on attachment 8892118 [details] Bug 1373739 - Make macOS compatible with the headless WPT changes. https://reviewboard.mozilla.org/r/163104/#review169918
Attachment #8892118 - Flags: review?(bdahl) → review+
Pushed by michael@spinda.net: https://hg.mozilla.org/integration/autoland/rev/52be5f7846f4 Disable WebGL in headless mode. r=bdahl https://hg.mozilla.org/integration/autoland/rev/a179412ca028 Constrain widget size to screen size in headless mode. r=jrmuizel https://hg.mozilla.org/integration/autoland/rev/349c74ed0bd1 Apply thumb styles directly to their orientation variants. r=mossop https://hg.mozilla.org/integration/autoland/rev/6ba8cca5631f Simulate window activation events in headless mode. r=jrmuizel https://hg.mozilla.org/integration/autoland/rev/5cd67cab4b11 Re-enable chrome-switching tests on headless mode. r=bdahl https://hg.mozilla.org/integration/autoland/rev/e8ee1babd3e8 Use ClientLayerManager in headless mode. r=dvander,jrmuizel https://hg.mozilla.org/integration/autoland/rev/36f942289a13 Enable Marionette "screenshot" tests in headless. r=bdahl https://hg.mozilla.org/integration/autoland/rev/3a9f911d58cd Make headless widgets hidden by default, matching other platforms. r=bdahl https://hg.mozilla.org/integration/autoland/rev/db5a86cfd703 Make headless compositing Windows-compatible, in addition to Linux. r=dvander https://hg.mozilla.org/integration/autoland/rev/dd5fea40c368 Hook HeadlessSound and HeadlessScreenHelper into Windows widgets. r=bdahl https://hg.mozilla.org/integration/autoland/rev/80a431fb9fe4 Headless: use native look-and-feel on Win, hardcoded on Linux. r=bdahl https://hg.mozilla.org/integration/autoland/rev/9448d431b2ca Make macOS compatible with the headless WPT changes. r=bdahl https://hg.mozilla.org/integration/autoland/rev/c045e10faa19 Set up automated test running for WPT in headless mode. r=jgraham
Comment on attachment 8881602 [details] Bug 1373739 - Disable WebGL in headless mode. https://reviewboard.mozilla.org/r/152760/#review173764
Attachment #8881602 - Flags: review+
Regressions: 1375585
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: