655017 - Panorama: Crash (X_GLXCreatePixmap: BadAlloc) with layers.acceleration.force-enabled=true

Status: RESOLVED FIXED

(Core :: Graphics, defect)

Description

[Constantine]

User-Agent:       Mozilla/5.0 (Windows NT 6.1; rv: 2.0b12) Gecko/20101009 Firefox/4.0 YB/3.5.1
Build Identifier: Mozilla/5.0 (X11; Linux x86_64 rv: 6.0a1) Gecko/20110505 Firefox/6.0a1

Firefox crashes when I enter Panorama with about:config option layers.acceleration.force-enabled=true. When I do that by running firefox from terminal I get such message:
###!!! ABORT: X_GLXCreatePixmap: BadAlloc (insufficient resources for operation); 2 requests ago: file /builds/slave/cen-lnx64-ntly/build/toolkit/xre/nsX11ErrorHandler.cpp, line 199

Reproducible: Always

Steps to Reproduce:
1.Create a clean profile
2.Open about:config & switch layers.acceleration.force-enabled to true
3.Restart firefox
4.Enter Panorama and get a crash

Actual Results:  
Crash

Expected Results:  
Normal working Panorama

Ubuntu 10.10, nvidia proprietary driver (version 270.41.06), videocard - GeForce 210

Comment 1

[Matt Woodrow (:mattwoodrow)]

Hrm, BadAlloc could happen from X not being able to allocate a new GLX window (insufficient memory or vram maybe?) or because we have already called this for the given pixmap.

I can't reproduce this on my system, leaning towards insufficient memory issues.

Is this reproducible for you? Any other steps required to consistently crash? How much memory do you have? In use? Vram?

Comment 2

[Constantine]

Fixed in latest nightlies, but scrolling ( and in general graphics ) strangely became much slower. For example, http://demos.hacks.mozilla.org/openweb/HWACCEL/ runs only with 10 FPS instead of ~40.
I don't know a command to know how much vram is used, but any other applications work normally, system memory usage is not high. Please, pay attention to this. I mark the bug as resolved.

Comment 3

[Constantine]

With today's nightly Panorama began crashing again (but hardware acceleration works even faster now). Please, tell me the way how can I inspect my vram etc. to give youmore information about issue and to find out what happens.

Comment 4

[Constantine]

Attachment: about:memory

Comment 5

[=Haron=]

Ubuntu 10.04.2 x86_64
X.Org Server 1.8.2
Nvidia GTX260 driver 270.29

Bug confirmed

Comment 6

[=Haron=]

Ubuntu 10.04.2 x86_64
X.Org Server 1.8.2
Nvidia GTX260 driver 270.29

Bug confirmed

:::: Mozilla/5.0 (X11; Linux x86_64; rv:7.0a1) Gecko/20110607 Firefox/7.0a1 #1307510638

Comment 7

[Constantine]

Today I got the similar error at http://chrome.angrybirds.com. The bug seems not to depend on Panorama. Maybe wrong memory allocation somewhere?

Comment 8

[Constantine]

Almost 2 months have gone but nothing changes.

Comment 9

[Matt Woodrow (:mattwoodrow)]

Attachment: Let gfxXlibSurface cache GLXPixmaps and use this for CanvasLayerOGL

Sorry, I've been busy with other project and haven't had much time to look at GLX.

Looks like this crash is from creating a GLXPixmap for an X pixmap when it already has one. The CanvasLayer creation code is creating a new CanvasLayer and calling Initialize on it when an old one still exists with the same backing surface.

This patch moves ownership of the GLXPixmap to the gfxXlibSurface so that only one is created and reused.

We could probably use this code path for some of the other layer types too.

Comment 10

[Joe Drew (not getting mail)]

Comment on attachment 549000 [details] [diff] [review]
Let gfxXlibSurface cache GLXPixmaps and use this for CanvasLayerOGL

Not sure who Matt meant to ask for review here, but let's give it to some of the usual suspects.

Comment 11

[Constantine]

The patch works for me.

Comment 12

[Karl Tomlinson (:karlt)]

Comment on attachment 549000 [details] [diff] [review]
Let gfxXlibSurface cache GLXPixmaps and use this for CanvasLayerOGL

I have to admit that I don't know enough about layer building to know why the
old layer needs to be kept past the new layer creation.  Are you able to
explain for me, please?

I think the gfxXlibSurface changes are good, but you should probably get a gfx
peer to sr/rs.  What is a bit awkward here is that the gfx surface wraps a
drawable which may be a window or a pixmap, but GetGLXPixmap() is only
suitable for pixmaps (even though an implementation probably wouldn't make the
distinction).  There are already xlib surface methods that only make sense for
pixmaps (Take/ReleasePixmap), so I guess this is OK.

It is usual to use "None" for a zero XID.

The change to GLXLibrary::CreatePixmap is addressing a different potential
cause of crash to the one identified in comment 9.  It doesn't seem quite
right to have a ScopedXErrorHandler in all builds but only wait and check the
error in debug builds.  I'm not clear on the value of this but I don't think
we want to wait in opt builds unless we know this is a real issue, and I don't
think we want the wait only in debug builds as that would make the
asynchronous behavior quite different in debug builds.  r- just for this.

Comment 13

[Benoit Jacob [:bjacob] (mostly away)]

Comment on attachment 549000 [details] [diff] [review]
Let gfxXlibSurface cache GLXPixmaps and use this for CanvasLayerOGL

I'm less competent than Karl on X11 and don't know gfxXlibSurface well, so given that Karl has already r-'d I don't think that my reviewing is needed anymore.

Comment 14

[Matt Woodrow (:mattwoodrow)]

(In reply to comment #12)
> Comment on attachment 549000 [details] [diff] [review] [diff] [details] [review]
> Let gfxXlibSurface cache GLXPixmaps and use this for CanvasLayerOGL
> 
> I have to admit that I don't know enough about layer building to know why the
> old layer needs to be kept past the new layer creation.  Are you able to
> explain for me, please?

I don't think it *needs* to, as such, but this is how the current code is structured. FrameLayerBuilder keeps references to the layers from the previous frame, and the canvas layer creation code retrieves this using GetLeafLayerFor(). It then decides if it can re-use this layer, and if not, creates a new one. Only once the new/existing layer is returned back to frame layer builder is the cached reference updated, and the old layer free'd.


> 
> The change to GLXLibrary::CreatePixmap is addressing a different potential
> cause of crash to the one identified in comment 9.  It doesn't seem quite
> right to have a ScopedXErrorHandler in all builds but only wait and check the
> error in debug builds.  I'm not clear on the value of this but I don't think
> we want to wait in opt builds unless we know this is a real issue, and I
> don't
> think we want the wait only in debug builds as that would make the
> asynchronous behavior quite different in debug builds.  r- just for this.

Sure, this was just debugging code and I'll remove it. We don't get callstacks from X errors when they go through GL code, so this just delays the assertion until we return to our code. I might make this a separate patch that implements it as a GLX debug mode similar to MOZ_GL_DEBUG.

Comment 15

[Matt Woodrow (:mattwoodrow)]

Attachment: Let gfxXlibSurface cache GLXPixmaps and use this for CanvasLayerOGL

Removed debugging code

Comment 16

[Karl Tomlinson (:karlt)]

Comment on attachment 549619 [details] [diff] [review]
Let gfxXlibSurface cache GLXPixmaps and use this for CanvasLayerOGL

(In reply to comment #14)
FrameLayerBuilder keeps references to the layers from the
> previous frame, and the canvas layer creation code retrieves this using
> GetLeafLayerFor(). It then decides if it can re-use this layer, and if not,
> creates a new one. Only once the new/existing layer is returned back to
> frame layer builder is the cached reference updated, and the old layer
> free'd.

Thanks.  Perhaps another option is to Destroy() the old layer but Destroy seems to be LayerOGL-specific at the moment and, like you imply, there may be other reasons to share GLXPixmaps.

Comment 17

[Matt Woodrow (:mattwoodrow)]

http://hg.mozilla.org/integration/mozilla-inbound/rev/86192af60310

Comment 18

[Marco Bonardo [:mak]]

http://hg.mozilla.org/mozilla-central/rev/86192af60310

Comment 19

[Dmitry]

I have Gentoo on AMD64.

On firefox-22 with latest nvidia-drivers-325.08 crashes is reappear again with layers.acceleration.force-enabled=true
But now Firefox crashing in any newly opened tab.

Error messages:

###!!! ABORT: X_GLXDestroyPixmap: GLXBadPixmap; 4 requests ago: file /home/tmp/portage/www-client/firefox-22.0/work/mozilla-release/toolkit/xre/nsX11ErrorHandler.cpp, line 157
###!!! ABORT: X_GLXDestroyPixmap: GLXBadPixmap; 4 requests ago: file /home/tmp/portage/www-client/firefox-22.0/work/mozilla-release/toolkit/xre/nsX11ErrorHandler.cpp, line 157

or

###!!! ABORT: X_GLXCreatePixmap: BadMatch (invalid parameter attributes); 57 requests ago: file /home/tmp/portage/www-client/firefox-22.0/work/mozilla-release/toolkit/xre/nsX11ErrorHandler.cpp, line 157
###!!! ABORT: X_GLXCreatePixmap: BadMatch (invalid parameter attributes); 57 requests ago: file /home/tmp/portage/www-client/firefox-22.0/work/mozilla-release/toolkit/xre/nsX11ErrorHandler.cpp, line 157

Please reopen that bug. With layers.acceleration.force-enabled=false browser works fine.
For me, layers.acceleration.force-enabled=true is the only way to prevent tearing in Firefox.

Comment 20

[Raghavendra Prabhu]

I can confirm the crash and workaround mentioned in comment 19, with firefox 23,24,25 and nvidia driver - 325.15

Comment 21

[Karl Tomlinson (:karlt)]

Comment 19 is bug 896287.