[Mac] Add a new WebSandboxSupport method to get the display's color space.

Remove the Mac-specific path for getting the color space in ImageDecoder.

This path required a connection to the WindowServer, which should be avoided
(see the two bugs for details). Instead this data can be retrieved via the
ColorProfile class, which can access a value that has been cached during
pre-sandbox warmup.

BUG=397642, 306348
R=noel@chromium.org

Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=181761

[Avi (use Gerrit), 2014-09-08 20:14:56]

avi@chromium.org changed reviewers:
+ avi@chromium.org

[Avi (use Gerrit), 2014-09-08 20:14:56]

LGTM

https://codereview.chromium.org/554033002/diff/1/public/platform/mac/WebSandb...
File public/platform/mac/WebSandboxSupport.h (right):

https://codereview.chromium.org/554033002/diff/1/public/platform/mac/WebSandb...
public/platform/mac/WebSandboxSupport.h:59: // Returns the color space for the
current display. The caller does not own
And by "current" you mean "main".

It's rather a bummer that we can't properly support color-spaces per-screen.

[Ken Russell (switch to Gerrit), 2014-09-08 21:34:57]

LGTM, but I'm not an OWNER in public/ .

https://codereview.chromium.org/554033002/diff/1/public/platform/mac/WebSandb...
File public/platform/mac/WebSandboxSupport.h (right):

https://codereview.chromium.org/554033002/diff/1/public/platform/mac/WebSandb...
public/platform/mac/WebSandboxSupport.h:59: // Returns the color space for the
current display. The caller does not own
On 2014/09/08 20:14:56, Avi wrote:
> And by "current" you mean "main".
> 
> It's rather a bummer that we can't properly support color-spaces per-screen.

Noel's actively working on adding per-screen color profile support. This change
looks like an urgent fix for upcoming OS releases.

[Robert Sesek, 2014-09-08 21:36:34]

rsesek@chromium.org changed reviewers:
+ jamesr@chromium.org

[Robert Sesek, 2014-09-08 21:37:26]

https://codereview.chromium.org/554033002/diff/1/public/platform/mac/WebSandb...
File public/platform/mac/WebSandboxSupport.h (right):

https://codereview.chromium.org/554033002/diff/1/public/platform/mac/WebSandb...
public/platform/mac/WebSandboxSupport.h:59: // Returns the color space for the
current display. The caller does not own
On 2014/09/08 21:34:57, Ken Russell wrote:
> On 2014/09/08 20:14:56, Avi wrote:
> > And by "current" you mean "main".
> > 
> > It's rather a bummer that we can't properly support color-spaces per-screen.
> 
> Noel's actively working on adding per-screen color profile support. This
change
> looks like an urgent fix for upcoming OS releases.

That's cool! This should make it easier to pipe down, since the SandboxSupport
impl could be hooked up to a IPC message filter.

[Robert Sesek, 2014-09-09 13:51:53]

jamesr: Please review (sorry my initial message wasn't clear).

[jamesr, 2014-09-09 20:03:47]

I thought Noel was working on pushing this down to the renderer rather than
having it pulled.  Is that correct?

If we need a pull model then lgtm, but that seems unfortunate.

[Robert Sesek, 2014-09-09 20:18:49]

On 2014/09/09 20:03:47, jamesr wrote:
> I thought Noel was working on pushing this down to the renderer rather than
> having it pulled.  Is that correct?
> 
> If we need a pull model then lgtm, but that seems unfortunate.

This isn't incompatible with a push model: something in the renderer needs to
keep track of the color space. Here that's happening by pulling directlhy from
the WindowServer (and in base::mac::GetSystemColorSpace, the value is cached in
a static). But doing that is incompatible with a strong sandbox, which is the
motivation for this change. This callsite would have to change in a push model.
Now there's a bit more plumbing for that to happen.

[Robert Sesek, 2014-09-09 21:05:43]

The CQ bit was checked by rsesek@chromium.org

[commit-bot: I haz the power, 2014-09-09 21:06:39]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/rsesek@chromium.org/554033002/20001

[commit-bot: I haz the power, 2014-09-09 21:23:48]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-09-09 21:23:49]

Try jobs failed on following builders:
  mac_blink_rel on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/mac_blink_rel/builds/22762)

[Noel Gordon, 2014-09-10 07:45:13]

On 2014/09/08 20:14:56, Avi wrote:

> It's rather a bummer that we can't properly support color-spaces per-screen.

It's a bummer that chrome mac is particularly broken in handling of multiple
screens.  Could you help to fix it?

https://code.google.com/p/chromium/codesearch#chromium/src/content/browser/we...

Could we perhaps make that routine return the right thing when a tab / popup is
in background?

[Noel Gordon, 2014-09-10 07:59:34]

On 2014/09/08 21:37:26, rsesek wrote:
>
https://codereview.chromium.org/554033002/diff/1/public/platform/mac/WebSandb...
> File public/platform/mac/WebSandboxSupport.h (right):
> 
>
https://codereview.chromium.org/554033002/diff/1/public/platform/mac/WebSandb...
> public/platform/mac/WebSandboxSupport.h:59: // Returns the color space for the
> current display. The caller does not own
> On 2014/09/08 21:34:57, Ken Russell wrote:
> > On 2014/09/08 20:14:56, Avi wrote:
> > > And by "current" you mean "main".
> > > 
> > > It's rather a bummer that we can't properly support color-spaces
per-screen.
> > 
> > Noel's actively working on adding per-screen color profile support. This
> change
> > looks like an urgent fix for upcoming OS releases.
> 
> That's cool! This should make it easier to pipe down, since the SandboxSupport
> impl could be hooked up to a IPC message filter.

Added comments https://codereview.chromium.org/549213004 which this CL builds
on, and has already been submitted. There are issues over there (user privacy,
thread-safety, and so on) that you might want to address.

[Noel Gordon, 2014-09-10 08:02:13]

On 2014/09/09 20:03:47, jamesr wrote:
> I thought Noel was working on pushing this down to the renderer rather than
> having it pulled.  Is that correct?

Yeap correct, and I'm not sure this change helps or hinders me as yet.
 
> If we need a pull model then lgtm, but that seems unfortunate.

And the pull model was working.  Why do we need to fix it?

[Noel Gordon, 2014-09-10 08:05:55]

On 2014/09/09 21:23:49, I haz the power (commit-bot) wrote:
> Try jobs failed on following builders:
>   mac_blink_rel on tryserver.blink
>
(http://build.chromium.org/p/tryserver.blink/builders/mac_blink_rel/builds/22762)

Was worth my time writing those unit tests.

[Noel Gordon, 2014-09-10 08:27:47]

On 2014/09/09 20:18:49, rsesek wrote:
> On 2014/09/09 20:03:47, jamesr wrote:
> > I thought Noel was working on pushing this down to the renderer rather than
> > having it pulled.  Is that correct?
> > 
> > If we need a pull model then lgtm, but that seems unfortunate.
> 
> This isn't incompatible with a push model: something in the renderer needs to
> keep track of the color space.

This CL not a replacement for the working pull model we have either.  As
mentioned in https://codereview.chromium.org/554033002/, maybe we should
consider using gfx:ColorProfile ?

> Here that's happening by pulling directlhy from
> the WindowServer (and in base::mac::GetSystemColorSpace, the value is cached
in
> a static).
> But doing that is incompatible with a strong sandbox, which is the
> motivation for this change. 

Strong sandbox?  Do we have security issues with color profiles read this way? 
Clusterfuzz has a large suite of color profiled images and fuzzing code to match
to bang on these by decoding in them on web pages.  No issues at this time.

[Noel Gordon, 2014-09-10 08:46:26]

noel@chromium.org changed reviewers:
+ noel@chromium.org

[Noel Gordon, 2014-09-10 08:46:27]

https://codereview.chromium.org/554033002/diff/20001/Source/platform/image-de...
File Source/platform/image-decoders/ImageDecoder.h (right):

https://codereview.chromium.org/554033002/diff/20001/Source/platform/image-de...
Source/platform/image-decoders/ImageDecoder.h:203: CGColorSpaceRef
monitorColorSpace = Platform::current()->sandboxSupport()->displayColorSpace();
As asked on the other review, is this thread-safe?  This code could be called
from the blink main-thread, or on any impl-side-painting thread.  Maybe that's
why the unit tests are complaining ...

https://codereview.chromium.org/554033002/diff/20001/Source/platform/image-de...
Source/platform/image-decoders/ImageDecoder.h:214: screenColorProfile(profile);
This finds its  way to your https://codereview.chromium.org/549213004
codereview, right? 

Seems we might be able to delete all mac-specific stuff from ImageDecoder.h with
some more work in content/renderer/renderer_webkitplatformsupport_impl.cc, no?

[Robert Sesek, 2014-09-10 14:33:54]

On 2014/09/10 08:27:47, Noel Gordon wrote:
> On 2014/09/09 20:18:49, rsesek wrote:
> > On 2014/09/09 20:03:47, jamesr wrote:
> > > I thought Noel was working on pushing this down to the renderer rather
than
> > > having it pulled.  Is that correct?
> > > 
> > > If we need a pull model then lgtm, but that seems unfortunate.
> > 
> > This isn't incompatible with a push model: something in the renderer needs
to
> > keep track of the color space.
> 
> This CL not a replacement for the working pull model we have either.  As
> mentioned in https://codereview.chromium.org/554033002/, maybe we should
> consider using gfx:ColorProfile ?
> 
> > Here that's happening by pulling directlhy from
> > the WindowServer (and in base::mac::GetSystemColorSpace, the value is cached
> in
> > a static).
> > But doing that is incompatible with a strong sandbox, which is the
> > motivation for this change. 
> 
> Strong sandbox?  Do we have security issues with color profiles read this way?

> Clusterfuzz has a large suite of color profiled images and fuzzing code to
match
> to bang on these by decoding in them on web pages.  No issues at this time.

Color profiles themselves are not the problem; they're part of the system and
even if they were untrusted data, handling that in the renderer is fine. The
problem is the renderer is getting them here by directly connecting to the
WindowServer, which the unprivileged process should not have access to.
base::mac::GetSystemColorSpace() is called as part of sandbox warmup to cache
the color profile, so that's why we need to use that. screenColorProfile() seems
like it could work, too, so I'll try that.

[Noel Gordon, 2014-09-10 15:14:59]

Thanks for the details, screenColorProfile() should work, though we'd need to do
it stages I think.

One caveat, I'd might need the WindowServer in a mac renderer if we can't fix
the issue I mentioned in #14 (to support the push model). I hoping we can fix
it, please advise.

Assuming we can fix #14, here's a possible plan after I re-read crbug.com/314012
to remind me how we got here :)

1) Undo most of
https://chromium.googlesource.com/chromium/src/+/c27ec9229128b81006d8a610d086...
  - that change made the color profile IPC to the browser win-only ...
  - that cut-off mac, so the mac had to read the color profile indirectly from
the WindowServer
  - so let's remove the win-only restriction everwhere
  - except in content/renderer/renderer_webkitplatformsupport_impl.cc
     - therein allow MAC to make the IPC
     - #if defined(OS_WIN) -> #if defined(OS_WIN) || defined(OS_MAC)
     - fix comments
  - try land that change, I can review
     - check that the webkit unit and layout tests still work (they should) but
let's check.

2) Remove the mac-specific code from webkit ImageDecoder.h
  - make MAC call through to screenColorProfile() api like the other ports.
  - land that, I can review
     - there may be some layout test rebaselines needed here

Once done, we could try to re-land the change that shut downs the WindowServer
before enabling the sandbox. Sounds like a plan?

[Robert Sesek, 2014-09-10 15:27:51]

On 2014/09/10 15:14:59, Noel Gordon wrote:
> Thanks for the details, screenColorProfile() should work, though we'd need to
do
> it stages I think.
> 
> One caveat, I'd might need the WindowServer in a mac renderer if we can't fix
> the issue I mentioned in #14 (to support the push model). I hoping we can fix
> it, please advise.
> 
> Assuming we can fix #14, here's a possible plan after I re-read
crbug.com/314012
> to remind me how we got here :)
> 
> 1) Undo most of
>
https://chromium.googlesource.com/chromium/src/+/c27ec9229128b81006d8a610d086...
>   - that change made the color profile IPC to the browser win-only ...
>   - that cut-off mac, so the mac had to read the color profile indirectly from
> the WindowServer
>   - so let's remove the win-only restriction everwhere
>   - except in content/renderer/renderer_webkitplatformsupport_impl.cc
>      - therein allow MAC to make the IPC
>      - #if defined(OS_WIN) -> #if defined(OS_WIN) || defined(OS_MAC)
>      - fix comments
>   - try land that change, I can review
>      - check that the webkit unit and layout tests still work (they should)
but
> let's check.
> 
> 2) Remove the mac-specific code from webkit ImageDecoder.h
>   - make MAC call through to screenColorProfile() api like the other ports.
>   - land that, I can review
>      - there may be some layout test rebaselines needed here
> 
> Once done, we could try to re-land the change that shut downs the WindowServer
> before enabling the sandbox. Sounds like a plan?

That sounds like a good plan, however I don't think there's any reason why (1)
needs to be done before (2). The color profile returned by
base::mac::GetSystemColorSpace() will be cached prior to engaging the sandbox,
so it should be possible to do (2) right now and then shut down the WindowServer
connection. Adding (1) would then get us multi-monitor color profile support,
which I believe has never worked on Mac.

There's another approach to getting the color space though, via a push model.
Since color spaces are set per-display, the BrowserWindow could listen for a
system notification when the window moves between screens. When this occurs, the
browser would send an IPC message to the corresponding renderer process(es) with
the new profile data.

[Noel Gordon, 2014-09-10 15:50:59]

On 2014/09/10 15:27:51, rsesek wrote:
 
> That sounds like a good plan, however I don't think there's any reason why (1)
> needs to be done before (2). The color profile returned by
> base::mac::GetSystemColorSpace() will be cached prior to engaging the sandbox,
> so it should be possible to do (2) right now and then shut down the
WindowServer
> connection. 

My concern was a two-sided patch, one in chrome, then one in blink, ad not
breaking tests.

If you can do it as you suggest, sounds good to me. So long as we provide the
primary
monitor profile to blink when it asks via screenColorProfile().

> Adding (1) would then get us multi-monitor color profile support,
> which I believe has never worked on Mac.

Maybe, not sure that IPC knows which screen or window to use to request the
color profile though.

[Noel Gordon, 2014-09-10 16:58:39]

On 2014/09/10 15:27:51, rsesek wrote:

Anyho, the color profile IPC is for the pull model, which worked for / wanted
the primary screen color profile only. The pull model will still need to work in
some case, even if we have a multi-monitor-aware push model.

> There's another approach to getting the color space though, via a push model.
> Since color spaces are set per-display, the BrowserWindow could listen for a
> system notification when the window moves between screens. 

renderers host-side have
  render_widget_host_impl::NotifyScreenInfoChanged()

on mac we have renderer_host/render_widget_host_view_mac.mm move notifications
- (void)windowDidChangeBackingProperties:(NSNotification*)notification { ...

they seem a bit laggy, as if they're on a timer.  We also need the color profile
when a tab NSView is created, again from
renderer_host/render_widget_host_view_mac.mm, something like 

void RenderWidgetHostViewMac::GetScreenInfo(blink::WebScreenInfo* results) {
  *results = GetWebScreenInfo(GetNativeView());
}

GetNativeView() is the tab NSView.  For background tabs, [GetNativeView()
window] is nil though, and that causes GetWebScreenInfo() to return primary
monitor info :)  That's the wrong info to send to blink if the NSView is on a
different monitor (files as http://crbug.com/357443).

Is there some simple way to find the browser native window (NSWindow) the tab
GetNativeView() is in?

> When this occurs, the browser would send an IPC message to the corresponding
renderer process(es) with
> the new profile data.

sgtm https://code.google.com/p/chromium/issues/detail?id=369787#c9

[Robert Sesek, 2014-09-10 17:01:26]

Patchset #3 (id:40001) has been deleted

[Robert Sesek, 2014-09-10 17:24:48]

On 2014/09/10 16:58:39, Noel Gordon wrote:
> On 2014/09/10 15:27:51, rsesek wrote:
> 
> Anyho, the color profile IPC is for the pull model, which worked for / wanted
> the primary screen color profile only. The pull model will still need to work
in
> some case, even if we have a multi-monitor-aware push model.
> 
> > There's another approach to getting the color space though, via a push
model.
> > Since color spaces are set per-display, the BrowserWindow could listen for a
> > system notification when the window moves between screens. 
> 
> renderers host-side have
>   render_widget_host_impl::NotifyScreenInfoChanged()
> 
> on mac we have renderer_host/render_widget_host_view_mac.mm move notifications
> - (void)windowDidChangeBackingProperties:(NSNotification*)notification { ...
> 
> they seem a bit laggy, as if they're on a timer.

Cool, this is precisely to what I was referring. I wouldn't worry about a bit of
lag, IMO.

> We also need the color profile
> when a tab NSView is created, again from
> renderer_host/render_widget_host_view_mac.mm, something like 
> 
> void RenderWidgetHostViewMac::GetScreenInfo(blink::WebScreenInfo* results) {
>   *results = GetWebScreenInfo(GetNativeView());
> }
> 
> GetNativeView() is the tab NSView.  For background tabs, [GetNativeView()
> window] is nil though, and that causes GetWebScreenInfo() to return primary
> monitor info :)  That's the wrong info to send to blink if the NSView is on a
> different monitor (files as http://crbug.com/357443).
> 
> Is there some simple way to find the browser native window (NSWindow) the tab
> GetNativeView() is in?

The view must have been removed from the hierarchy, so it's no longer in a
window. In that case, asking the RWHV's embedder/parent would work. It looks
like RWHV also tracks a lastWindow_.

[Noel Gordon, 2014-09-10 18:19:26]

On 2014/09/10 17:24:48, rsesek wrote:
> > 
> > on mac we have renderer_host/render_widget_host_view_mac.mm move
notifications
> > - (void)windowDidChangeBackingProperties:(NSNotification*)notification { ...
> > 
> > they seem a bit laggy, as if they're on a timer.
> 
> Cool, this is precisely to what I was referring. I wouldn't worry about a bit
of
> lag, IMO.

I worry about lag / rendering on mac. This is a chrome mac window crossing
screens. The image has no color profile, but the rendering is ... well ...
strange to me. 

https://www/~noel/color/cross/chrome.mp4

Adding push model color profiles on top of that is a challenge.  Browser,
renderer, blink all have to play nice and co-ordinate better to make it smooth.

[Robert Sesek, 2014-09-10 18:23:29]

On 2014/09/10 18:19:26, Noel Gordon wrote:
> On 2014/09/10 17:24:48, rsesek wrote:
> > > 
> > > on mac we have renderer_host/render_widget_host_view_mac.mm move
> notifications
> > > - (void)windowDidChangeBackingProperties:(NSNotification*)notification {
...
> > > 
> > > they seem a bit laggy, as if they're on a timer.
> > 
> > Cool, this is precisely to what I was referring. I wouldn't worry about a
bit
> of
> > lag, IMO.
> 
> I worry about lag / rendering on mac. This is a chrome mac window crossing
> screens. The image has no color profile, but the rendering is ... well ...
> strange to me. 
> 
> https://www/~noel/color/cross/chrome.mp4
> 
> Adding push model color profiles on top of that is a challenge.  Browser,
> renderer, blink all have to play nice and co-ordinate better to make it
smooth.

Fair enough. Keeping it pull is less plumbing, anyways :).

PTAL at patch set 3. This passes blink tests (which does exercise color
profiles), and they pass locally on my machine with the WindowServer
disconnection patch applied.

I believe this change should be a no-op behaviorally. Previously this code was
getting the color space for the main display, which is also what
base::mac::GetSystemColorSpace() returns, and that is what gfx::ColorSpace uses.
And that color space is cached in pre-sandbox warmup, so it should always be
available in the renderer.

Next steps for me would be to lock down WindowServer (which is necessary to fix
a Yosemite bug), and then I can do step (1) of your above plan.

[Noel Gordon, 2014-09-10 18:27:24]

On 2014/09/10 17:24:48, rsesek wrote:
> On 2014/09/10 16:58:39, Noel Gordon wrote:

> > Is there some simple way to find the browser native window (NSWindow) the
tab
> > GetNativeView() is in?
> 
> The view must have been removed from the hierarchy, so it's no longer in a
> window. 

All tabs have an NSview; only the active tab has an NSWindow it seems.  When a
background tab become active, it is assigned to the NSWindow, and the formerly
active tab loses it, best I can tell.

> In that case, asking the RWHV's embedder/parent would work. It looks
> like RWHV also tracks a lastWindow_.

Many thanks for this answer, I will try that.

[Noel Gordon, 2014-09-10 18:28:26]

And patch lgtm btw :)

[Robert Sesek, 2014-09-10 18:37:17]

On 2014/09/10 18:27:24, Noel Gordon wrote:
> On 2014/09/10 17:24:48, rsesek wrote:
> > On 2014/09/10 16:58:39, Noel Gordon wrote:
> 
> > > Is there some simple way to find the browser native window (NSWindow) the
> tab
> > > GetNativeView() is in?
> > 
> > The view must have been removed from the hierarchy, so it's no longer in a
> > window. 
> 
> All tabs have an NSview; only the active tab has an NSWindow it seems.  When a
> background tab become active, it is assigned to the NSWindow, and the formerly
> active tab loses it, best I can tell.

That makes sense. I haven't looked at this code in a while, but my guess is that
the NSView associated from the tab is removed from its superview, which takes it
out of the window. The active tab is then inserted into the view hierarchy where
the now-removed tab was. That way extraneous views don't draw when the window
requests it.

[Noel Gordon, 2014-09-10 18:46:02]

On 2014/09/10 18:23:29, rsesek wrote:
> On 2014/09/10 18:19:26, Noel Gordon wrote:
> > On 2014/09/10 17:24:48, rsesek wrote:
> > > > 
> > > > on mac we have renderer_host/render_widget_host_view_mac.mm move
> > notifications
> > > > - (void)windowDidChangeBackingProperties:(NSNotification*)notification {
> ...
> > > > 
> > > > they seem a bit laggy, as if they're on a timer.
> > > 
> > > Cool, this is precisely to what I was referring. I wouldn't worry about a
> bit
> > of
> > > lag, IMO.
> > 
> > I worry about lag / rendering on mac. This is a chrome mac window crossing
> > screens. The image has no color profile, but the rendering is ... well ...
> > strange to me. 
> > 
> > https://www/~noel/color/cross/chrome.mp4
> > 
> > Adding push model color profiles on top of that is a challenge.  Browser,
> > renderer, blink all have to play nice and co-ordinate better to make it
> smooth.
> 
> Fair enough. Keeping it pull is less plumbing, anyways :).
>
> PTAL at patch set 3. This passes blink tests (which does exercise color
> profiles), and they pass locally on my machine with the WindowServer
> disconnection patch applied.

Thankyou for testing.
 
> I believe this change should be a no-op behaviorally. Previously this code was
> getting the color space for the main display, which is also what
> base::mac::GetSystemColorSpace() returns, and that is what gfx::ColorSpace
uses.
> And that color space is cached in pre-sandbox warmup, so it should always be
> available in the renderer.

sgtm.
 
> Next steps for me would be to lock down WindowServer (which is necessary to
fix
> a Yosemite bug), and then I can do step (1) of your above plan.

Given what you've said, tests passing etc, we can skip 1) I think.

[Robert Sesek, 2014-09-10 20:01:43]

The CQ bit was checked by rsesek@chromium.org

[commit-bot: I haz the power, 2014-09-10 20:02:24]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/rsesek@chromium.org/554033002/60001

[commit-bot: I haz the power, 2014-09-10 20:12:02]

Committed patchset #3 (id:60001) as 181761