Closed Bug 1264062 Opened 8 years ago Closed 8 years ago

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

Categories

(Testing :: General, defect)

Version 3
Unspecified
Windows XP
defect
Not set
normal

Tracking

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

RESOLVED FIXED
mozilla48
Tracking Status
e10s ? ---
firefox46 --- wontfix
firefox47 --- ?
firefox48 --- fixed

People

(Reporter: RyanVM, Unassigned)

References

(Blocks 1 open bug)

Details

Attachments

(1 file)

+++ 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)
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.
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+
https://hg.mozilla.org/mozilla-central/rev/745ea81986fc
Status: NEW → RESOLVED
Closed: 8 years ago
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.
You need to log in before you can comment on or make changes to this bug.