1540776 - Move GetICMProfileW calls from content to parent

Status: RESOLVED FIXED

(Core :: Graphics, enhancement, P1)

Description

[Jim Mathies [:jimm]]

This api use needs to move to parent.

Comment 1

[Jim Mathies [:jimm]]

Attachment: wip - reorg cms profile routines

Comment 2

[Jim Mathies [:jimm]]

Attachment: wip - reorg cms profile routines

Comment 3

[Jim Mathies [:jimm]]

Attachment: Bug 1540776 - Reorganize cms functions to collect color data in the parent. r=jrmuizel

Comment 4

[Jim Mathies [:jimm]]

Attachment: Bug 1540776 - Store GTK color management data in an nsTArray instead of a raw buffer. r=gcp

Depends on D25810

Comment 5

[Jim Mathies [:jimm]]

Attachment: Bug 1540776 - Store OSX color management data in an nsTArray instead of a raw buffer. r=haik

Depends on D25811

Comment 6

[Jeff Muizelaar [:jrmuizel]]

I will try to get to this tomorrow.

Comment 7

[Chris Martin [:cmartin]]

Attachment: Bug 1540776 - Change CMSOutputProfile functions to return nsArray

Currently, the GetCMSOutputProfile() and related methods pass their output
using the old C-style "ptr, len" parameters. This makes them more difficult
to deal with later in this change when they need to be safely passed over IPC.

This refactors them to return nsTArray<uint8_t> results instead. I also
removed some old cruft, refactored the code, and fixed a bug that has gone
unnoticed for a long time.

Comment 8

[Chris Martin [:cmartin]]

This change is needed for Win32k lowndown. I have the refactoring change above, and I will add the color profile code shortly.

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/504aca5e5357
Change CMSOutputProfile functions to return nsArray r=gcp

Comment 10

[Daniel Varga [:dvarga]]

https://hg.mozilla.org/mozilla-central/rev/504aca5e5357

Comment 11

[Chris Martin [:cmartin]]

Oops - Forgot to set as leave-open. There's a few more changes on their way for this.

Comment 12

[Chris Martin [:cmartin]]

Attachment: Bug 1540776 - Move common CMS logic to platform-specific logic

In the content process, Windows won't perform any of the common logic for
CMS profile loading anymore. That means that it is no longer "common logic",
and that profile loading is now always platform-specific.

This change refactors out the "common logic" into a callable function that
the platform-specific functions can call if they want to. Soon, the Windows
logic won't call it in the content process.

Comment 13

[Chris Martin [:cmartin]]

Attachment: Bug 1540776 - Collect CMS data in parent process and pass to child

Once Win32k lockdown is enabled, we can't call GetICMProfileW() in the child,
nor can we access whatever arbitrary file
"gfx.color_management.display_profile" points to.

As a result, we must collect the data from these files in the parent process
and pass it to child.

Depends on D64674

Comment 14

[Chris Martin [:cmartin]]

Attachment: Bug 1540776 - Have parent send color profile to child during launch

For Win32k lockdown, we need to remove the content processes' ability to
call GetICMProfileW(). Since it needs this to retrieve the output color
profile, a new synchronous call is added that allows it to request the
parent process to read this file on its behalf.

The contents of the file are now being cached as well, as this should help
ease some of the increased parent process I/O caused by the children not
being able to do this in their process anymore.

Comment 15

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/18c3c5e79f1c
Add sync IPC message for content to request color profile r=aosmond,jld

Comment 16

[Jonathan Kew [:jfkthame]]

Does every child do this (e.g. as part of its gfxPlatform initialization), or is it done only on-demand (and if so, how common is that)? I'm wondering because of the risk that the request to the parent could block for a considerable length of time if the parent process is busy.

Comment 17

[Chris Martin [:cmartin]]

(In reply to Jonathan Kew (:jfkthame) from comment #16)

Does every child do this (e.g. as part of its gfxPlatform initialization), or is it done only on-demand (and if so, how common is that)? I'm wondering because of the risk that the request to the parent could block for a considerable length of time if the parent process is busy.

You're right that this is done by every child process as part of gfxPlatform initialization, and whenever a pref update forces a reload of the color profile. In normal circumstances, that's once per child process. The main motivation for this message existing is the possibility of a pref change forcing a reload.

Comment 18

[Jonathan Kew [:jfkthame]]

I'm concerned, then, that we might run into the same issue as we had with the system font list on Android, and recently fixed in bug 1620111. Basically, we were using a sync IPC message to get the list of the fonts from the parent process, but this could cause child startup to block for hundreds of milliseconds if the parent was busy at that time. So we just removed that sync message and instead of "pulling" the data by sending a message back to the parent during child startup -- which the parent might not respond to for some time -- we instead "push" the data as part of the XPCOMProcessAttributes message when launching the child.

Having just eliminated that blocking message from child-process startup, it seems unfortunate to introduce a new one which may suffer from the same issue and reintroduce similar child-startup delay. Could we instead attach the color profile data to the process-attributes message, and avoid the extra sync-ipc round-trip?

Comment 19

[Alexandru Michis [:malexandru]]

Backed out changeset 18c3c5e79f1c for causing xpcshell failures in test_BHRObserver.js

Backout link: https://hg.mozilla.org/integration/autoland/rev/17adc9cb1db942af888d6db8ed5141dde2d8a1e9

Push with failures: https://treeherder.mozilla.org/?#/jobs?repo=autoland&group_state=expanded&searchStr=windows%2C10%2Cx64%2Cshippable%2Copt%2Cxpcshell%2Ctests%2Ctest-windows10-64-shippable%2Fopt-xpcshell-e10s-1%2Cx%28x1%29&tochange=18c3c5e79f1c121f29e851362ec19831e8d5f82f&fromchange=d72cc8e3d9496e2b4dbc4e98800d0c25463e0c37&selectedJob=293407452

Failure log: https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=293407451&repo=autoland&lineNumber=5023

Comment 20

[Chris Martin [:cmartin]]

Updated review to address the issue brought up by :jfkthame, and also fix this XPCShell timeout failure

Comment 21

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/8b091426bc10
Have parent send color profile to child during launch r=aosmond,jld,jfkthame

Comment 22

[Razvan Maries]

Backed out for build bustages and perma failures.

Push with failure: https://treeherder.mozilla.org/#/jobs?repo=autoland&resultStatus=pending%2Crunning%2Csuccess%2Ctestfailed%2Cbusted%2Cexception&searchStr=linux%2Cx64%2Cdebug%2Cbuild-linux64%2Fdebug%2C%28b%29&revision=8b091426bc102a132d665e967da50f870f1893fa&selectedJob=294872536

Failure log: https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=294872536&repo=autoland&lineNumber=3900
https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=294876738&repo=autoland&lineNumber=575

Backout: https://hg.mozilla.org/integration/autoland/rev/3d5fa5fe3e1417a3812d9211ac93ab617a061c46

Comment 23

[Chris Martin [:cmartin]]

Investigating - Will likely need a slight refactor.

Comment 24

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/9087564e9fea
Have parent send color profile to child during launch r=aosmond,jld,jfkthame,florian

Comment 25

[Andrei Ciure[:aciure]]

https://hg.mozilla.org/mozilla-central/rev/9087564e9fea

Comment 26

[BugBot [:suhaib / :marco/ :calixte]]

Since the status are different for nightly and release, what's the status for beta?
For more information, please visit auto_nag documentation.