Retry GDI font failure when sandbox is in place

Retry GDI font failure when sandbox is in place


BUG=none
TEST=none

Closed without commit, use Ben's version
http://codereview.appspot.com/5379043

[arthurhsu, 2011-11-15 04:28:11]

This is the skia part, for Chromium part, see
http://codereview.chromium.org/8566026/

[reed1, 2011-11-15 13:09:16]

http://codereview.appspot.com/5364052/diff/1/include/ports/SkSandbox_win.h
File include/ports/SkSandbox_win.h (right):

http://codereview.appspot.com/5364052/diff/1/include/ports/SkSandbox_win.h#ne...
include/ports/SkSandbox_win.h:18: public:
lower-case 'e'
const LOGFONT&

[Steve VanDeBogart, 2011-11-15 17:02:04]

http://codereview.appspot.com/5364052/diff/1/include/ports/SkSandbox_win.h
File include/ports/SkSandbox_win.h (right):

http://codereview.appspot.com/5364052/diff/1/include/ports/SkSandbox_win.h#ne...
include/ports/SkSandbox_win.h:17: class SkSandbox {
There's no reason for this to be a class.

http://codereview.appspot.com/5364052/diff/1/src/pdf/SkPDFFont.cpp
File src/pdf/SkPDFFont.cpp (right):

http://codereview.appspot.com/5364052/diff/1/src/pdf/SkPDFFont.cpp#newcode553
src/pdf/SkPDFFont.cpp:553: if (fontData == NULL) {
Lets not paper over any failures until we understand them all.

http://codereview.appspot.com/5364052/diff/1/src/ports/SkFontHost_win.cpp
File src/ports/SkFontHost_win.cpp (right):

http://codereview.appspot.com/5364052/diff/1/src/ports/SkFontHost_win.cpp#new...
src/ports/SkFontHost_win.cpp:1184: size_t bufferSize = GetFontData(hdc,
tables[i], 0, NULL, 0);
There are other calls to GetFontData, we should probably add a local function
that wraps the retry logic.  Are there other functions that should be similarly
wrapped?

[bungeman, 2011-11-15 18:41:38]

On 2011/11/15 04:28:11, arthurhsu wrote:
> This is the skia part, for Chromium part, see
> http://codereview.chromium.org/8566026/

I do not believe this is the way to go about this. Please see
http://codereview.appspot.com/5379043 .

[arthurhsu, 2011-11-15 18:56:20]

Can you elaborate a bit how this approach works when Skia is in the
sandboxed process?  The moment GDI call fails, recreating a handle for that
won't succeed either.  Sandboxed process does not have file access
privilege and that's why it's relayed to the browser process to load the
font again.

On Tue, Nov 15, 2011 at 10:41 AM, <bungeman@google.com> wrote:

> On 2011/11/15 04:28:11, arthurhsu wrote:
>
>> This is the skia part, for Chromium part, see
>>
http://codereview.chromium.**org/8566026/<http://codereview.chromium.org/8566...
>>
>
> I do not believe this is the way to go about this. Please see
>
http://codereview.appspot.com/**5379043<http://codereview.appspot.com/5379043>.
>
>
http://codereview.appspot.com/**5364052/<http://codereview.appspot.com/5364052/>
>

[bungeman, 2011-11-15 19:16:06]

In this case on Windows, the 'handle' is the LOGFONT. No handle is recreated.
The way this would work in Chrome is that the Chrome gyp would not add
SkFontHost_sandbox_none.cpp. Instead Chrome would have its own
skia/ext/SkFontHost_sandbox_win.cpp which would then make the IPC to get GDI to
remap the font data into system memory.

The advantage is that Skia will always be the one kicking off this process on
failure cases only, so this will allow the IPC to happen on both the printing
and non-printing paths.

On 2011/11/15 18:56:20, arthurhsu wrote:
> Can you elaborate a bit how this approach works when Skia is in the
> sandboxed process?  The moment GDI call fails, recreating a handle for that
> won't succeed either.  Sandboxed process does not have file access
> privilege and that's why it's relayed to the browser process to load the
> font again.
> 
> On Tue, Nov 15, 2011 at 10:41 AM, <mailto:bungeman@google.com> wrote:
> 
> > On 2011/11/15 04:28:11, arthurhsu wrote:
> >
> >> This is the skia part, for Chromium part, see
> >>
>
http://codereview.chromium.**org/8566026/%3Chttp://codereview.chromium.org/85...>
> >>
> >
> > I do not believe this is the way to go about this. Please see
> >
>
http://codereview.appspot.com/**5379043%3Chttp://codereview.appspot.com/5379043>.
> >
> >
>
http://codereview.appspot.com/**5364052/%3Chttp://codereview.appspot.com/5364...>
> >

[arthurhsu, 2011-11-15 21:42:51]

On 2011/11/15 19:16:06, bungeman wrote:
> In this case on Windows, the 'handle' is the LOGFONT. No handle is recreated.
> The way this would work in Chrome is that the Chrome gyp would not add
> SkFontHost_sandbox_none.cpp. Instead Chrome would have its own
> skia/ext/SkFontHost_sandbox_win.cpp which would then make the IPC to get GDI
to
> remap the font data into system memory.
> 
> The advantage is that Skia will always be the one kicking off this process on
> failure cases only, so this will allow the IPC to happen on both the printing
> and non-printing paths.
> 
> On 2011/11/15 18:56:20, arthurhsu wrote:
> > Can you elaborate a bit how this approach works when Skia is in the
> > sandboxed process?  The moment GDI call fails, recreating a handle for that
> > won't succeed either.  Sandboxed process does not have file access
> > privilege and that's why it's relayed to the browser process to load the
> > font again.
> > 
> > On Tue, Nov 15, 2011 at 10:41 AM, <mailto:bungeman@google.com> wrote:
> > 
> > > On 2011/11/15 04:28:11, arthurhsu wrote:
> > >
> > >> This is the skia part, for Chromium part, see
> > >>
> >
>
http://codereview.chromium.**org/8566026/%253Chttp://codereview.chromium.org/...>
> > >>
> > >
> > > I do not believe this is the way to go about this. Please see
> > >
> >
>
http://codereview.appspot.com/**5379043%253Chttp://codereview.appspot.com/537...>.
> > >
> > >
> >
>
http://codereview.appspot.com/**5364052/%253Chttp://codereview.appspot.com/53...>
> > >

LGTM

Please commit and I can start from here.