410392 - Stack-allocate gfxContext (and friends) in /widget/

Status: RESOLVED WONTFIX

(Core :: Widget, defect, P3)

Description

[Alfred Kayser]

Spinoff from bug 383166

Comment 1

[Alfred Kayser]

See attachment 2 [details] [diff] [review] for the widget part. Note however this needs some more testing and research, as in some cases the stack-allocated object is then passed on. Not sure if that is completely safe.

Comment 2

[Peter Weilbacher]

OK, I applied attachment 294250 [details] [diff] [review] from bug 383166 and rebuilt SeaMonkey debug on OS/2. It crashes with a double free exception and a call stack passing through nsWindow::OnPaint(), nsCOMPtr<nsIRenderingContext>::~nsCOMPtr(), ..., and gfxContext::Release() already on startup. As I modeled the code in OnPaint() after the respective Windows painting code, there could be a similar problem there... When backing out the OS/2 part of the patch it seems to run, even canvases display correctly with the canvas part in.

Comment 3

[Alfred Kayser]

That's why I didn't want to check that part in yet.

Comment 4

[Alfred Kayser]

Found the issue: when passing a pointer to stack-allocated nsISupports objects to someone doing ref-counting, makes the ref-counting code then tries to delete the stack-allocated object. Simply do NS_ADDREF to prevent deallocation of stack objects

Comment 5

[Alfred Kayser]

Attachment: V1: Put gfxContext and gfxSurface on the stack in widget & canvas

To be able to stack-allocate gfxImageSurface in Windows/nsWindow.cpp some code reshuffling is needed.

Comment 6

[Stuart Parmenter]

I really do'nt like all these manual addrefs.  There has to be a better way to do this.

Comment 7

[Alfred Kayser]

Possibly, if someone provides me some clue here. The point is that these 'objects' are not allocated as 'object' and should never be released, so we need to make sure that for these 'stack based instances', the refcount never reached zero. NS_ADDREF seems to be the most simple solution here. 

Comment 8

[Olli Pettay [:smaug][[e-mail]]]

Sounds evil to stack allocate refcounted objects.
Would it be possible to make those non-refcounted, or have
AddRef/Release No-Ops (and document that) - similar to nsIFrame objects?

Comment 9

[Alfred Kayser]

That would mean that we must create special classes for these (gfxContextStack or such?). Note that the mozilla codebase is trying to move from reference counting to garbage collection where this is all no longer relevant.

Comment 10

[Olli Pettay [:smaug][[e-mail]]]

(In reply to comment #9)
> Note that the mozilla codebase is trying to move from reference
> counting to garbage collection where this is all no longer relevant.
That is moz2. I assume this bug is something to be fixed in 1.9.
And stack based gc-able-objects in a gc-system wouldn't work well either.

Comment 11

[Olli Pettay [:smaug][[e-mail]]]

(But I'm not a gfx peer or anything so if stuart accepts the approach, I won't
 complain more :) )

Comment 12

[Alfred Kayser]

I still think that using NS_ADDREF is a simple solution to make some objects stack based. For Moz2 a better solution is still required.

A total different approach is to make 'gfxContext' a member of the renderingContext where it is used, instead of allocating it separately. (RC's are the only users of refcounted gfxContext).

Similar would be to combine gfxImageSurface and gfxContext into a 'gfxImageContext', to prevent separate allocation of these.

But this is all way more complex than just adding a few NS_ADDREF's to the stack allocated objects (which only say: This function has a strong reference to this object because it is stack allocated).

Comment 13

[Olli Pettay [:smaug][[e-mail]]]

(In reply to comment #12)
> I still think that using NS_ADDREF is a simple solution to make some objects
> stack based.
That works only if it can be ensured that no one holds a pointer to the object
after the block, in which the stack object is declared, is cleared.
If that it the case with gfxXXX objects, fine, but must be, IMO, also documented
somewhere that one must not hold an owning pointer to such object longer than
the lifetime of the block.

Comment 14

[Mike Schroepfer]

Yea - want this for b3...

Comment 15

[Stuart Parmenter]

vlad: any thoughts on a better solution here?

Comment 16

[Stuart Parmenter]

also, we may want to revert 383166 or apply a similar solution to it.

Comment 17

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

We can stack-allocate gfxContext, but we cannot stack allocate any gfx*Surface.   The problem is that the reference count for gfxSurfaces is tied to the cairo surface refcount; calling AddRef/Release just does a cairo_surface_reference()/cairo_surface_release().  The actual wrapper object is stored as user data on the surface, and is deleted via a user data destructor callback, again all within the cairo surface code.  Doing it this way is pretty much required, because otherwise it's a disaster to keep track of refcounts (because we might need to wrap surfaces we get from cairo, not just ones we create ourselves).

If we call NS_ADDREF on a stack object, we'll end up addrefing the cairo surface and then never releasing it.  If we do release it in some way, we'll end up crashing, because the destructor code from the wrapper callback will try to free the wrapper.

Note that the only thing we're stack allocating here is the gfx*Surface -- we're always going to allocate the cairo_surface_*_t, because we don't control that.  If this is showing up as a problem, we can allocate all wrappers via a slab allocator.

Comment 18

[Mike Schroepfer]

Pav - what we want to do here?

Comment 19

[Stuart Parmenter]

this shouldn't block.  it won't make a huge difference, but would be nice to do if we can figure out a good way to do it.

Comment 20

[Alfred Kayser]

Attachment: V2: Only put those on the stack that are not referred by pointer

This patch only puts those gfxContext instances on the stack that will not be referenced by pointer.

Comment 21

[Alfred Kayser]

Requesting 'wanting1.9' for the smaller and safer patch.

Comment 22

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

I think this is a WONTFIX -- out of the safer bits, 3 of them are only used when dragging (and only once), and the fourth is only used when doing translucent windows under Gtk.  An extra allocation in any of these cases won't make any difference, and I don't think we should get in the habit of stack-allocating refcounted objects (at the very least, the code would be confusing to someone reading it who might not understand why it's OK to do it there but not elsewhere).