Bug 225179

Created attachment 427362 [details] Patch

What are the reasons to believe that thread safety is at least roughly implemented?

Created attachment 427372 [details] Patch

(In reply to Alexey Proskuryakov from comment #6) > What are the reasons to believe that thread safety is at least roughly > implemented? Yes. Chris Lord and others at Igalia have been actively working on improving it for quite a while now.

Right, I'm aware of the work. What is not clear is what reasons we have to believe that it's nearing completion. As we have very few threading assertions in WebCore code, and don't use TSan, this is not an idle question.

(In reply to Alexey Proskuryakov from comment #9) > Right, I'm aware of the work. What is not clear is what reasons we have to > believe that it's nearing completion. > > As we have very few threading assertions in WebCore code, and don't use > TSan, this is not an idle question. I see. I don’t know if it is nearing completion.

For the record, I don't think the thread safety aspect is 100% (or 99%, or whatever value you want to use to mean "done enough") yet. For text rendering, I know that it definitely isn't there on non-Linux. The general way we've gone about implementing this is to try to minimise the contact of the OffscreenCanvas code-paths with everything else, then within that small (hopefully) scope, isolate data as much as possible. We don't share any data across threads, so "thread-safe" is a bit of a misleading term (I've been using the term "worker-safe", which is also inadequate, but at least a bit closer). Most of the work has been focused on making things safe to use off the main thread, but not deviating from their creation thread. For example, this has lead to a sub-set of CSS parsing (for colours and font properties and shorthands) that is wholly contained with regards to its data-use, and so ought to be worker-safe, assuming you only supply it data that is safe for it to access. We rely on the canvas implementation being safe to run off the main thread, but only on its creation thread, which is true in the case of Linux/cairo and there's a buffer-transferring abstraction and locking around handing off buffers to the main thread and compositor. The biggest question mark, in my opinion, is font loading and rendering, where the reach is so wide and so platform-dependent that I find it hard to feel confident there. We rely on font functions being safe to run on their creation thread and we don't share cached fonts across threads (or indeed *any* data beyond possibly the file cache, as loading is done on the main thread via ThreadableLoader). This is the same approach as other parts of OffscreenCanvas, but I would suspect that if there are problems, this is where they would lie. I would love to hear ways that we could more rigorously guarantee not introducing thread-safety issues (TSan looks interesting). I don't think this is a problem unique to OffscreenCanvas. I don't think OffscreenCanvas is a feature that's ready to enable by default, but I do think it would be useful for it to be tested on multiple platforms so that we can at least see where the problems are and to guard against regressing. I'd like to encourage discussion with the end goal of being able to enable this feature, even if it isn't ready right now.

It definitely should be disabled on the bots and in Safari Technology Preview - otherwise, random memory corruption from threading issues will be resulting in hard to diagnose unreproducible problems all over the place. As for removing a compile flag, Sam's patch deletes a lot of #ifs, which is nice, but I don't have an opinion beyond that.

Created attachment 427383 [details] Patch

(In reply to Alexey Proskuryakov from comment #12) > It definitely should be disabled on the bots and in Safari Technology > Preview - otherwise, random memory corruption from threading issues will be > resulting in hard to diagnose unreproducible problems all over the place. > > As for removing a compile flag, Sam's patch deletes a lot of #ifs, which is > nice, but I don't have an opinion beyond that. The WIP only deletes the #ifdefs temporarily to work around the build systems which still don't work correctly when changing #ifdefs that effect generated content. I plan to revert that part and just change the define to 1 when things get closer to working.

I think disabling it on the bots would be bad and set a bad precedent. My goal here is to have it enabled on the bots.

Perhaps I'm not following you. Enabling something that is expected to cause memory corruption seems so obviously undesirable that you must not mean it?

(In reply to Alexey Proskuryakov from comment #16) > Perhaps I'm not following you. Enabling something that is expected to cause > memory corruption seems so obviously undesirable that you must not mean it? I guess I don't really know what you mean by "expected to cause memory corruption"? I don't currently expect it to cause memory corruption, but I don't know how to prove that negative. If there are bugs, we should fix them, but the first step toward fixing bugs, in my experience, is to make sure the code is being run and tested regularly. And the way we do that in WebKit is to have it compiled in but disabled at runtime. In general, and I know generalities are not the greatest for discussions like this, especially in a bug tracker, I don't think we should have code in the repository that is not being compiled in to the apple ports (with the obvious exception of platform specific code). It makes it quite a bit harder to refactor and test changes, as one is often just guessing and using the bots to try make sure the ports that do compile it in keep working. I think I understand your concern in this case, which is that this specific code is in such bad shape and would cause hard to debug issues due to memory corruption, and that the responsibility of debugging those issues would not be on the person who wrote this code (or enabled it), but on someone else (at least initially) because it was not obvious that the issue was due to this code. Is that a satisfactory explanation of your concern? Assuming that is your concern, I definitely see where you are coming from, but I also don't know how to resolve it with my desire to have the code enabled. Since we lack the quantitate tools to measure the problem space, we need some other means to decide whether this code, or some subset of it, can be enabled without undue burden to those diagnosing new issues as they arise. Before I go and make my own proposals, do you have a specific proposal in mind for what would be the bar in your mind for enabling this functionality? This is definitely a tricky situation and not one the governance structure of the project has much to say on, but I think we can work out some compromise or plan here, so I appreciate any additional input.

I agree with pretty much all that you said above. This doesn't seem like a governance issue at this point. From what Chris said in comment 11, it seems like he agrees that the code is in a "bad shape" at this point. There are several enablement steps, which turn into several options if we are combining them: - do not enable the code to compile by default - we are here; - enable compiling, but disable the feature at runtime everywhere (tests, Safari Tech Preview); - enable the feature at runtime (with a sub-choice of whether to do it in local/CI builds only, or also in STP) - but skip all tests that use it; - enable the feature, and run the tests. I'm opposing the last option, basically for the reasons you explained. Enabling by default and just not running any tests is OK, although fragile (what if the next WPT reimport adds more tests?), so it would be appropriate to develop some kind of mitigation if that's what we do, and I don't have a proposal for such a mitigation. People who know more about details of this project may have opinions on the right timing of the other steps, but I'll keep my feedback focused on testing impact.

(In reply to Alexey Proskuryakov from comment #18) > I agree with pretty much all that you said above. This doesn't seem like a > governance issue at this point. > > From what Chris said in comment 11, it seems like he agrees that the code is > in a "bad shape" at this point. Well no, I wouldn't say that :) I just think that there are almost certainly issues to discover. Parts of it I'm reasonably confident about, other parts not so much - but like Sam says, it'll be much harder to make progress on that without getting wider testing. > There are several enablement steps, which turn into several options if we > are combining them: > > - do not enable the code to compile by default - we are here; > - enable compiling, but disable the feature at runtime everywhere (tests, > Safari Tech Preview); > - enable the feature at runtime (with a sub-choice of whether to do it in > local/CI builds only, or also in STP) - but skip all tests that use it; > - enable the feature, and run the tests. > > I'm opposing the last option, basically for the reasons you explained. > Enabling by default and just not running any tests is OK, although fragile > (what if the next WPT reimport adds more tests?), so it would be appropriate > to develop some kind of mitigation if that's what we do, and I don't have a > proposal for such a mitigation. > > People who know more about details of this project may have opinions on the > right timing of the other steps, but I'll keep my feedback focused on > testing impact. While this would still be preferable to not building the code, I still think we want to be running these tests somewhere soon. It strikes me that this can't be the only feature where this has been an issue in the past. There are several parts of WebKit that would have perhaps even greater potential problems with thread safety (asynchronous video decoding/playback, compositing, async scrolling, anything else to do with workers, GPUProcess, network process, etc. etc.) - what was done to mitigate thread safety issues in those cases when enabling tests?

As the person who originally implemented Web Workers in WebKit, I don't think that we ever exposed such a large body of code that was written with main thread assumption to workers before. The examples you gave all had designs where we knew what exactly needed to be thread safe, and it was largely new code.

(In reply to Alexey Proskuryakov from comment #20) > As the person who originally implemented Web Workers in WebKit, I don't > think that we ever exposed such a large body of code that was written with > main thread assumption to workers before. The examples you gave all had > designs where we knew what exactly needed to be thread safe, and it was > largely new code. Point taken, but I think the concern still stands, these are cases where a lot of code that interacts with the main thread that isn't on the main thread was introduced; there must have been concerns then? Was there anything specific done to address those concerns? I'm looking for hints of what it is that would assuage people's concerns beyond "write an entirely new canvas and font handling implementation". For what it's worth, OffscreenCanvas really doesn't do much cross-thread - cross-thread communication is only used to transfer buffers (which are done via specific, newly-written interfaces/code) and for font loading (via ThreadableLoader). I'll read up on TSan in the meantime.

I think that the examples that you mentioned are not comparable to offscreen canvas - most didn't expose any existing code to secondary threads. Web Workers themselves may be the closest comparison, and the path taken there was to review ALL static variables in JavaScriptCore, removing most of them. JSC was much simpler back then than it is today, and also it used almost no system APIs. Then worker APIs like loading were implemented by posting messages to the main thread.

Hi Alexey, one aspect I am still unclear on is this question: "Before I go and make my own proposals, do you have a specific proposal in mind for what would be the bar in your mind for enabling this functionality?" By "this functionality", I mean compiling the code and running the tests.

I am not sufficiently familiar with this code to set the bar. Seems clear that it should be higher than YOLO. This is exposing lot of code, so it seems like the bar needs to be unprecedentedly high too.

<rdar://problem/77585471>

(In reply to Alexey Proskuryakov from comment #24) > I am not sufficiently familiar with this code to set the bar. Seems clear > that it should be higher than YOLO. > > This is exposing lot of code, so it seems like the bar needs to be > unprecedentedly high too. Ok, I don't think it is practical for you to object to this landing and being enable, but not giving specific feedback on what needs to change. I don't think "audit code in webcore" is a practical bar. This is not just being done YOLO. This code has been enabled in other ports for a while, and there are number of tests written for it. I filed some bugs on issues I have found and disabled tests that expertise known unsafe paths (as you can see from the patch). My bar at this point is going to be that we run the tests a bunch and see if there are failures or flakes and either disable/or fix those things.

> I don't think "audit code in webcore" is a practical bar. The conclusion that logically follows from this is that we should delay enabling this feature until someone suggests a more practical validation plan that experts can agree with. It does not become my responsibility to suggest it. Your plan (that we just "run the tests a bunch") seems clearly insufficient to me. I think that there may be a deadlock here until experts in the exposed code get involved. I still don't think of this as a governance issue, in my view it's that technical aspects are not understood yet.

(In reply to Alexey Proskuryakov from comment #27) > > I don't think "audit code in webcore" is a practical bar. > > The conclusion that logically follows from this is that we should delay > enabling this feature until someone suggests a more practical validation > plan that experts can agree with. It does not become my responsibility to > suggest it. > > Your plan (that we just "run the tests a bunch") seems clearly insufficient > to me. > > I think that there may be a deadlock here until experts in the exposed code > get involved. I still don't think of this as a governance issue, in my view > it's that technical aspects are not understood yet. Ok. Can you suggest which experts you would like to weigh in? I also thought of another potential idea as a potential stop gap that I think might not be problematic. Since OffscreenCanvas can operate on both the main thread and in workers, how would you feel about enabling compiling and testing of OffscreenCanvas running on the main thread. That is, not run the tests that exercise the worker offscreen canvas code initially. I still think we need to find a path to enabling it in workers, but I also don't want to stall this too much.

To a degree, this is a replay of the discussion that was had here: <https://lists.webkit.org/pipermail/webkit-dev/2019-October/030845.html>. So I would suggest soliciting opinions from people who participated in it. I can't help but notice Ryosuke's advice to work with relevant experts - have they been identified since 2019? Maybe even revive that thread to summarize what happened since then. WebKit has experts in threading correctness, rendering, CSS, fonts etc., I just don't want to name names because of the risk of accidentally omitting someone. I will take the liberty of CC'ing Antti and Myles though, in addition to the existing long CC list. > Since OffscreenCanvas can operate on both the main thread and in workers, how would you feel about enabling compiling and testing of OffscreenCanvas running on the main thread. That is, not run the tests that exercise the worker offscreen canvas code initially. I have no objection to enabling OffscreenCanvas on main thread only, from the point of view of test suite integrity. Someone else may have opinions on whether it's good enough to be enabled in Safari Tech Preview (notably, have privacy aspects been worked out?)

Alright, I guess I'll comment here ... I've been mentioned by name .... I have a bunch of thoughts (because of course I do). 1. It's not really okay to block features on specific individuals. If a reviewer is confident enough to meaningfully review a patch, they can go ahead and do so. And, if no one volunteers, the poster can feel free to simplify / break up their patch to make it more easily reviewable. 2. I would be happy to spend significant time doing a deep audit of OffscreenCanvas's font code, in close contact with Chris Lord (possibly even including pair programming???). However, my schedule doesn't really have any holes of this size for at least a month. Is waiting a month for Myles's review a requirement of landing this patch? I hope not. But I'd be happy to do it. 3. I'm not sure I understand Ryosuke's logic in https://lists.webkit.org/pipermail/webkit-dev/2019-October/030855.html "The thread unsafe code can be turned into an attack gadget if it exists at all in production binary." The whole reason we're having this discussion is that WebCore is full of thread-unsafe code. I don't understand the value of compiling out some possibly-unsafe code when lots of definitely-unsafe code still exists in the binary. If we disable OffscreenCanvas at runtime, then an uncompromised process won't be able to use it, and if the process is already compromised, then compiling out OffscreenCanvas won't help us. Therefore, regarding this specific particular patch, I can't think of a reason to not land it, because it keeps OffscreenCanvas disabled at runtime. 4. It's difficult to believe that the state of the software engineering industry is that we have no quantitative way to measure how safe multi-threaded code is. Relying on Myles's review, instead of using industry-standard tools, seems like a mistake. Surely we should start this process by using TSan or something else. I think TSan operates at runtime, rather than compile-time, so maybe there are multiple tools we can use. If you asked for my review, the first thing I would try to do would be to determine through the static call graph all of the singleton objects that are reachable from the OffscreenCanvas API - and surely there are better methods of doing this than code inspection by hand. 5. From memory, I can think of plenty of sandbox restriction patches that landed without review of their area expert, and without a thorough understanding of what behavior will be the result of just blocking various services. Our strategy for much of this work, as far as I can tell, is to land the patch early, then fixup and breakage that occurred as we learn about it. Either this sandboxing work was a violation of the policy that Alexey is describing, or this sandboxing work counts as precedent for this particular methodology of software engineering. 6. One potential path forward is to 1) first, strategize about the right time of year with relevant party's schedules to do this, and then 2) enable the feature just in layout tests, and watch the flakey test rate. Reverting the patch that enables the feature is trivial. While waiting for the right time of year, we can use tools to try to find threading problems, and we can have relevant people review when it is convenient for them. (I do, however, understand if Chris Lord is not thrilled about the prospect waiting months for the stars to align in just the right way to try out this feature in tests. Sam's suggestion of enabling a limited subset of the tests which expect to behave better - because they only use a single thread - seems like a good halfway-step.) I want to be sensitive to the fact that A) OffscreenCanvas is a valuable, powerful, and highly requested feature, and B) Chris Lord and Sam Weinig can choose any projects they want to contribute to. I don't think it's fair to say that all code changes in a particular area need to be reviewed by a particular person, or even a particular company, and that Chris or Sam have to petition specific people to land a patch _that still keeps the feature off_. Good software engineering practices are objective, not subjective.

(In reply to Myles C. Maxfield from comment #30) > or even a particular company Excepting platform-specific code, of course.

Thanks for everyone's comments and discussion! Taking a pragmatic view, I'd like to break down how OffscreenCanvas is organised at the moment and what I think needs checking. OffscreenCanvas, in terms of what's exposed to the web consists of two new objects, OffscreenCanvas (in WebCore/html) and OffscreenCanvasRenderingContext2D (in WebCore/html/canvas). Implementation-detail wise, there's also PlaceholderRenderingContext (in WebCore/html/canvas) and ImageBufferPipe (in platform/graphics). OffscreenCanvas: https://html.spec.whatwg.org/multipage/canvas.html#offscreencanvas OffscreenCanvasRenderingContext2D: https://html.spec.whatwg.org/multipage/canvas.html#offscreencanvasrenderingcontext2d When working on the main thread, this operates identically to HTMLCanvasElement, except there isn't any element. Functions to render to an HTML canvas element still work on the main thread, it would just be somewhat pointless. I think we can ignore this particular path in the discussion, there obviously aren't any threading issues there. When working on a Worker thread, OffscreenCanvas still works much like HTMLCanvasElement, but WorkerGlobalScope provides many of the data structures that on the main thread would be singletons. The interface for these is on ScriptExecutionContext and includes a CSSValuePool, a FontCache and a CSSFontSelector, all of which are created on-demand and per-Worker. The singleton versions of these assert if you try to access them off of the main thread. Custom font loading is abstracted behind FontLoadRequest (in WebCore/loader) and ScriptExecutionContext implements the necessary functions to create and handle requests. On Document, this uses the pre-existing path using CachedResourceLoader, on WorkerGlobalScope this uses ThreadableLoader (similarly to how Worker script loading works). When control of a canvas element has been transferred to an OffscreenCanvas, ImageBufferPipe is used to provide a platform layer for rendering via PlaceholderRenderingContext. This part is platform-specific and is currently only implemented for Linux (in platform/graphics/nicosia/NicosiaImageBufferPipe.cpp) - this part I believe is the only platform-specific part and is necessary for asynchronous composition. Not implementing this just means you lose asynchronous composition, synchronous updates will still work. In terms of code that isn't entirely newly-written that is now exposed to Workers, this is mostly encapsulated in ImageBuffer/GraphicsContext, CSSFontSelector and FontCache. At least on Linux, GraphicsContext is a pretty thin wrapper over cairo and, at least as far as I'm aware, doesn't rely on static data, so I'm not too concerned about this one. The last two have been extensively modified to work in this context, but I don't know much about the platform-specific implementations to know what static data there needs to be dealt with. As much as possible, I've tried to make all static data assert when accessed off the creation thread. I haven't spent much time looking at WebGL with OffscreenCanvas, and this may also be an area that requires a more extensive audit. This part can also easily be disabled if need be. All changes have of course been discussed and reviewed, some in quite some depth over quite a long period of time - perhaps I'm misinterpreting, but it does feel like there's some idea that this code has come out of the blue. On the contrary, it's been quite a long time in the making and landed very gradually, with tests (on GTK/WPE only, unfortunately). (In reply to Myles C. Maxfield from comment #30) > 2. I would be happy to spend significant time doing a deep audit of > OffscreenCanvas's font code, in close contact with Chris Lord (possibly even > including pair programming???). However, my schedule doesn't really have any > holes of this size for at least a month. Is waiting a month for Myles's > review a requirement of landing this patch? I hope not. But I'd be happy to > do it. I'm also happy to do this if this is what's necessary, and would understand if that's the case. What's being asked is definitely not feasible for one person, so I very much appreciate the offer! > 5. From memory, I can think of plenty of sandbox restriction patches that > landed without review of their area expert, and without a thorough > understanding of what behavior will be the result of just blocking various > services. Our strategy for much of this work, as far as I can tell, is to > land the patch early, then fixup and breakage that occurred as we learn > about it. Either this sandboxing work was a violation of the policy that > Alexey is describing, or this sandboxing work counts as precedent for this > particular methodology of software engineering. For what it's worth, similar things to this in Gecko land behind prefs which are turned on in nightly builds to get testing and automatically disabled for subsequent tiers of release until they are deemed stable enough. They're also not shy to revert things if they cause failures that can't be immediately fixed. This does sound like what Myles is suggesting, to me. I do question the feasibility of ever being able to land a feature like this if the bar is so high to even just enable building/testing. If we enabled testing and discovered it caused a bunch of issues, I don't think anyone would be upset if it needed to be subsequently disabled, but that period of testing would definitely be useful to discover and log potential issues. > 6. One potential path forward is to 1) first, strategize about the right > time of year with relevant party's schedules to do this, and then 2) enable > the feature just in layout tests, and watch the flakey test rate. Reverting > the patch that enables the feature is trivial. While waiting for the right > time of year, we can use tools to try to find threading problems, and we can > have relevant people review when it is convenient for them. (I do, however, > understand if Chris Lord is not thrilled about the prospect waiting months > for the stars to align in just the right way to try out this feature in > tests. Sam's suggestion of enabling a limited subset of the tests which > expect to behave better - because they only use a single thread - seems like > a good halfway-step.) > > I want to be sensitive to the fact that A) OffscreenCanvas is a valuable, > powerful, and highly requested feature, and B) Chris Lord and Sam Weinig can > choose any projects they want to contribute to. I don't think it's fair to > say that all code changes in a particular area need to be reviewed by a > particular person, or even a particular company, and that Chris or Sam have > to petition specific people to land a patch _that still keeps the feature > off_. Good software engineering practices are objective, not subjective. I think this is a reasonable path forward. I don't see there being any reasonable objection to at least building the code and running the non-worker tests. There is no off-main-thread interaction in this case and it would definitely help prevent breakage and reduce maintenance burden. I'll have to discuss with my colleagues to make sure, but I also don't see it being a problem for me being able to spend time on this further down the line, so I can be considered to be on standby to help with verifying the worker part of this feature, even if we have to wait some unspecified amount of time to do so.

This got enabled in bug 247107.