Bug 225304

Created attachment 427643 [details] Patch

Just a note, I guess there were 3 'obvious' options to fix this; 1- Figure out how to decouple GraphicsContext from ImageBuffer 2- Replay the state stack on a new GraphicsContext 3- Return a copy of the buffer instead of the original buffer I went with option 3 as it's by far the easiest, smallest and least invasive change. I played around with option 2 and it's very feasible, but it feels a bit hacky. I didn't even attempt option 1 as that seems like a pretty huge departure from how things are setup right now. The downside of option 3 is that you end up with a copy and a clear rather than just an allocate and clear, but given transferToImageBitmap isn't really expected to be fast, this doesn't seem unreasonable to me (we have similar work-arounds for similar problems elsewhere in canvas). This is one of those areas where I feel dogged adherence to the spec is actually just harming both browsers and users, but it's tested in WPT and it's pretty low effort to fix this.

Comment on attachment 427643 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=427643&action=review > Source/WebCore/html/OffscreenCanvas.cpp:271 > + // As the canvas context state is stored in GraphicsContext, which is tightly bound > + // to buffer(), to avoid resetting the context state, we have to make a copy and > + // clear the original buffer rather than returning the original buffer. The comment "tightly bound to buffer()" seems to say that we can’t possibly get a GraphicsContext to release its current buffer and allocate a new one. Is that true, maybe due to limitations to CGGraphicsContext on Apple’s platforms, or is this simply new work we’d need to do in GraphicsContext? > Source/WebCore/html/OffscreenCanvas.cpp:272 > + ASSERT(buffer() && drawingContext()); Maybe assert these separately? But also, we don’t normally have to assert pointers are non-null; dereferencing them usually hits a null dereference quickly without us writing any additional code. And also, if we are using m_context below, and want to assert it’s not null, then we should assert that rather than drawingContext().

Created attachment 427748 [details] Patch

(In reply to Darin Adler from comment #3) > Comment on attachment 427643 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=427643&action=review > > > Source/WebCore/html/OffscreenCanvas.cpp:271 > > + // As the canvas context state is stored in GraphicsContext, which is tightly bound > > + // to buffer(), to avoid resetting the context state, we have to make a copy and > > + // clear the original buffer rather than returning the original buffer. > > The comment "tightly bound to buffer()" seems to say that we can’t possibly > get a GraphicsContext to release its current buffer and allocate a new one. > > Is that true, maybe due to limitations to CGGraphicsContext on Apple’s > platforms, or is this simply new work we’d need to do in GraphicsContext? I've changed this comment, I don't think I picked my words well - the GraphicsContext is created and owned by the ImageBuffer and CanvasBase always accesses it through the image buffer. I think being able to release the associated surface and recreate one would involve some platform-specific plumbing that isn't currently there and may be non-trivial. It would be a lot easier to do option 2 in comment #2 than this. > > Source/WebCore/html/OffscreenCanvas.cpp:272 > > + ASSERT(buffer() && drawingContext()); > > Maybe assert these separately? > > But also, we don’t normally have to assert pointers are non-null; > dereferencing them usually hits a null dereference quickly without us > writing any additional code. > > And also, if we are using m_context below, and want to assert it’s not null, > then we should assert that rather than drawingContext(). I've just removed this assert, none of those pointers can be null at that point unless something went catastrophically wrong. m_context is checked at the start of the function and nothing is called that can destroy it, buffer() is checked just above with the !m_hasCreatedImageBuffer check and drawingContext() can only be null if m_context isn't a 2d context (it is and is checked within this block) and buffer() returns null (which shouldn't happen because m_hasCreatedImageBuffer is true).

Committed r277011 (237327@main): <https://commits.webkit.org/237327@main> All reviewed patches have been landed. Closing bug and clearing flags on attachment 427748 [details].

<rdar://problem/77548954>