Currently, each <canvas> has its own associated GLContext. Because of this, it's very easy to run out of these or at least use a ton of memory. We should limit this to some sane value, and demote older canvases to software once the limit is reached. The test_canvas mochitest is a great example of how things can go bad here.

Comment on attachment 768437 [details] [diff] [review] Limit number of GLContexts used with SkiaGL Review of attachment 768437 [details] [diff] [review]: ----------------------------------------------------------------- r=me, but with a suggestion to make it nicer: ::: content/canvas/src/CanvasRenderingContext2D.cpp @@ +106,5 @@ > #include "SurfaceTypes.h" > using mozilla::gl::GLContext; > using mozilla::gl::GLContextProvider; > + > +static nsTArray<mozilla::dom::CanvasRenderingContext2D*>* sDemotableContexts = nullptr; I understand that you had to make this a pointer to avoid having a static constructor. And then you have to new' this nsTArray the first time it's used. But you have a better way to achieve this. The trick is that static local variables at function scope are only constructed when first called, so you as long as you have static local variables (as opposed to globals) you need not worry with that problem at all. So do this: static nsTArray<mozilla::dom::CanvasRenderingContext2D*>& DemotableContexts() { static nsTArray<mozilla::dom::CanvasRenderingContext2D*> value; return value; } ::: gfx/skia/src/gpu/gl/GrGLCaps.h @@ +183,5 @@ > > /// Is GL_BGRA supported > bool bgraFormatSupport() const { return fBGRAFormatSupport; } > > + /// Is GL_RGBA supported with glReadPixels() Did you actually mean to add a comment to this skia file? Won't this just make rebasing harder?

(In reply to Benoit Jacob [:bjacob] from comment #2) > Comment on attachment 768437 [details] [diff] [review] > Limit number of GLContexts used with SkiaGL > > Review of attachment 768437 [details] [diff] [review]: > ----------------------------------------------------------------- > > r=me, but with a suggestion to make it nicer: > > ::: content/canvas/src/CanvasRenderingContext2D.cpp > @@ +106,5 @@ > > #include "SurfaceTypes.h" > > using mozilla::gl::GLContext; > > using mozilla::gl::GLContextProvider; > > + > > +static nsTArray<mozilla::dom::CanvasRenderingContext2D*>* sDemotableContexts = nullptr; > > I understand that you had to make this a pointer to avoid having a static > constructor. And then you have to new' this nsTArray the first time it's > used. But you have a better way to achieve this. The trick is that static > local variables at function scope are only constructed when first called, so > you as long as you have static local variables (as opposed to globals) you > need not worry with that problem at all. So do this: > > static nsTArray<mozilla::dom::CanvasRenderingContext2D*>& > DemotableContexts() { > static nsTArray<mozilla::dom::CanvasRenderingContext2D*> value; > return value; > } That's a nice trick. Won't that trigger the leak checker on shutdown though? > > ::: gfx/skia/src/gpu/gl/GrGLCaps.h > @@ +183,5 @@ > > > > /// Is GL_BGRA supported > > bool bgraFormatSupport() const { return fBGRAFormatSupport; } > > > > + /// Is GL_RGBA supported with glReadPixels() > > Did you actually mean to add a comment to this skia file? Won't this just > make rebasing harder? Oops that's stuff from another patch. Fixed.

(In reply to James Willcox (:snorp) (jwillcox@mozilla.com) from comment #3) > (In reply to Benoit Jacob [:bjacob] from comment #2) > > Comment on attachment 768437 [details] [diff] [review] > > Limit number of GLContexts used with SkiaGL > > > > Review of attachment 768437 [details] [diff] [review]: > > ----------------------------------------------------------------- > > > > r=me, but with a suggestion to make it nicer: > > > > ::: content/canvas/src/CanvasRenderingContext2D.cpp > > @@ +106,5 @@ > > > #include "SurfaceTypes.h" > > > using mozilla::gl::GLContext; > > > using mozilla::gl::GLContextProvider; > > > + > > > +static nsTArray<mozilla::dom::CanvasRenderingContext2D*>* sDemotableContexts = nullptr; > > > > I understand that you had to make this a pointer to avoid having a static > > constructor. And then you have to new' this nsTArray the first time it's > > used. But you have a better way to achieve this. The trick is that static > > local variables at function scope are only constructed when first called, so > > you as long as you have static local variables (as opposed to globals) you > > need not worry with that problem at all. So do this: > > > > static nsTArray<mozilla::dom::CanvasRenderingContext2D*>& > > DemotableContexts() { > > static nsTArray<mozilla::dom::CanvasRenderingContext2D*> value; > > return value; > > } > > That's a nice trick. Won't that trigger the leak checker on shutdown though? This is just a nsTArray of POD's (pointers). Are our leak tools guarding these, or are they specifically about refcounted objects?

Note: if our leak tools are annoying about that, you could use a plain static array given that the size is known at compile time, or you could use a std::vector if runtime-sizing matters.

Comment on attachment 768711 [details] [diff] [review] Limit number of GLContexts used with SkiaGL Review of attachment 768711 [details] [diff] [review]: ----------------------------------------------------------------- If our leak tools give you trouble, just use something else than nsTArray as discussed above.