Implementing OffscreenCanvas
Hi all, I've spent the last month or so 'finishing' the implementation of OffscreenCanvas[1], based on Žan Doberšek's work from a year ago[2]. OffscreenCanvas is an API for being able to use canvas drawing without a visible canvas, and from within Workers. It's supported by Blink and has partial support in Gecko. It's at the point now where I'd consider it a finished draft - it is almost fully implemented and passes the majority of relevant tests in a debug build without crashing, but has some areas that need completion on other platforms (async drawing on non-Linux) and some missing parts (Web Inspector, ImageBitmapRenderingContext). It almost certainly needs reworking in places. My work is on GitHub[3] - I'd like to solicit reviews and comment. Some of the bugs hanging off [2] have patches that need review and I think are near ready to being landable as the foundation of this work. It is broadly split up like so: - Refactor to move functionality from HTMLCanvasElement to CanvasBase - Refactor to not unnecessarily require HTMLCanvasElement in places - Implement OffscreenCanvas functionality - Make font loading/styling usable from a Worker and without a Document - Implement AnimationFrameProvider on DedicatedWorkerGlobalScope - Implement asynchronous drawing updates on placeholder canvases I expect the font-related stuff to be the most contentious, and my AnimationFrameProvider implementation may be too trivial (but might be ok for a first go?) All feedback appreciated. Best regards, Chris [1] https://html.spec.whatwg.org/multipage/canvas.html#the-offscreencanvas-inter... [2] https://bugs.webkit.org/show_bug.cgi?id=183720 [3] https://github.com/Cwiiis/webkit/tree/offscreen-canvas
Hi Chris! Canvas is a very popular GPU fingerprinting vector and allowing it offscreen sounds like a more convenient way to perform such an attack on user privacy. Do you know if Blink or Gecko have elaborated on this? What is your assessment? Given the cross-engine effort to fight device fingerprinting and WebKit and Gecko’s recently published tracking prevention policies, we should do a threat analysis of this feature. Regards, John
On Oct 10, 2019, at 4:24 AM, Chris Lord <clord@igalia.com> wrote:
Hi all,
I've spent the last month or so 'finishing' the implementation of OffscreenCanvas[1], based on Žan Doberšek's work from a year ago[2]. OffscreenCanvas is an API for being able to use canvas drawing without a visible canvas, and from within Workers. It's supported by Blink and has partial support in Gecko.
It's at the point now where I'd consider it a finished draft - it is almost fully implemented and passes the majority of relevant tests in a debug build without crashing, but has some areas that need completion on other platforms (async drawing on non-Linux) and some missing parts (Web Inspector, ImageBitmapRenderingContext). It almost certainly needs reworking in places.
My work is on GitHub[3] - I'd like to solicit reviews and comment. Some of the bugs hanging off [2] have patches that need review and I think are near ready to being landable as the foundation of this work. It is broadly split up like so:
- Refactor to move functionality from HTMLCanvasElement to CanvasBase - Refactor to not unnecessarily require HTMLCanvasElement in places - Implement OffscreenCanvas functionality - Make font loading/styling usable from a Worker and without a Document - Implement AnimationFrameProvider on DedicatedWorkerGlobalScope - Implement asynchronous drawing updates on placeholder canvases
I expect the font-related stuff to be the most contentious, and my AnimationFrameProvider implementation may be too trivial (but might be ok for a first go?)
All feedback appreciated. Best regards,
Chris
[1] https://html.spec.whatwg.org/multipage/canvas.html#the-offscreencanvas-inter... [2] https://bugs.webkit.org/show_bug.cgi?id=183720 [3] https://github.com/Cwiiis/webkit/tree/offscreen-canvas _______________________________________________ webkit-dev mailing list webkit-dev@lists.webkit.org https://lists.webkit.org/mailman/listinfo/webkit-dev
Hi John, I don't know what the current state is of counter-measures for such an attack, but I don't immediately imagine OffscreenCanvas would make them more effective. The patch series doesn't add any new rendering paths, so whatever was possible before will likely still be possible and whatever wasn't will hopefully still not be possible. That said, I'll look into this and discuss it with some people that will know better than me and try to get a better picture. Thanks, Chris On 2019-10-10 17:32, John Wilander wrote:
Hi Chris!
Canvas is a very popular GPU fingerprinting vector and allowing it offscreen sounds like a more convenient way to perform such an attack on user privacy. Do you know if Blink or Gecko have elaborated on this? What is your assessment?
Given the cross-engine effort to fight device fingerprinting and WebKit and Gecko’s recently published tracking prevention policies, we should do a threat analysis of this feature.
Regards, John
On Oct 10, 2019, at 4:24 AM, Chris Lord <clord@igalia.com> wrote:
Hi all,
I've spent the last month or so 'finishing' the implementation of OffscreenCanvas[1], based on Žan Doberšek's work from a year ago[2]. OffscreenCanvas is an API for being able to use canvas drawing without a visible canvas, and from within Workers. It's supported by Blink and has partial support in Gecko.
It's at the point now where I'd consider it a finished draft - it is almost fully implemented and passes the majority of relevant tests in a debug build without crashing, but has some areas that need completion on other platforms (async drawing on non-Linux) and some missing parts (Web Inspector, ImageBitmapRenderingContext). It almost certainly needs reworking in places.
My work is on GitHub[3] - I'd like to solicit reviews and comment. Some of the bugs hanging off [2] have patches that need review and I think are near ready to being landable as the foundation of this work. It is broadly split up like so:
- Refactor to move functionality from HTMLCanvasElement to CanvasBase - Refactor to not unnecessarily require HTMLCanvasElement in places - Implement OffscreenCanvas functionality - Make font loading/styling usable from a Worker and without a Document - Implement AnimationFrameProvider on DedicatedWorkerGlobalScope - Implement asynchronous drawing updates on placeholder canvases
I expect the font-related stuff to be the most contentious, and my AnimationFrameProvider implementation may be too trivial (but might be ok for a first go?)
All feedback appreciated. Best regards,
Chris
[1] https://html.spec.whatwg.org/multipage/canvas.html#the-offscreencanvas-inter... [2] https://bugs.webkit.org/show_bug.cgi?id=183720 [3] https://github.com/Cwiiis/webkit/tree/offscreen-canvas _______________________________________________ webkit-dev mailing list webkit-dev@lists.webkit.org https://lists.webkit.org/mailman/listinfo/webkit-dev
For clarity, it’s already possible to render to a regular canvas offscreen. The <canvas> can be hidden using any of the techniques that can make any other canvas invisible. Name notwithstanding, OffscreenCanvas is mainly about being able to render from a Worker, not about enabling rendering offscreen. Thus, I would not expect it to make it easier to invisibly fingerprint using canvas.
On Oct 10, 2019, at 9:32 AM, Chris Lord <clord@igalia.com> wrote:
Hi John,
I don't know what the current state is of counter-measures for such an attack, but I don't immediately imagine OffscreenCanvas would make them more effective. The patch series doesn't add any new rendering paths, so whatever was possible before will likely still be possible and whatever wasn't will hopefully still not be possible. That said, I'll look into this and discuss it with some people that will know better than me and try to get a better picture.
Thanks,
Chris
On 2019-10-10 17:32, John Wilander wrote:
Hi Chris!
Canvas is a very popular GPU fingerprinting vector and allowing it offscreen sounds like a more convenient way to perform such an attack on user privacy. Do you know if Blink or Gecko have elaborated on this? What is your assessment?
Given the cross-engine effort to fight device fingerprinting and WebKit and Gecko’s recently published tracking prevention policies, we should do a threat analysis of this feature.
Regards, John
On Oct 10, 2019, at 4:24 AM, Chris Lord <clord@igalia.com> wrote:
Hi all,
I've spent the last month or so 'finishing' the implementation of OffscreenCanvas[1], based on Žan Doberšek's work from a year ago[2]. OffscreenCanvas is an API for being able to use canvas drawing without a visible canvas, and from within Workers. It's supported by Blink and has partial support in Gecko.
It's at the point now where I'd consider it a finished draft - it is almost fully implemented and passes the majority of relevant tests in a debug build without crashing, but has some areas that need completion on other platforms (async drawing on non-Linux) and some missing parts (Web Inspector, ImageBitmapRenderingContext). It almost certainly needs reworking in places.
My work is on GitHub[3] - I'd like to solicit reviews and comment. Some of the bugs hanging off [2] have patches that need review and I think are near ready to being landable as the foundation of this work. It is broadly split up like so:
- Refactor to move functionality from HTMLCanvasElement to CanvasBase - Refactor to not unnecessarily require HTMLCanvasElement in places - Implement OffscreenCanvas functionality - Make font loading/styling usable from a Worker and without a Document - Implement AnimationFrameProvider on DedicatedWorkerGlobalScope - Implement asynchronous drawing updates on placeholder canvases
I expect the font-related stuff to be the most contentious, and my AnimationFrameProvider implementation may be too trivial (but might be ok for a first go?)
All feedback appreciated. Best regards,
Chris
[1] https://html.spec.whatwg.org/multipage/canvas.html#the-offscreencanvas-inter... [2] https://bugs.webkit.org/show_bug.cgi?id=183720 [3] https://github.com/Cwiiis/webkit/tree/offscreen-canvas _______________________________________________ webkit-dev mailing list webkit-dev@lists.webkit.org https://lists.webkit.org/mailman/listinfo/webkit-dev
webkit-dev mailing list webkit-dev@lists.webkit.org https://lists.webkit.org/mailman/listinfo/webkit-dev
On Oct 10, 2019, at 9:42 AM, Maciej Stachowiak <mjs@apple.com> wrote:
For clarity, it’s already possible to render to a regular canvas offscreen. The <canvas> can be hidden using any of the techniques that can make any other canvas invisible. Name notwithstanding, OffscreenCanvas is mainly about being able to render from a Worker, not about enabling rendering offscreen.
Thus, I would not expect it to make it easier to invisibly fingerprint using canvas.
My thinking here is that viable mitigations for device fingerprinting could be requiring a Canvas to be visible on screen or even for it to get user interaction before it’ll get access to hardware acceleration. Such restrictions would make it harder for tracking scripts to get away with GPU fingerprinting that the site owner never wanted or accepted. If we allow offscreen Canvas in workers, site owners will stay in the dark and we can’t tie user interaction to it. Regards, John
On Oct 10, 2019, at 9:32 AM, Chris Lord <clord@igalia.com> wrote:
Hi John,
I don't know what the current state is of counter-measures for such an attack, but I don't immediately imagine OffscreenCanvas would make them more effective. The patch series doesn't add any new rendering paths, so whatever was possible before will likely still be possible and whatever wasn't will hopefully still not be possible. That said, I'll look into this and discuss it with some people that will know better than me and try to get a better picture.
Thanks,
Chris
On 2019-10-10 17:32, John Wilander wrote:
Hi Chris!
Canvas is a very popular GPU fingerprinting vector and allowing it offscreen sounds like a more convenient way to perform such an attack on user privacy. Do you know if Blink or Gecko have elaborated on this? What is your assessment?
Given the cross-engine effort to fight device fingerprinting and WebKit and Gecko’s recently published tracking prevention policies, we should do a threat analysis of this feature.
Regards, John
On Oct 10, 2019, at 4:24 AM, Chris Lord <clord@igalia.com> wrote:
Hi all,
I've spent the last month or so 'finishing' the implementation of OffscreenCanvas[1], based on Žan Doberšek's work from a year ago[2]. OffscreenCanvas is an API for being able to use canvas drawing without a visible canvas, and from within Workers. It's supported by Blink and has partial support in Gecko.
It's at the point now where I'd consider it a finished draft - it is almost fully implemented and passes the majority of relevant tests in a debug build without crashing, but has some areas that need completion on other platforms (async drawing on non-Linux) and some missing parts (Web Inspector, ImageBitmapRenderingContext). It almost certainly needs reworking in places.
My work is on GitHub[3] - I'd like to solicit reviews and comment. Some of the bugs hanging off [2] have patches that need review and I think are near ready to being landable as the foundation of this work. It is broadly split up like so:
- Refactor to move functionality from HTMLCanvasElement to CanvasBase - Refactor to not unnecessarily require HTMLCanvasElement in places - Implement OffscreenCanvas functionality - Make font loading/styling usable from a Worker and without a Document - Implement AnimationFrameProvider on DedicatedWorkerGlobalScope - Implement asynchronous drawing updates on placeholder canvases
I expect the font-related stuff to be the most contentious, and my AnimationFrameProvider implementation may be too trivial (but might be ok for a first go?)
All feedback appreciated. Best regards,
Chris
[1] https://html.spec.whatwg.org/multipage/canvas.html#the-offscreencanvas-inter... [2] https://bugs.webkit.org/show_bug.cgi?id=183720 [3] https://github.com/Cwiiis/webkit/tree/offscreen-canvas _______________________________________________ webkit-dev mailing list webkit-dev@lists.webkit.org https://lists.webkit.org/mailman/listinfo/webkit-dev
webkit-dev mailing list webkit-dev@lists.webkit.org https://lists.webkit.org/mailman/listinfo/webkit-dev
On Oct 10, 2019, at 10:18 AM, John Wilander <wilander@apple.com> wrote:
On Oct 10, 2019, at 9:42 AM, Maciej Stachowiak <mjs@apple.com> wrote:
For clarity, it’s already possible to render to a regular canvas offscreen. The <canvas> can be hidden using any of the techniques that can make any other canvas invisible. Name notwithstanding, OffscreenCanvas is mainly about being able to render from a Worker, not about enabling rendering offscreen.
Thus, I would not expect it to make it easier to invisibly fingerprint using canvas.
My thinking here is that viable mitigations for device fingerprinting could be requiring a Canvas to be visible on screen or even for it to get user interaction before it’ll get access to hardware acceleration. Such restrictions would make it harder for tracking scripts to get away with GPU fingerprinting that the site owner never wanted or accepted. If we allow offscreen Canvas in workers, site owners will stay in the dark and we can’t tie user interaction to it.
That is neat thinking. If a mitigation like this was effective, and we did it across the board, that would mean OffscreenCanvas is basically always disabled. But I’m not sure it’s viable. A few concerns: - It’s notoriously hard to tell if an element is really visible onscreen. - Canvas backing store size is independent of its layout size, so even if we could solve the visibility problem, a rather small canvas could still get all the fingerprinting bits. (And the minimum size can’t be too low.) - Rendering offscreen for multiple buffering or to to implement sprites is a legit use case and necessary for games. - Rendering a canvas that’s currently scrolled below the fold is valuable. Otherwise you get a scrolling glitch later, or show the user blank content. - Even software rendering is fingerprintable. - Drawing itself isn’t even the fingerprinting step, it’s the readback. I think canvas fingerprinting mitigations will have to be more along the lines of identifying the true source of the script attempting to perform readback and returning fake values, injecting noise (either into drawing or into readback), or things along these lines. It might also be possible to specifically block readback from invisible/offscreen canvases (as long as copying directly to another canvas is still allowed). Thus, on reflection, I still do not think OffscreenCanvas hurts our ability to do fingerprinting mitigations. But it is very good to consider tracking and fingerprinting risk for every feature, so I’m glad you brought this up! Regards, Maciej
Regards, John
On Oct 10, 2019, at 9:32 AM, Chris Lord <clord@igalia.com> wrote:
Hi John,
I don't know what the current state is of counter-measures for such an attack, but I don't immediately imagine OffscreenCanvas would make them more effective. The patch series doesn't add any new rendering paths, so whatever was possible before will likely still be possible and whatever wasn't will hopefully still not be possible. That said, I'll look into this and discuss it with some people that will know better than me and try to get a better picture.
Thanks,
Chris
On 2019-10-10 17:32, John Wilander wrote:
Hi Chris!
Canvas is a very popular GPU fingerprinting vector and allowing it offscreen sounds like a more convenient way to perform such an attack on user privacy. Do you know if Blink or Gecko have elaborated on this? What is your assessment?
Given the cross-engine effort to fight device fingerprinting and WebKit and Gecko’s recently published tracking prevention policies, we should do a threat analysis of this feature.
Regards, John
On Oct 10, 2019, at 4:24 AM, Chris Lord <clord@igalia.com> wrote:
Hi all,
I've spent the last month or so 'finishing' the implementation of OffscreenCanvas[1], based on Žan Doberšek's work from a year ago[2]. OffscreenCanvas is an API for being able to use canvas drawing without a visible canvas, and from within Workers. It's supported by Blink and has partial support in Gecko.
It's at the point now where I'd consider it a finished draft - it is almost fully implemented and passes the majority of relevant tests in a debug build without crashing, but has some areas that need completion on other platforms (async drawing on non-Linux) and some missing parts (Web Inspector, ImageBitmapRenderingContext). It almost certainly needs reworking in places.
My work is on GitHub[3] - I'd like to solicit reviews and comment. Some of the bugs hanging off [2] have patches that need review and I think are near ready to being landable as the foundation of this work. It is broadly split up like so:
- Refactor to move functionality from HTMLCanvasElement to CanvasBase - Refactor to not unnecessarily require HTMLCanvasElement in places - Implement OffscreenCanvas functionality - Make font loading/styling usable from a Worker and without a Document - Implement AnimationFrameProvider on DedicatedWorkerGlobalScope - Implement asynchronous drawing updates on placeholder canvases
I expect the font-related stuff to be the most contentious, and my AnimationFrameProvider implementation may be too trivial (but might be ok for a first go?)
All feedback appreciated. Best regards,
Chris
[1] https://html.spec.whatwg.org/multipage/canvas.html#the-offscreencanvas-inter... [2] https://bugs.webkit.org/show_bug.cgi?id=183720 [3] https://github.com/Cwiiis/webkit/tree/offscreen-canvas _______________________________________________ webkit-dev mailing list webkit-dev@lists.webkit.org https://lists.webkit.org/mailman/listinfo/webkit-dev
webkit-dev mailing list webkit-dev@lists.webkit.org https://lists.webkit.org/mailman/listinfo/webkit-dev
Hi Chris, I'm excited that you're working on OffscreenCanvas because I think it would be a valuable feature, and I'm confident we can come up with a strategy to limit its privacy & security risk as we see fit. However, many of your patches seem to ignore the fact most of WebCore objects aren't thread safe. For example, CSS parser and the entire CSS object model aren't designed to used in non-main thread. Regardless of how ready Linux ports might be, we can't land or enable this feature in WebKit until all thread safety issues are sorted out. Unfortunately, I can't make a time commitment to review & find every thread safety issue in your patches. Please work with relevant experts and go over your code changes. For example, it's never safe to an object that's RefCounted in multiple threads because RefCounted isn't thread safe. One would have to use ThreadSafeRefCounted. It's never safe to use AtomString from one another in another because AtomString has a pool of strings per thread. For that matter, it's never safe to use a single String object from two or more threads because String itself is RefCounted and isn't thread safe. It's not not okay to do readonly access to basic container types like Vector, HashMap, etc... because they don't guarantee atomic update of internal data structures and doing so can result in UAF. I think the hardest part of this project is validating that enabling this feature wouldn't introduce dozens of new thread safety issues and thereby security vulnerabilities. - R. Niwa On Thu, Oct 10, 2019 at 4:23 AM Chris Lord <clord@igalia.com> wrote:
I've spent the last month or so 'finishing' the implementation of OffscreenCanvas[1], based on Žan Doberšek's work from a year ago[2]. OffscreenCanvas is an API for being able to use canvas drawing without a visible canvas, and from within Workers. It's supported by Blink and has partial support in Gecko.
It's at the point now where I'd consider it a finished draft - it is almost fully implemented and passes the majority of relevant tests in a debug build without crashing, but has some areas that need completion on other platforms (async drawing on non-Linux) and some missing parts (Web Inspector, ImageBitmapRenderingContext). It almost certainly needs reworking in places.
My work is on GitHub[3] - I'd like to solicit reviews and comment. Some of the bugs hanging off [2] have patches that need review and I think are near ready to being landable as the foundation of this work. It is broadly split up like so:
- Refactor to move functionality from HTMLCanvasElement to CanvasBase - Refactor to not unnecessarily require HTMLCanvasElement in places - Implement OffscreenCanvas functionality - Make font loading/styling usable from a Worker and without a Document - Implement AnimationFrameProvider on DedicatedWorkerGlobalScope - Implement asynchronous drawing updates on placeholder canvases
I expect the font-related stuff to be the most contentious, and my AnimationFrameProvider implementation may be too trivial (but might be ok for a first go?)
All feedback appreciated. Best regards,
Chris
[1]
https://html.spec.whatwg.org/multipage/canvas.html#the-offscreencanvas-inter... [2] https://bugs.webkit.org/show_bug.cgi?id=183720 [3] https://github.com/Cwiiis/webkit/tree/offscreen-canvas _______________________________________________ webkit-dev mailing list webkit-dev@lists.webkit.org https://lists.webkit.org/mailman/listinfo/webkit-dev
On Oct 10, 2019, at 12:57 PM, Ryosuke Niwa <rniwa@webkit.org> wrote:
Hi Chris,
I'm excited that you're working on OffscreenCanvas because I think it would be a valuable feature
Me too!!!
, and I'm confident we can come up with a strategy to limit its privacy & security risk as we see fit.
However, many of your patches seem to ignore the fact most of WebCore objects aren't thread safe. For example, CSS parser and the entire CSS object model aren't designed to used in non-main thread. Regardless of how ready Linux ports might be, we can't land or enable this feature in WebKit until all thread safety issues are sorted out.
Unfortunately, I can't make a time commitment to review & find every thread safety issue in your patches. Please work with relevant experts and go over your code changes.
I’d be happy to work with you on this.
For example, it's never safe to an object that's RefCounted in multiple threads because RefCounted isn't thread safe. One would have to use ThreadSafeRefCounted. It's never safe to use AtomString from one another in another because AtomString has a pool of strings per thread. For that matter, it's never safe to use a single String object from two or more threads because String itself is RefCounted and isn't thread safe. It's not not okay to do readonly access to basic container types like Vector, HashMap, etc... because they don't guarantee atomic update of internal data structures and doing so can result in UAF.
I think the hardest part of this project is validating that enabling this feature wouldn't introduce dozens of new thread safety issues and thereby security vulnerabilities.
Sounds like this this is a good candidate for a feature flag.
- R. Niwa
On Thu, Oct 10, 2019 at 4:23 AM Chris Lord <clord@igalia.com <mailto:clord@igalia.com>> wrote:
I've spent the last month or so 'finishing' the implementation of OffscreenCanvas[1], based on Žan Doberšek's work from a year ago[2]. OffscreenCanvas is an API for being able to use canvas drawing without a visible canvas, and from within Workers. It's supported by Blink and has partial support in Gecko.
It's at the point now where I'd consider it a finished draft - it is almost fully implemented and passes the majority of relevant tests in a debug build without crashing, but has some areas that need completion on other platforms (async drawing on non-Linux) and some missing parts (Web Inspector, ImageBitmapRenderingContext). It almost certainly needs reworking in places.
My work is on GitHub[3] - I'd like to solicit reviews and comment. Some of the bugs hanging off [2] have patches that need review and I think are near ready to being landable as the foundation of this work. It is broadly split up like so:
- Refactor to move functionality from HTMLCanvasElement to CanvasBase - Refactor to not unnecessarily require HTMLCanvasElement in places - Implement OffscreenCanvas functionality - Make font loading/styling usable from a Worker and without a Document - Implement AnimationFrameProvider on DedicatedWorkerGlobalScope - Implement asynchronous drawing updates on placeholder canvases
I expect the font-related stuff to be the most contentious, and my AnimationFrameProvider implementation may be too trivial (but might be ok for a first go?)
All feedback appreciated. Best regards,
Chris
[1] https://html.spec.whatwg.org/multipage/canvas.html#the-offscreencanvas-inter... <https://html.spec.whatwg.org/multipage/canvas.html#the-offscreencanvas-interface> [2] https://bugs.webkit.org/show_bug.cgi?id=183720 <https://bugs.webkit.org/show_bug.cgi?id=183720> [3] https://github.com/Cwiiis/webkit/tree/offscreen-canvas <https://github.com/Cwiiis/webkit/tree/offscreen-canvas> _______________________________________________ webkit-dev mailing list webkit-dev@lists.webkit.org <mailto:webkit-dev@lists.webkit.org> https://lists.webkit.org/mailman/listinfo/webkit-dev <https://lists.webkit.org/mailman/listinfo/webkit-dev> _______________________________________________ webkit-dev mailing list webkit-dev@lists.webkit.org <mailto:webkit-dev@lists.webkit.org> https://lists.webkit.org/mailman/listinfo/webkit-dev <https://lists.webkit.org/mailman/listinfo/webkit-dev>
On Thu, Oct 10, 2019 at 2:07 PM Myles C. Maxfield <mmaxfield@apple.com> wrote:
On Oct 10, 2019, at 12:57 PM, Ryosuke Niwa <rniwa@webkit.org> wrote:
Hi Chris,
I'm excited that you're working on OffscreenCanvas because I think it would be a valuable feature
Me too!!!
, and I'm confident we can come up with a strategy to limit its privacy & security risk as we see fit.
However, many of your patches seem to ignore the fact most of WebCore objects aren't thread safe. For example, CSS parser and the entire CSS object model aren't designed to used in non-main thread. Regardless of how ready Linux ports might be, we can't land or enable this feature in WebKit until all thread safety issues are sorted out.
Unfortunately, I can't make a time commitment to review & find every thread safety issue in your patches. Please work with relevant experts and go over your code changes.
I’d be happy to work with you on this.
For example, it's never safe to an object that's RefCounted in multiple threads because RefCounted isn't thread safe. One would have to use ThreadSafeRefCounted. It's never safe to use AtomString from one another in another because AtomString has a pool of strings per thread. For that matter, it's never safe to use a single String object from two or more threads because String itself is RefCounted and isn't thread safe. It's not not okay to do readonly access to basic container types like Vector, HashMap, etc... because they don't guarantee atomic update of internal data structures and doing so can result in UAF.
I think the hardest part of this project is validating that enabling this feature wouldn't introduce dozens of new thread safety issues and thereby security vulnerabilities.
Sounds like this this is a good candidate for a feature flag.
Yeah, in fact, this should really have a compile time flag in addition to runtime flag, and it should be default off until we've done enough fuzzing. The thread unsafe code can be turned into an attack gadget if it exists at all in production binary. - R. Niwa On Thu, Oct 10, 2019 at 4:23 AM Chris Lord <clord@igalia.com> wrote:
I've spent the last month or so 'finishing' the implementation of OffscreenCanvas[1], based on Žan Doberšek's work from a year ago[2]. OffscreenCanvas is an API for being able to use canvas drawing without a visible canvas, and from within Workers. It's supported by Blink and has partial support in Gecko.
It's at the point now where I'd consider it a finished draft - it is almost fully implemented and passes the majority of relevant tests in a debug build without crashing, but has some areas that need completion on other platforms (async drawing on non-Linux) and some missing parts (Web Inspector, ImageBitmapRenderingContext). It almost certainly needs reworking in places.
My work is on GitHub[3] - I'd like to solicit reviews and comment. Some of the bugs hanging off [2] have patches that need review and I think are near ready to being landable as the foundation of this work. It is broadly split up like so:
- Refactor to move functionality from HTMLCanvasElement to CanvasBase - Refactor to not unnecessarily require HTMLCanvasElement in places - Implement OffscreenCanvas functionality - Make font loading/styling usable from a Worker and without a Document - Implement AnimationFrameProvider on DedicatedWorkerGlobalScope - Implement asynchronous drawing updates on placeholder canvases
I expect the font-related stuff to be the most contentious, and my AnimationFrameProvider implementation may be too trivial (but might be ok for a first go?)
All feedback appreciated. Best regards,
Chris
[1]
https://html.spec.whatwg.org/multipage/canvas.html#the-offscreencanvas-inter... [2] https://bugs.webkit.org/show_bug.cgi?id=183720 [3] https://github.com/Cwiiis/webkit/tree/offscreen-canvas _______________________________________________ webkit-dev mailing list webkit-dev@lists.webkit.org https://lists.webkit.org/mailman/listinfo/webkit-dev
_______________________________________________ webkit-dev mailing list webkit-dev@lists.webkit.org https://lists.webkit.org/mailman/listinfo/webkit-dev
On 2019-10-10 23:15, Ryosuke Niwa wrote:
On Thu, Oct 10, 2019 at 2:07 PM Myles C. Maxfield <mmaxfield@apple.com> wrote:
On Oct 10, 2019, at 12:57 PM, Ryosuke Niwa <rniwa@webkit.org> wrote:
Hi Chris,
I'm excited that you're working on OffscreenCanvas because I think it would be a valuable feature
Me too!!!
, and I'm confident we can come up with a strategy to limit its privacy & security risk as we see fit.
However, many of your patches seem to ignore the fact most of WebCore objects aren't thread safe. For example, CSS parser and the entire CSS object model aren't designed to used in non-main thread. Regardless of how ready Linux ports might be, we can't land or enable this feature in WebKit until all thread safety issues are sorted out.
Unfortunately, I can't make a time commitment to review & find every thread safety issue in your patches. Please work with relevant experts and go over your code changes.
I’d be happy to work with you on this.
For example, it's never safe to an object that's RefCounted in multiple threads because RefCounted isn't thread safe. One would have to use ThreadSafeRefCounted. It's never safe to use AtomString from one another in another because AtomString has a pool of strings per thread. For that matter, it's never safe to use a single String object from two or more threads because String itself is RefCounted and isn't thread safe. It's not not okay to do readonly access to basic container types like Vector, HashMap, etc... because they don't guarantee atomic update of internal data structures and doing so can result in UAF.
To the best of my knowledge, I've taken this into account (it's hard to see this without applying the whole patch series, as later patches assume the changes made with respect to thread-safety in the earlier patches). There are of course bound to be things I've missed - and I'm also open to taking a different strategy on certain things too, I'm fairly new to the WebKit codebase (well, I'm assuming having worked on it around 10 years ago doesn't apply too much anymore :)) and I imagine I've taken some naive approaches in places that someone more experienced would be able to correct me on.
I think the hardest part of this project is validating that enabling this feature wouldn't introduce dozens of new thread safety issues and thereby security vulnerabilities.
Sounds like this this is a good candidate for a feature flag.
Yeah, in fact, this should really have a compile time flag in addition to runtime flag, and it should be default off until we've done enough fuzzing. The thread unsafe code can be turned into an attack gadget if it exists at all in production binary.
This sounds good to me, I'll file a bug and write a patch for this. I assume there are ways of enabling features when tests are run? While a user (or a developer) using WebKit wouldn't want this feature to be enabled, I think it should be enabled for (some) test runs as soon as possible to give us an indication of any issues that exist at the moment.
On Thu, Oct 10, 2019 at 4:23 AM Chris Lord <clord@igalia.com> wrote:
I've spent the last month or so 'finishing' the implementation of OffscreenCanvas[1], based on Žan Doberšek's work from a year ago[2]. OffscreenCanvas is an API for being able to use canvas drawing without a visible canvas, and from within Workers. It's supported by Blink and has partial support in Gecko.
It's at the point now where I'd consider it a finished draft - it is almost fully implemented and passes the majority of relevant tests in a debug build without crashing, but has some areas that need completion on other platforms (async drawing on non-Linux) and some missing parts (Web Inspector, ImageBitmapRenderingContext). It almost certainly needs reworking in places.
My work is on GitHub[3] - I'd like to solicit reviews and comment. Some of the bugs hanging off [2] have patches that need review and I think are near ready to being landable as the foundation of this work. It is broadly split up like so:
- Refactor to move functionality from HTMLCanvasElement to CanvasBase - Refactor to not unnecessarily require HTMLCanvasElement in places - Implement OffscreenCanvas functionality - Make font loading/styling usable from a Worker and without a Document - Implement AnimationFrameProvider on DedicatedWorkerGlobalScope - Implement asynchronous drawing updates on placeholder canvases
I expect the font-related stuff to be the most contentious, and my AnimationFrameProvider implementation may be too trivial (but might be ok for a first go?)
All feedback appreciated. Best regards,
Chris
[1]
https://html.spec.whatwg.org/multipage/canvas.html#the-offscreencanvas-inter...
[2] https://bugs.webkit.org/show_bug.cgi?id=183720 [3] https://github.com/Cwiiis/webkit/tree/offscreen-canvas _______________________________________________ webkit-dev mailing list webkit-dev@lists.webkit.org https://lists.webkit.org/mailman/listinfo/webkit-dev _______________________________________________ webkit-dev mailing list webkit-dev@lists.webkit.org https://lists.webkit.org/mailman/listinfo/webkit-dev
On Fri, Oct 11, 2019 at 2:09 AM Chris Lord <clord@igalia.com> wrote:
On 2019-10-10 23:15, Ryosuke Niwa wrote:
On Thu, Oct 10, 2019 at 2:07 PM Myles C. Maxfield <mmaxfield@apple.com> wrote:
On Oct 10, 2019, at 12:57 PM, Ryosuke Niwa <rniwa@webkit.org> wrote:
Hi Chris,
I'm excited that you're working on OffscreenCanvas because I think it would be a valuable feature
Me too!!!
, and I'm confident we can come up with a strategy to limit its privacy & security risk as we see fit.
However, many of your patches seem to ignore the fact most of WebCore objects aren't thread safe. For example, CSS parser and the entire CSS object model aren't designed to used in non-main thread. Regardless of how ready Linux ports might be, we can't land or enable this feature in WebKit until all thread safety issues are sorted out.
Unfortunately, I can't make a time commitment to review & find every thread safety issue in your patches. Please work with relevant experts and go over your code changes.
I’d be happy to work with you on this.
For example, it's never safe to an object that's RefCounted in multiple threads because RefCounted isn't thread safe. One would have to use ThreadSafeRefCounted. It's never safe to use AtomString from one another in another because AtomString has a pool of strings per thread. For that matter, it's never safe to use a single String object from two or more threads because String itself is RefCounted and isn't thread safe. It's not not okay to do readonly access to basic container types like Vector, HashMap, etc... because they don't guarantee atomic update of internal data structures and doing so can result in UAF.
To the best of my knowledge, I've taken this into account (it's hard to see this without applying the whole patch series, as later patches assume the changes made with respect to thread-safety in the earlier patches). There are of course bound to be things I've missed - and I'm also open to taking a different strategy on certain things too, I'm fairly new to the WebKit codebase (well, I'm assuming having worked on it around 10 years ago doesn't apply too much anymore :)) and I imagine I've taken some naive approaches in places that someone more experienced would be able to correct me on.
I think you want Antti's input most here since many of the classes you're touching in CSS is maintained by Antti these days. The thing that worries me most about this feature is the thread safety. Historically, we had many thread safety issues regarding Timer, RefCounted objects, WeakPtr, etc... Chris (Dumez) and I have been addressing some of those issues more systematically (e.g. https://trac.webkit.org/r241499 & https://trac.webkit.org/r248113) because we seem to keep making same mistakes across the codebase. The key here is to really make which objects are used concurrently or in non-main threads obvious to whomever reading the code in the future. I can think of a few ways to do that. For one, we should be adding lots of thread safety assertions. Assertions are better than comments because they're obvious to readers and they warn us whenever they get out of date or we make mistakes. In fact, I discourage adding comments about thread safety; if you find yourself doing that, then it's time to refactor code such that the code is obviously multi-threaded or concurrent by the virtue of the way things are structured or make it not matter because the code is naturally thread safe. Just as an example, one of your patches made CSSValuePool's member functions thread safe but it left the rest of code still access CSSValuePool::singleton() object. This is problematic because singleton objects are usually shared across threads. A better approach is to have each caller explicitly get a specific instance of CSSValuePool from some other object; e.g. CSSParser's member variable. This will make it so that the code that needs to run concurrently would not rely on singleton objects, and whatever new code which gets introduced to CSSValuePool or CSSParser would naturally be thread safe so long as the author follows the same convention. Finally, we can add an assertion in CSSValuePool::singleton() to make sure it's only called in the thread. This way, anyone who makes a mistake of calling this singleton function in CSSParser, or elsewhere which can be used by the offscreen canvas code in worker would hit this assertion.
I think the hardest part of this project is validating that
enabling this feature wouldn't introduce dozens of new thread safety issues and thereby security vulnerabilities.
Sounds like this this is a good candidate for a feature flag.
Yeah, in fact, this should really have a compile time flag in addition to runtime flag, and it should be default off until we've done enough fuzzing. The thread unsafe code can be turned into an attack gadget if it exists at all in production binary.
This sounds good to me, I'll file a bug and write a patch for this. I assume there are ways of enabling features when tests are run? While a user (or a developer) using WebKit wouldn't want this feature to be enabled, I think it should be enabled for (some) test runs as soon as possible to give us an indication of any issues that exist at the moment.
If you added a compile time flag, then you'd have to manually enable that for testing. Historically, we've done things like adding a special buildbot builder which runs tests with a certain feature enabled. You could also opt to turn on the compile time flag on by default on GTK+ port assuming the rest of GTK+ port maintainers are okay with that. For this feature, you may also want to setup a bot or some machine which runs ThreadSanitizer: https://clang.llvm.org/docs/ThreadSanitizer.html I'd definitely recommend doing extensive fuzzing to make sure we've sorted out all the thread safety issues before turning it on by default though especially because offscreen canvas isn't a kind of feature normal layout tests or WPT tests would exercise so just running them would probably be not enough to catch all nasty thread safety issues. - R. Niwa
On 2019-10-11 12:13, Ryosuke Niwa wrote:
On Fri, Oct 11, 2019 at 2:09 AM Chris Lord <clord@igalia.com> wrote:
On 2019-10-10 23:15, Ryosuke Niwa wrote:
On Thu, Oct 10, 2019 at 2:07 PM Myles C. Maxfield <mmaxfield@apple.com> wrote:
On Oct 10, 2019, at 12:57 PM, Ryosuke Niwa <rniwa@webkit.org> wrote:
Hi Chris,
I'm excited that you're working on OffscreenCanvas because I think it would be a valuable feature
Me too!!!
, and I'm confident we can come up with a strategy to limit its privacy & security risk as we see fit.
However, many of your patches seem to ignore the fact most of WebCore objects aren't thread safe. For example, CSS parser and the entire CSS object model aren't designed to used in non-main thread. Regardless of how ready Linux ports might be, we can't land or enable this feature in WebKit until all thread safety issues are sorted out.
Unfortunately, I can't make a time commitment to review & find every thread safety issue in your patches. Please work with relevant experts and go over your code changes.
I’d be happy to work with you on this.
For example, it's never safe to an object that's RefCounted in multiple threads because RefCounted isn't thread safe. One would have to use ThreadSafeRefCounted. It's never safe to use AtomString from one another in another because AtomString has a pool of strings per thread. For that matter, it's never safe to use a single String object from two or more threads because String itself is RefCounted and isn't thread safe. It's not not okay to do readonly access to basic container types like Vector, HashMap, etc... because they don't guarantee atomic update of internal data structures and doing so can result in UAF.
To the best of my knowledge, I've taken this into account (it's hard to see this without applying the whole patch series, as later patches assume the changes made with respect to thread-safety in the earlier patches). There are of course bound to be things I've missed - and I'm also open to taking a different strategy on certain things too, I'm fairly new to the WebKit codebase (well, I'm assuming having worked on it around 10 years ago doesn't apply too much anymore :)) and I imagine I've taken some naive approaches in places that someone more experienced would be able to correct me on.
I think you want Antti's input most here since many of the classes you're touching in CSS is maintained by Antti these days.
The thing that worries me most about this feature is the thread safety. Historically, we had many thread safety issues regarding Timer, RefCounted objects, WeakPtr, etc... Chris (Dumez) and I have been addressing some of those issues more systematically (e.g. https://trac.webkit.org/r241499 & https://trac.webkit.org/r248113) because we seem to keep making same mistakes across the codebase. The key here is to really make which objects are used concurrently or in non-main threads obvious to whomever reading the code in the future.
I can think of a few ways to do that. For one, we should be adding lots of thread safety assertions. Assertions are better than comments because they're obvious to readers and they warn us whenever they get out of date or we make mistakes. In fact, I discourage adding comments about thread safety; if you find yourself doing that, then it's time to refactor code such that the code is obviously multi-threaded or concurrent by the virtue of the way things are structured or make it not matter because the code is naturally thread safe. Just as an example, one of your patches made CSSValuePool's member functions thread safe but it left the rest of code still access CSSValuePool::singleton() object. This is problematic because singleton objects are usually shared across threads. A better approach is to have each caller explicitly get a specific instance of CSSValuePool from some other object; e.g. CSSParser's member variable. This will make it so that the code that needs to run concurrently would not rely on singleton objects, and whatever new code which gets introduced to CSSValuePool or CSSParser would naturally be thread safe so long as the author follows the same convention. Finally, we can add an assertion in CSSValuePool::singleton() to make sure it's only called in the thread. This way, anyone who makes a mistake of calling this singleton function in CSSParser, or elsewhere which can be used by the offscreen canvas code in worker would hit this assertion.
Just an update on this, CSS parsing largely happens via static functions, so there's no one place to add a CSSValuePool member variable - however, I did add it where applicable and added alternative functions to all of the CSS parsing static functions that end up in using a CSSValuePool with versions of the function that accept a given CSSValuePool. I wonder if we want to remove the versions that don't to prevent accidental usage of CSSValuePool::singleton()?
I think the hardest part of this project is validating that enabling this feature wouldn't introduce dozens of new thread safety issues and thereby security vulnerabilities.
Sounds like this this is a good candidate for a feature flag.
Yeah, in fact, this should really have a compile time flag in addition to runtime flag, and it should be default off until we've done enough fuzzing. The thread unsafe code can be turned into an attack gadget if it exists at all in production binary.
This sounds good to me, I'll file a bug and write a patch for this. I assume there are ways of enabling features when tests are run? While a user (or a developer) using WebKit wouldn't want this feature to be enabled, I think it should be enabled for (some) test runs as soon as possible to give us an indication of any issues that exist at the moment.
If you added a compile time flag, then you'd have to manually enable that for testing. Historically, we've done things like adding a special buildbot builder which runs tests with a certain feature enabled. You could also opt to turn on the compile time flag on by default on GTK+ port assuming the rest of GTK+ port maintainers are okay with that.
So I was just looking at adding a setting, but looks like it's already behind a runtime-setting that's controlled by the experimental-features flag, so I guess we're ok as it is?
For this feature, you may also want to setup a bot or some machine which runs ThreadSanitizer: https://clang.llvm.org/docs/ThreadSanitizer.html
I'd definitely recommend doing extensive fuzzing to make sure we've sorted out all the thread safety issues before turning it on by default though especially because offscreen canvas isn't a kind of feature normal layout tests or WPT tests would exercise so just running them would probably be not enough to catch all nasty thread safety issues.
We're looking into setting up an extra test machine with ThreadSanitizer, we'll have a look at fuzzing too. I'll post when we've made some headway. Cheers, Chris
On 2019-10-17 16:23, Chris Lord wrote:
So I was just looking at adding a setting, but looks like it's already behind a runtime-setting that's controlled by the experimental-features flag, so I guess we're ok as it is?
After discussing with Zan, we decided the best thing to do was to split this setting (which currently controls both ImageBitmap and OffscreenCanvas) and add a build option for the OffscreenCanvas part - this way we can retain the current behaviour that affects ImageBitmap and still allow disabling OffscreenCanvas by default and in the build. Sequence-wise, this will go in before any OffscreenCanvas-specific changes go in (i.e. before bug 182686). --Chris
On 11/10/2019 11:09, Chris Lord wrote:
This sounds good to me, I'll file a bug and write a patch for this. I assume there are ways of enabling features when tests are run? While a user (or a developer) using WebKit wouldn't want this feature to be enabled, I think it should be enabled for (some) test runs as soon as possible to give us an indication of any issues that exist at the moment.
This can be achieved with a run-time flag that is off by default but enabled for the layout tests. See TestController::resetPreferencesToConsistentValues() in Tools/WebKitTestRunner/TestController.cpp
participants (6)
-
Carlos Alberto Lopez Perez
-
Chris Lord
-
John Wilander
-
Maciej Stachowiak
-
Myles C. Maxfield
-
Ryosuke Niwa