Page MenuHomePhabricator
Differential D111890

Bug 1698732 - Change reftest-content to get Azure info without Win32k APIs
ClosedPublic
Actions

Authored by cmartin on Apr 13 2021, 5:31 PM.
Tags
Referenced Files
Unknown Object (File)
Mon, Apr 21, 2:53 PM
Unknown Object (File)
Sat, Apr 19, 5:25 AM
Unknown Object (File)
Fri, Apr 18, 10:36 PM
Unknown Object (File)
Jan 3 2025, 2:04 AM
Unknown Object (File)
Jun 2 2024, 3:26 PM
Subscribers
emilio
jrmuizel

Details

Reviewers
bas
emilio
jrmuizel
Commits
rMOZILLACENTRALe201e698220e: Bug 1698732 - Change reftest-content to get Azure info without Win32k APIs…
Bugzilla Bug ID
1698732
Summary

Currently, reftest-content uses GfxInfo::GetInfo() to obtain information about
the Azure backend. GetInfo() uses Win32k APIs, and therefore will mostly
return garbage in content processes.

This adds a new way to obtain the same information directly from GfxInfo
without using Win32k APIs.

Diff Detail

Repository
rMOZILLACENTRAL mozilla-central
Lint
Lint Not Applicable
Unit
Tests Not Applicable
Build Status
Buildable 315542
Build 409292: arc lint + arc unit

Event Timeline

cmartin created this revision.Apr 13 2021, 5:31 PM
Herald added a project: secure-revision. · View Herald TranscriptApr 13 2021, 5:31 PM
Harbormaster completed remote builds in B310543: Diff 426440.Apr 13 2021, 5:31 PM
phab-bot published this revision for review.Apr 13 2021, 5:33 PM
phab-bot changed the visibility from "Custom Policy" to "Public (No Login Required)".
phab-bot changed the edit policy from "Custom Policy" to "Restricted Project (Project)".
phab-bot removed a project: secure-revision.
Herald added a subscriber: emilio. · View Herald TranscriptApr 13 2021, 5:33 PM
cmartin added reviewers: bas, emilio.Apr 13 2021, 5:52 PM

Hey guys,

This change is mostly to remove usage of GfxInfo::GetInfo() in content, which uses Win32k APIs (which fail when Win32k Lockdown is enabled)

There are a couple more changes coming after this to remove another usage I found, and then finally to forbid the usage by adding a bunch of assertions.

Thanks!

cmartin added a project: testing-exception-unchanged (Doesn't change behavior for users).Apr 13 2021, 5:52 PM
emilio edited reviewers, added: jrmuizel; removed: emilio.Apr 13 2021, 10:52 PM
emilio added a subscriber: jrmuizel.

This looks sensible to me though I'm not the most familiar with this code. Maybe Jeff can review it? @jrmuizel if you don't have the context or cycles or what not feel free to put the patch back in my review queue and I'll do my best :-). Thanks!

layout/tools/reftest/reftest-content.js
1484

Doesn't this throw on non-windows?

jrmuizel resigned from this revision.Apr 13 2021, 11:16 PM

I'm on parental leave.

bas accepted this revision.Apr 14 2021, 10:27 PM
bas added inline comments.
layout/tools/reftest/reftest-content.js
1484

Agreed, this feels to me like this should be in a try-catch.

This revision is now accepted and ready to land.Apr 14 2021, 10:27 PM
cmartin updated this revision to Diff 427698.Apr 15 2021, 2:40 PM

Address review concerns

Harbormaster completed remote builds in B311548: Diff 427698.Apr 15 2021, 2:41 PM
cmartin marked 2 inline comments as done.Apr 15 2021, 2:43 PM
cmartin added inline comments.
layout/tools/reftest/reftest-content.js
1484

You both were absolutely right -- Good catch! I don't know why I made this stupid mistake, but these fields were never meant to *only* work on Windows -- They should always work on all platforms!

This actually makes this code change smaller, since now I can implement the function in GfxInfoBase instead of the individual platform implementations.

emilio accepted this revision.Apr 15 2021, 5:05 PM
emilio added inline comments.
layout/tools/reftest/reftest-content.js
1481

This change seems a bit unrelated, either change the commit message to cover it, or move it to another patch?

widget/GfxInfoBase.cpp
1814

CopyAsciiToUTF16 would be slightly more efficient I think, but not that it matters.

cmartin updated this revision to Diff 427883.Apr 15 2021, 6:44 PM
cmartin marked an inline comment as done.

Address more review concerns

Harbormaster completed remote builds in B311688: Diff 427883.Apr 15 2021, 6:44 PM
cmartin marked 2 inline comments as done.Apr 15 2021, 6:45 PM
cmartin added inline comments.
layout/tools/reftest/reftest-content.js
1481

Fair enough -- I will move it somewhere else and keep this change more focused.

widget/GfxInfoBase.cpp
1814

Didn't even know this existed. Done!

cmartin updated this revision to Diff 432201.Apr 27 2021, 1:13 PM
cmartin marked 2 inline comments as done.

Rebase

Harbormaster completed remote builds in B315186: Diff 432201.Apr 27 2021, 1:13 PM
cmartin added a child revision: D112187: Bug 1698732 - Remove all uses of GfxInfo::GetInfo() for Azure Backend.Apr 27 2021, 3:15 PM
Closed by commit rMOZILLACENTRALe201e698220e: Bug 1698732 - Change reftest-content to get Azure info without Win32k APIs… (authored by cmartin). · Explain WhyApr 27 2021, 3:58 PM
This revision was automatically updated to reflect the committed changes.
cmartin added a commit: rMOZILLACENTRALe201e698220e: Bug 1698732 - Change reftest-content to get Azure info without Win32k APIs….
Harbormaster completed remote builds in B315542: Diff 432280.Jul 26 2021, 8:34 PM

Revision Contents
Changeset List

PathSizeCoverage (All)Coverage (Touched)
gfx/
tests/
gtest/
8 lines87%
Loading...
thebes/
3 lines78%
Loading...
53 lines79%
Loading...
layout/
tools/
reftest/
5 lines--
6 lines--
widget/
2 lines8%
Loading...
16 lines73%
Loading...
2 lines--

Diff 432280

View Options

gfx/tests/gtest/TestConfigManager.cpp

Loading...
View Options

gfx/thebes/gfxPlatform.h

Loading...
View Options

gfx/thebes/gfxPlatform.cpp

Loading...
View Options

layout/tools/reftest/manifest.jsm

Loading...
View Options

layout/tools/reftest/reftest-content.js

Loading...
View Options

widget/GfxInfoBase.h

Loading...
View Options

widget/GfxInfoBase.cpp

Loading...
View Options

widget/nsIGfxInfo.idl

Loading...
Privacy · Cookies · Legal