Windows XP debug tests are perma-asserting on Ash after the landing of bug 1262954

RESOLVED FIXED in Firefox 48

Status

RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: RyanVM, Unassigned)

Tracking

(Blocks: 1 bug)

Version 3
mozilla48
Unspecified
Windows XP
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(e10s?, firefox46 wontfix, firefox47 ?, firefox48 fixed)

Details

Attachments

(1 attachment)

(Reporter)

Description

3 years ago
+++ This bug was initially created as a clone of Bug #1262954 +++

Looks like bug 1262954 fixed opt builds on Ash, but debug runs are all permafailing with a "!sPrefBrowserTabsRemoteAutostart || IsVistaOrLater()" assertion in gfx/thebes/gfxPlatform.cpp. George said he'd take a look.

https://treeherder.mozilla.org/logviewer.html#?job_id=165762&repo=ash
Flags: needinfo?(gwright)
Created attachment 8740630 [details] [diff] [review]
0001-Bug-1264062-Don-t-bother-checking-which-accelerated-.patch

We're failing the assert because there was an assumption in the assert that we wouldn't bother checking what accelerated layer types are available if layers acceleration isn't enabled.
Flags: needinfo?(gwright)
Attachment #8740630 - Flags: review?(milan)
Comment on attachment 8740630 [details] [diff] [review]
0001-Bug-1264062-Don-t-bother-checking-which-accelerated-.patch

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

Check with :dvander on my claim as well.

::: gfx/thebes/gfxPlatform.cpp
@@ +2079,4 @@
>      if (gfxPrefs::LayersAccelerationForceEnabled()) {
>        sLayersSupportsD3D9 = true;
>        sLayersSupportsD3D11 = true;
> +    } else if (!gfxPrefs::LayersAccelerationDisabled() && gfxInfo) {

I have a feeling that should be gfxPlatform::ShouldUseLayersAcceleration(), rather than gfxPrefs::LayersAccelerationDisabled().
Attachment #8740630 - Flags: review?(milan) → review-
(In reply to Milan Sreckovic [:milan] from comment #3)
> I have a feeling that should be gfxPlatform::ShouldUseLayersAcceleration(),
> rather than gfxPrefs::LayersAccelerationDisabled().

I don't think that's going to work easily in practice. InitLayersAccelerationPrefs() is a local function within gfxPlatform, which is called when initialising gfxPlatform itself. ShouldUseLayersAcceleration() is a method on gfxPlatform which isn't static, and isn't easily made static due to AcceleratedLayersByDefault() being virtual.

I mean, it looks like it'll probably work if I do gfxPlatform::GetPlatform()->ShouldUseLayersAcceleration(), but I don't really like the idea of doing that mid-initialise for gfxPlatform.
Using gfxPlatform::GetPlatform()->ShouldUseLayersAcceleration() looks green on try.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=a35224887196

(Patch at https://hg.mozilla.org/try/rev/8e1e3d8bcfac)

But I'm still not convinced it's a good idea.
(Reporter)

Comment 6

3 years ago
Your Try push doesn't have e10s force-enabled. You'll want to apply the patch below and re-push.
https://hg.mozilla.org/try/rev/5af92c1d7d1106dbc73b772efdcb14965703cebf

To verify it's working correctly, you can check the logs and verify that the Mode is set to e10s (the run from comment 5 shows non-e10s as expected).
Should be the same either way, as the code I added is executed in e10s and non-e10s.
Comment on attachment 8740630 [details] [diff] [review]
0001-Bug-1264062-Don-t-bother-checking-which-accelerated-.patch

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

I take my objections back.  This should be fine, given where we're using it.
Attachment #8740630 - Flags: review- → review+

Comment 10

3 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/745ea81986fc
Status: NEW → RESOLVED
Last Resolved: 3 years ago
status-firefox48: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
In bug 1262954 comment 14, jmaher said he would like to uplift bug 1262954 to Beta 47. In bug 1262954 comment 16, RyanVM says we will want to take this bug fix if we take that one.
status-firefox46: --- → wontfix
status-firefox47: --- → ?
You need to log in before you can comment on or make changes to this bug.