Bug 215599

Created attachment 406895 [details] Patch

The style checker is overzealous in this case; there's no existing bug, but one would have been introduced in earlier versions of this patch.

Comment on attachment 406895 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=406895&action=review > Source/WebCore/html/canvas/ANGLEInstancedArrays.cpp:37 > -ANGLEInstancedArrays::ANGLEInstancedArrays(WebGLRenderingContextBase& context) > +ANGLEInstancedArrays::ANGLEInstancedArrays(WebGLRenderingContextBase* context) Since the context must not be null, then it still should be passed as a reference. > Source/WebCore/html/canvas/ANGLEInstancedArrays.cpp:41 > + context->graphicsContextGL()->getExtensions().ensureEnabled("GL_ANGLE_instanced_arrays"); See, we dereference it here without checking for null.

Comment on attachment 406895 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=406895&action=review > Source/WebCore/html/canvas/WebGLExtension.cpp:41 > +WebGLExtension::WebGLExtension(WebGLRenderingContextBase* context) > : m_context(context) > { > } So I guess this is where Darin is saying it should be a &, but leave m_context as a *. > Source/WebCore/html/canvas/WebGLExtension.h:79 > + // Lose this extension. Passing true means force loss of the > + // extension. Some extensions like WEBGL_lose_context are not > + // normally lost when the context is lost, but must be lost when > + // destroying their WebGLRenderingContextBase. > + virtual void lose(bool); > + bool isLost() { return !m_context; } I guess an alternative would be for the extension to hold a WeakPtr to WebGLRenderingContextBase, which would avoid having to call lose(). I'm not sure that's better, and I guess you'd still need a way to force it on WebGLLoseContext. Could we use a more descriptive name for the parameter to lose()? We already have WebGLRenderingContextBase::LostContextMode. Can we use it? Would it be clear that RealLostContext is a forced one? Maybe we could rename LostContextMode... enum class LoseContextMode { NotForced, Forced }; virtual void loseContext(LoseContextMode == NotForced); I also think loseContext over lose, since that's what it is actually doing. > Source/WebCore/html/canvas/WebGLRenderingContextBase.cpp:1031 > + loseExtensions(true); So if it was a WeakPtr this is where it would automatically disappear. > Source/WebCore/html/canvas/WebGLRenderingContextBase.cpp:6097 > + loseExtensions(mode == RealLostContext); If we use LostContextMode we can just pass this mode along.

Comment on attachment 406895 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=406895&action=review >> Source/WebCore/html/canvas/ANGLEInstancedArrays.cpp:37 >> +ANGLEInstancedArrays::ANGLEInstancedArrays(WebGLRenderingContextBase* context) > > Since the context must not be null, then it still should be passed as a reference. I see. OK. Reverted this here and throughout. >> Source/WebCore/html/canvas/ANGLEInstancedArrays.cpp:41 >> + context->graphicsContextGL()->getExtensions().ensureEnabled("GL_ANGLE_instanced_arrays"); > > See, we dereference it here without checking for null. Understood. Done. >> Source/WebCore/html/canvas/WebGLExtension.cpp:41 >> } > > So I guess this is where Darin is saying it should be a &, but leave m_context as a *. Understood. Made this change throughout. >> Source/WebCore/html/canvas/WebGLExtension.h:79 >> + bool isLost() { return !m_context; } > > I guess an alternative would be for the extension to hold a WeakPtr to WebGLRenderingContextBase, which would avoid having to call lose(). I'm not sure that's better, and I guess you'd still need a way to force it on WebGLLoseContext. > > Could we use a more descriptive name for the parameter to lose()? > > We already have WebGLRenderingContextBase::LostContextMode. Can we use it? > Would it be clear that RealLostContext is a forced one? Maybe we could rename LostContextMode... > > enum class LoseContextMode { > NotForced, > Forced > }; > > virtual void loseContext(LoseContextMode == NotForced); > > I also think loseContext over lose, since that's what it is actually doing. Thanks for the suggestion. I would like to be more explicit and avoid using WeakPtr to potentially sweep lifetime issues under the rug. Additionally, extensions' lose() (now loseParentContext()) has to zero out the connection back to the parent context on demand. Have taken your suggestion to take WebGLRenderingContextBase::LostContextMode here. Didn't want to rename the enum values since I found NotForced / Forced hard to reason about. >> Source/WebCore/html/canvas/WebGLRenderingContextBase.cpp:1031 >> + loseExtensions(true); > > So if it was a WeakPtr this is where it would automatically disappear. Understood, but I'd like to keep this explicit. >> Source/WebCore/html/canvas/WebGLRenderingContextBase.cpp:6097 >> + loseExtensions(mode == RealLostContext); > > If we use LostContextMode we can just pass this mode along. Done.

Created attachment 406972 [details] Patch

Comment on attachment 406972 [details] Patch Addressed previous review feedback. Marking cq+.

Comment on attachment 406972 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=406972&action=review > Source/WebCore/html/canvas/OESVertexArrayObject.cpp:63 > + auto locker = holdLock(m_context->objectGraphLock()); What is this lock locking? Can't it be moved after the next if-statement which returns from this function? > Source/WebCore/html/canvas/OESVertexArrayObject.cpp:66 > - if (!arrayObject || m_context.isContextLost()) > + if (!arrayObject || m_context->isContextLost()) > return; It looks like this if-statement can be combined with the one at the top. if (!arrayObject || !m_context || m_context->isContextLost()) return; > Source/WebCore/html/canvas/OESVertexArrayObject.cpp:80 > + if (!m_context) > + return false; > + > + return arrayObject && !m_context->isContextLost() && arrayObject->hasEverBeenBound() > + && m_context->graphicsContextGL()->getExtensions().isVertexArrayOES(arrayObject->object()); I think these two statements can be combined in one statement: return arrayObject && m_context && !m_context->isContextLost() && ... > Source/WebCore/html/canvas/OESVertexArrayObject.cpp:88 > + if (!m_context) > + return; > + > + auto locker = holdLock(m_context->objectGraphLock()); Ditto. Do we need to check arrayObject is not null as we did above ? > Source/WebCore/html/canvas/WebGL2RenderingContext.cpp:2663 > #define ENABLE_IF_REQUESTED(type, variable, nameLiteral, canEnable) \ Is there a reason for not implementing this as a template function? A template function will act the same the same as macro but it will give better compilation errors and it is debuggable. > Source/WebCore/html/canvas/WebGLExtension.h:79 > + virtual void loseParentContext(WebGLRenderingContextBase::LostContextMode); I think this function should return a bool. See my comment below where it is called. > Source/WebCore/html/canvas/WebGLRenderingContextBase.cpp:7759 > +#define LOSE_EXTENSION(variable) \ > + if (variable) { \ > + variable->loseParentContext(mode); \ > + if (variable->isLost()) \ > + (void) variable.releaseNonNull(); \ > + } The same comment about making this macro a template function. Also can we make loseParentContext() return a bool indicating whether it succeeded or not? In this case, this code can be written like this: if (variable && variable->loseParentContext(mode)) ... I am also puzzled by why we need to call variable.releaseNonNull() instead of just calling variable = nullptr;

Committed r265975: <https://trac.webkit.org/changeset/265975> All reviewed patches have been landed. Closing bug and clearing flags on attachment 406972 [details].

<rdar://problem/67511049>

sabouhallawa@ thanks for your feedback. Most of it focuses on code structure which was there previously to my patch. I'd be happy to address it if you'd like to file another bug and block it on this one.

I think there is another thread here worth following up on. Increasingly, the WebKit project is using smart pointers to manage pointer lifetime in a mechanical, low-level way that is easy to get correct from a security perspective. This has greatly increased our use of WeakPtr for pointers to objects where we once used raw pointers. In many cases, this WeakPtr use is redundant with explicit lifetime management. Functions like "detach" or "lose" or "clear" that explicitly manage the pointer. And of course, those functions are more flexible than WeakPtr: they can clear out the pointer at the correct logical time when it’s not the same time that the object is being destroyed. However, for security hardening, wider use of WeakPtr is desirable. We don’t want mistakes to lead to dangling pointers. It's valuable to mitigate this effect, even if those mistakes would do other harm. This makes me slightly concerned about this remark from a project strategy point of view: "I would like to be more explicit and avoid using WeakPtr to potentially sweep lifetime issues under the rug." I think it’s great if we can smoke out lifetime mistakes, especially in debug builds. If WeakPtr makes it more difficult to do that, we should consider how to enhance it so that it doesn’t get in the way. This "sweeping of lifetime issues under the rug" is exactly what we *want* from WeakPtr to help prevent our mistakes from being exploitable security vulnerabilities.

Note that this patch caused significant regression Bug 216337.