I think we should be able to use filter serialization here for transporting filters on CompositorFrames before OOP raster. This is desired for removing quirks in skia for implementing security constraints (https://chromium-review.googlesource.com/c/chromium/src/+/759129) and a workaround that uses kSkImageFilter PaintFilter type for this use-case in the interim.
To have parity with current serialization of SkImageFilters, we only need to serialize filter types with primitives. The following ones which are security sensitive are handled as follows:
1) ImagePaintFilter: This has a PaintImage with encoded image data. We want to decode during serialization and serialize the bitmap.
2) RecordPaintFilter: Disabled as SkPictureImageFilter.
3) PaintFlagsPaintFilter: This actually warrants a security review, because it serializes the paint and thus potentially SkPictures with it.
For ImagePaintFilter, this would be serialized via the transfer cache in the same way that we plan (or do) serialize images.
For RecordPaintFilter, I'm not sure what you mean that it's disabled? We have the ability to serialize a paint record, so why wouldn't we be able to serialize this filter?
PaintFlagsPaintFilter: this would serialize PaintFlags in the same way that we currently serialize PaintFlags for everything else, so I'm not sure why a separate review is warranted.
In #6 I was talking about filter serialization for existing use case of filters on CompositorFrames. Right now we serialize the underlying SkImageFilter and create a kSkImageFilter PaintFilter type in the browser. This serialization is already security constrained to drop SkPictures and decode images in the renderer during serialization. Finally for OOP, we will be able to drop the constraint above and use PaintRecords. Also, transfer cache is only used with OOP right now?
My point was that if we want to drop using skia serialization for the existing use-case sooner than when all of PaintRecord serialization is ready, we can do that by disabling the same filter types from PaintFilter. vmiura@, is this a priority?
> My point was that if we want to drop using skia serialization for the existing
> use-case sooner than when all of PaintRecord serialization is ready, we can do
> that by disabling the same filter types from PaintFilter. vmiura@, is this a
> priority?
Yes I think we should prioritize this to unblock Skia / OOP-Printing progress. I don't think we need to support RecordPaintFilter in Compositor frames; it's unexpected in that context.
I'm concerned about what you mentioned of PaintFlagsPaintFilter serializing/deserializing SkPaints -- which would set us back to square 1 as far as allowing arbitrary SkPicture deserialization in the Browser process. Could we switch this to serialize PaintFlags with no embedded SkPaints?
I'm sorry about being a bit vague in #8. I meant that currently the PaintFlagsPaintFilter's skia equivalent is SkPaintImageFilter which has an SkPaint, and I wasn't sure whether that is hardened enough. It can internally have SkPictures in objects other than SkPictureImageFilter (like SkPictureShaders). And I wasn't sure if we disable picture serialization altogether or just in filters. Looks like we disable it with picture shaders as well (https://cs.chromium.org/chromium/src/third_party/skia/src/shaders/SkPictureShader.cpp?sq=package:chromium&l=184). The #define here is kinda misleading (https://cs.chromium.org/chromium/src/third_party/skia/src/core/SkPicture.cpp?type=cs&l=19). It also has SkTypeface (web fonts?) which we do serialize with SkPaints.
Do we need to similarly drop all PaintRecords in PaintFlags when serializing PaintFlagsPaintFilter for CompositorFrames? Typefaces are already ignored.
Re #11: Hmm, yes I think we need to block deserialization of complex Sk types in PaintFlags, or finish converting those to cc::Paint types if they're required.
So we ignore the typeface; which other PaintFlags fields are a concern? SkDrawLooper can be ignored too I think.
Ignoring PaintRecord, SkTypeface and SkDrawLooper (which has an SkPaint) should suffice I think, since these types can include font data which we definitely can not allow. At least for the purpose of switching away from skia here, we won't be increasing the serialization surface if we have these constraints.
Comment 1 by khushals...@chromium.org
, Oct 27 2017