Incomplete framebuffer abort in mozilla::layers::LayerManagerOGL::CreateFBOWithTexture with "error 0x8cdd"

RESOLVED INVALID

Status

()

Core
Graphics: Layers
--
critical
RESOLVED INVALID
5 years ago
2 years ago

People

(Reporter: Scoobidiver (away), Assigned: mattwoodrow)

Tracking

({crash, regression, reproducible})

22 Branch
All
Mac OS X
crash, regression, reproducible
Points:
---

Firefox Tracking Flags

(firefox21 unaffected, firefox22+ verified, firefox23+ verified, firefox24 affected, firefox25 affected)

Details

(crash signature, URL)

Attachments

(3 attachments, 2 obsolete attachments)

(Reporter)

Description

5 years ago
It might be related to or a duplicate of bug 721667.

Signature 	mozalloc_abort(char const*) | Abort | NS_DebugBreak | mozilla::layers::LayerManagerOGL::CreateFBOWithTexture(nsIntRect const&, mozilla::layers::LayerManagerOGL::InitMode, unsigned int, unsigned int*, unsigned int*) More Reports Search
UUID	7df2e8d2-0458-40e4-a768-273b12130430
Date Processed	2013-04-30 13:46:22
Uptime	1616
Last Crash	27.3 minutes before submission
Install Age	27.0 minutes since version was first installed.
Install Time	2013-04-30 13:19:11
Product	Firefox
Version	23.0a1
Build ID	20130430030941
Release Channel	nightly
OS	Mac OS X
OS Version	10.8.3 12D78
Build Architecture	x86
Build Architecture Info	GenuineIntel family 6 model 42 stepping 7
Crash Reason	EXC_BAD_ACCESS / KERN_PROTECTION_FAILURE
Crash Address	0x0
App Notes 	
AdapterVendorID: 0x1002, AdapterDeviceID: 0x6740GL Layers? GL Context? GL Context+ GL Layers+ WebGL? WebGL+ xpcom_runtime_abort(###!!! ABORT: Framebuffer not complete -- error 0x8cdd, mFBOTextureTarget 0xde1, aRect.width 16384, aRect.height 16384: file ../../../../gfx/layers/opengl/LayerManagerOGL.cpp, line 1277)
Processor Notes 	sp-processor05.phx1.mozilla.com_19953:2012; exploitability tool: ERROR: unable to analyze dump
EMCheckCompatibility	True
Adapter Vendor ID	0x1002
Adapter Device ID	0x6740

Frame 	Module 	Signature 	Source
0 	libmozalloc.dylib 	mozalloc_abort 	mozalloc_abort.cpp:30
1 	XUL 	Abort 	nsDebugImpl.cpp:430
2 	XUL 	NS_DebugBreak 	nsDebugImpl.cpp:387
3 	XUL 	mozilla::layers::LayerManagerOGL::CreateFBOWithTexture 	LayerManagerOGL.cpp:1277
4 	XUL 	mozilla::layers::ContainerLayerOGL::RenderLayer 	ContainerLayerOGL.cpp:237
5 	XUL 	_ZThn552_N7mozilla6layers17ContainerLayerOGL11RenderLayerEiRK10nsIntPoint 	ContainerLayerOGL.cpp:393
6 	XUL 	mozilla::layers::ContainerLayerOGL::RenderLayer 	ContainerLayerOGL.cpp:275
7 	XUL 	_ZThn552_N7mozilla6layers17ContainerLayerOGL11RenderLayerEiRK10nsIntPoint 	ContainerLayerOGL.cpp:393
8 	XUL 	mozilla::layers::ContainerLayerOGL::RenderLayer 	ContainerLayerOGL.cpp:275
9 	XUL 	_ZThn552_N7mozilla6layers17ContainerLayerOGL11RenderLayerEiRK10nsIntPoint 	ContainerLayerOGL.cpp:393
10 	XUL 	mozilla::layers::LayerManagerOGL::Render 	LayerManagerOGL.cpp:862
11 	XUL 	mozilla::layers::LayerManagerOGL::EndTransaction 	LayerManagerOGL.cpp:562
12 	XUL 	mozilla::layers::LayerManagerOGL::EndEmptyTransaction 	LayerManagerOGL.cpp:503
13 	XUL 	PresShell::Paint 	layout/base/nsPresShell.cpp:5516
14 	XUL 	nsViewManager::Refresh 	view/src/nsViewManager.cpp:330
15 	XUL 	nsViewManager::PaintWindow 	view/src/nsViewManager.cpp:646
16 	XUL 	nsView::PaintWindow 	view/src/nsView.cpp:995
17 	XUL 	nsChildView::PaintWindow 	widget/cocoa/nsChildView.mm:1572
18 	XUL 	-[ChildView drawUsingOpenGL] 	widget/cocoa/nsChildView.mm:2911
19 	XUL 	-[ChildView drawUsingOpenGLCallback] 	widget/cocoa/nsChildView.mm:2926
20 	Foundation 	Foundation@0x794cf 	

More reports at:
https://crash-stats.mozilla.com/report/list?signature=mozalloc_abort%28char+const*%29+|+Abort+|+NS_DebugBreak+|+mozilla%3A%3Alayers%3A%3ALayerManagerOGL%3A%3ACreateFBOWithTexture%28nsIntRect+const%26%2C+mozilla%3A%3Alayers%3A%3ALayerManagerOGL%3A%3AInitMode%2C+unsigned+int%2C+unsigned+int*%2C+unsigned+int*%29
https://crash-stats.mozilla.com/report/list?signature=mozalloc_abort%28char+const*%29+|+NS_DebugBreak+|+HexEncode%28void+const*%2C+unsigned+long%29%3A%3AkHexChars
(Reporter)

Comment 1

5 years ago
It's #4 top browser crasher in 22.0b1 and #5 in 23.0a2 on Mac OS X.

Comments are similar to those of bug 721667:
"Moving/zooming out of map at recreation.gov"
"was zooming in/out on maps.google.com"
status-firefox24: --- → affected
tracking-firefox22: --- → ?
tracking-firefox23: --- → ?
Keywords: topcrash
Joe - do you think this is related to bug 721667 or similar enough that you can take it on?
Assignee: nobody → joe
tracking-firefox22: ? → +
tracking-firefox23: ? → +
Flags: needinfo?(joe)
I didn't even know that I was assigned to that bug! I have no ideas about this bug or that one, unfortunately.
Assignee: joe → nobody
Flags: needinfo?(joe)
Here are steps for 100% repro. You must have Ad Block Plus installed.

1. Install Ad Block Plus: https://adblockplus.org/en/firefox
2. Load http://www.theatlanticcities.com/technology/2013/05/terrifying-fascinating-timelapse-30-years-human-impact-earth-gifs/5540/
3. Scroll down to Google Earth widget
4. Click on the map's zoom control (in the upper left corner)
5. CRASH with these errors:

###!!! ABORT: Framebuffer not complete -- error 0x8cdd, mFBOTextureTarget 0xde1, aRect.width 16384, aRect.height 16384: file /builds/slave/rel-m-beta-osx64_bld-000000000/build/gfx/layers/opengl/LayerManagerOGL.cpp, line 1545
###!!! ABORT: Framebuffer not complete -- error 0x8cdd, mFBOTextureTarget 0xde1, aRect.width 16384, aRect.height 16384: file /builds/slave/rel-m-beta-osx64_bld-000000000/build/gfx/layers/opengl/LayerManagerOGL.cpp, line 1545

6. Disable Ad Block Plus. You don't need to uninstall it.
7. Repeat steps 2-4
8. NO CRASH!

I bisected this regression to Nightly 22.0a1 (2013-03-31). Here is the pushlog for build 03-31:

https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=8693d1d4c86d&tochange=1932c6f78248
btw, I am running Mac OS X 10.8.3 on a Retina MacBook Pro.
(Reporter)

Comment 6

5 years ago
(In reply to Chris Peterson (:cpeterson) from comment #4)
> https://hg.mozilla.org/mozilla-central/
> pushloghtml?fromchange=8693d1d4c86d&tochange=1932c6f78248
I suspect bug 852489 in this range.
Keywords: reproducible

Comment 7

5 years ago
Over to roc given this. Can we see if a backout helps?
Assignee: nobody → roc
(Reporter)

Comment 8

5 years ago
After the fix of bug 855221, it's #1 browser crasher in 22.0b3 on Mac OS X.
Keywords: regressionwindow-wanted
(Reporter)

Updated

5 years ago
Crash Signature: [@ mozalloc_abort(char const*) | NS_DebugBreak ] [@ mozalloc_abort(char const*) | Abort | NS_DebugBreak | mozilla::layers::LayerManagerOGL::CreateFBOWithTexture(nsIntRect const&, mozilla::layers::LayerManagerOGL::InitMode, unsigned int, unsigned … → [@ mozalloc_abort(char const*) | NS_DebugBreak ] [@ mozalloc_abort(char const*) | Abort | NS_DebugBreak | mozilla::layers::LayerManagerOGL::CreateFBOWithTexture(nsIntRect const&, mozilla::layers::LayerManagerOGL::InitMode, unsigned int, unsigned …
Assignee: roc → matt.woodrow
(Assignee)

Comment 9

5 years ago
Created attachment 758355 [details]
Reduced test case

Somehow this page is occasionally triggering massive layers, and we're trying to allocate a framebuffer of that size.

Even though the size (16384) is within the max texture/renderbuffer size, we still fail to allocate it and abort.

I can only reproduce this with my discrete GPU (AMD Radeon HD 6770M), not with the integrated one. This my be related to why Adblock was required.
(Assignee)

Comment 10

5 years ago
Reducing the max texture size to 8192 fixes the issue, so we probably should do that for this card.
(Assignee)

Comment 11

5 years ago
Looking at the crash reports, there are multiple nvidia cards hitting this issue too, and some at 8192x8192.

I'm not sure if we should try create a 'blacklist' that lowers the max texture size for each of these cards, or possible detect at runtime that creating an FBO at the max texture size failed and dynamically lower it.

Bjacob, any thoughts here?
Flags: needinfo?(bjacob)
(Reporter)

Comment 12

5 years ago
(In reply to Matt Woodrow (:mattwoodrow) from comment #11)
> Looking at the crash reports, there are multiple nvidia cards hitting this
> issue too, and some at 8192x8192.
It seems indeed related to NVIDIA:
     91% (51/56) vs.  52% (166/318) GeForceGLDriver
9% of crashes with this signature are likely other bugs.
(Assignee)

Comment 13

5 years ago
I can reproduce it locally with my AMD card though, so we need to at least handle that as well.
(In reply to Matt Woodrow (:mattwoodrow) from comment #11)
> Looking at the crash reports, there are multiple nvidia cards hitting this
> issue too, and some at 8192x8192.
> 
> I'm not sure if we should try create a 'blacklist' that lowers the max
> texture size for each of these cards, or possible detect at runtime that
> creating an FBO at the max texture size failed and dynamically lower it.
> 
> Bjacob, any thoughts here?

The max texture size is partly an abstract concept: many desktop systems return 16384, but you really shouldn't try creating a 16384x16384x(32bpp) texture as that would take 2^(14+14+2) == 2^30 bytes == 1 GB of memory which would easily either fail, or cause the entire system to become unresponsive through a combination of evicting textures from GPU memory into main memory, and swapping out main-memory things into slow on-disk swap.

Taking a step back:
 - "maximum array size" concepts are rendered partly moot by the availability of slow virtual memory, because it's no longer a boolean success/failure situation, it's more like a continuous degradation with slower and slower behavior.
 - "GPU memory" is a fuzzy concept, as on many systems (Intel, mobiles...) it's just main memory and on other systems the driver silently swaps GPU memory into main memory; and "main memory" is virtualized itself with slow on-disk swap, so there too you have more of a continuum... the more memory you use, the higher the risk that it be slow memory, and you can't know how much 'fast' memory exactly you can have, so better not try to create maximum-size textures.

Here, I haven't looked at the specifics, but this bug sounds like we shouldn't abort on something that is as driver-dependent and runtime-context-dependent as whether a framebuffer is complete.

So I agree that we should react more gracefully to these incomplete framebuffers, by continuing with smaller textures.

Does this problem still exist in new-layers? If yes, we should fix it there first. Sounds like yes, as comment 1 mentions it on 23.0a2.
Flags: needinfo?(bjacob)
(In reply to Matt Woodrow (:mattwoodrow) from comment #10)
> Reducing the max texture size to 8192 fixes the issue, so we probably should
> do that for this card.

Can we get a patch up for Tuesday's b5 go to build?
(Assignee)

Comment 17

5 years ago
Created attachment 759581 [details] [diff] [review]
Don't crash if the framebuffer fails

This is the easy fix, and I think we should take this to begin with. This just stops us crashing, and skips attempting to render content that is ridiculously big.
Attachment #759581 - Flags: review?(bjacob)
(Assignee)

Comment 18

5 years ago
Created attachment 759584 [details] [diff] [review]
Use the proxy texture to check if allocation will succeed

WIP idea for the better fix (and one we probably don't want to uplift).

There's a few issues left to fix here.

One already exists, though this would make it worse, and that's if we hit CreateFBOWithTexture with InitModeCopy where the *source* is a downscaled fbo. That could cause all sorts of weirdness to happen, and we probably need to pass a 'isScaled' flag along with the framebuffer id to each RenderLayer call.

The other is that this general bug applies to normal texture uploads too, though it will just silently be sad rather than crashing.

I guess the best thing to do is add proxy checks to TexImage2D (which this doesn't call for some reason), and dynamically update the max texture size if it fails so that all future usage will be for the smaller size.
Attachment #759584 - Flags: feedback?(bjacob)
Comment on attachment 759581 [details] [diff] [review]
Don't crash if the framebuffer fails

Review of attachment 759581 [details] [diff] [review]:
-----------------------------------------------------------------

::: gfx/layers/opengl/ContainerLayerOGL.cpp
@@ +241,5 @@
> +                                          &containerSurface)) {
> +        aContainer->gl()->PopViewportRect();
> +        aContainer->gl()->PopScissorRect();
> +        aContainer->gl()->fBindFramebuffer(LOCAL_GL_FRAMEBUFFER, aPreviousFrameBuffer);
> +        return;

What is going to be the effect of this early return here? Will the user see a big black rectangle, or what?

::: gfx/layers/opengl/LayerManagerOGL.cpp
@@ +1277,5 @@
>      msg.AppendInt(aRect.height);
> +    NS_WARNING(msg.get());
> +
> +    mGLContext->fBindFramebuffer(LOCAL_GL_FRAMEBUFFER, 0);
> +    mGLContext->fDeleteFramebuffers(1, &fbo);

Aren't we leaking the texture, tex, here? Please also double check this function's code above for anything else we might be leaking.
Attachment #759581 - Flags: review?(bjacob) → review-
Comment on attachment 759584 [details] [diff] [review]
Use the proxy texture to check if allocation will succeed

Review of attachment 759584 [details] [diff] [review]:
-----------------------------------------------------------------

The basic idea is worth pursuing. At the same time, as you mention, it is scary to have to do this kind of work over and over again everytime we want to create a texture. It sounds sane to have a texImage2D wrapper that would keep track of failures and update the max texture size accordingly. The other part of the problem is how would we use it. A different API than texImage2D would be needed. CreateFBOWithTexture is such a different API, but we need something more generalist and in a 'shared' place in the code. I am afraid of suggesting adding this kind of thing to GLContext as we are talking of trimming GLContext to be more just a lightweight wrapper around OpenGL, but that is still a plausible thing to add to GLContext. The alternative is some shared place in gfx/layers or gfx/gl outside of GLContext, like maybe somewhere around TextureClient since IIUC this is the only place where new surfaces are requested in new-layers.

::: gfx/layers/opengl/LayerManagerOGL.cpp
@@ +1193,5 @@
>    mGLContext->fActiveTexture(LOCAL_GL_TEXTURE0);
>    mGLContext->fGenTextures(1, &tex);
>    mGLContext->fBindTexture(mFBOTextureTarget, tex);
>  
> +  aClamped = false;

Is aClamped a pointer to boolean, or a boolean?

@@ +1268,5 @@
> +                              LOCAL_GL_UNSIGNED_BYTE,
> +                              NULL);
> +      GLint format;
> +      mGLContext->fGetTexLevelParameteriv(proxy, 0, LOCAL_GL_TEXTURE_INTERNAL_FORMAT, &format);
> +      if (!format) {

Why do this dance? If glTexImage2D fails, it should give you a GL error, like GL_OUT_OF_MEMORY. If we don't trust that, then I wouldn't trust this either.
Attachment #759584 - Flags: feedback?(bjacob) → feedback+
(Assignee)

Comment 21

5 years ago
(In reply to Benoit Jacob [:bjacob] from comment #19)

> What is going to be the effect of this early return here? Will the user see
> a big black rectangle, or what?

I guess that is in theory possible, but that is very unlikely. This early return just stops us from drawing the subtree, and we should always have some sane content behind it that will be shown instead. The common case will be some crazy 3d transformed content (like my testcase) that will disappear and we'll just have the white page background.

Not ideal, but considerable better than crashing, and a safe patch to uplift.


> 
> Aren't we leaking the texture, tex, here? Please also double check this
> function's code above for anything else we might be leaking.

Yes, good catch.
(Assignee)

Comment 22

5 years ago
(In reply to Benoit Jacob [:bjacob] from comment #20)

> Is aClamped a pointer to boolean, or a boolean?

Clearly I didn't compile this :)

> 
> Why do this dance? If glTexImage2D fails, it should give you a GL error,
> like GL_OUT_OF_MEMORY. If we don't trust that, then I wouldn't trust this
> either.

GL_OUT_OF_MEMORY doesn't exist, nor does any error code for this situation. Checking the width/height/format is the recommended way to determine if a proxy texture succeeded.

I assume this is because GL only wants errors to occur in situations that are preventable. There's no way to know if a texture size is acceptable (apart from the max texture size), so users aren't able to prevent hitting this error.

The max texture size isn't really sufficient since it only specifies a single dimension. It means that the driver is in theory alright with a texture that has that as one of the dimensions. So |maxTextureSize| x 1 x 16bit might be ok, but |maxTextureSize| x |maxTextureSize| x 32bit might be way too big for graphics memory.

I believe this is what the proxy texture targets were added for, and a non-error way of determining their success.
(In reply to Matt Woodrow (:mattwoodrow) from comment #22)
> (In reply to Benoit Jacob [:bjacob] from comment #20)
> 
> > Is aClamped a pointer to boolean, or a boolean?
> 
> Clearly I didn't compile this :)
> 
> > 
> > Why do this dance? If glTexImage2D fails, it should give you a GL error,
> > like GL_OUT_OF_MEMORY. If we don't trust that, then I wouldn't trust this
> > either.
> 
> GL_OUT_OF_MEMORY doesn't exist, nor does any error code for this situation.
> Checking the width/height/format is the recommended way to determine if a
> proxy texture succeeded.

Sorry, I didn't understand this. Can you explain? GL_OUT_OF_MEMORY does exist, and it does get generated by texImage2D calls that fail because there was not enough memory for the requested operation. Maybe I am missing something as I am not sure what is meant by "proxy texture" ?
I'm getting this crash, while zooming 3 times on the map at http://www.phonehouse.nl/winkels/
This is on a Macbook Pro.
(Assignee)

Comment 26

5 years ago
> Sorry, I didn't understand this. Can you explain? GL_OUT_OF_MEMORY does
> exist, and it does get generated by texImage2D calls that fail because there
> was not enough memory for the requested operation. Maybe I am missing
> something as I am not sure what is meant by "proxy texture" ?

Hrm, so there is. It isn't listed as a possible error for glTexImage2D (http://www.opengl.org/sdk/docs/man/xhtml/glTexImage2D.xml) though, and no error of any sort is generated in our failure case.

I believe GL_OUT_OF_MEMORY only occurs during a true out-of-memory situation, whereas glTexImage2D can silently fail if the texture won't fit in vram.

My reference for the proxy texture is http://www.glprogramming.com/red/chapter09.html, see the heading 'Texture Proxy'.

It provides an alternate texture target to GL_TEXTURE_2D, GL_PROXY_TEXTURE_2D. You can use this target for a glTexImage2D call, and then query if it was successful. If it was, then you need to do the actual glTexImage2D call with GL_TEXTURE_2D as the target.

The suggested query to use is glGetTexLevelParameteriv to check the width, height or format.
(Assignee)

Comment 27

5 years ago
Created attachment 760271 [details] [diff] [review]
Don't crash if the framebuffer fails v2
Attachment #759581 - Attachment is obsolete: true
Attachment #760271 - Flags: review?(bjacob)
(In reply to Matt Woodrow (:mattwoodrow) from comment #26)
> > Sorry, I didn't understand this. Can you explain? GL_OUT_OF_MEMORY does
> > exist, and it does get generated by texImage2D calls that fail because there
> > was not enough memory for the requested operation. Maybe I am missing
> > something as I am not sure what is meant by "proxy texture" ?
> 
> Hrm, so there is. It isn't listed as a possible error for glTexImage2D
> (http://www.opengl.org/sdk/docs/man/xhtml/glTexImage2D.xml) though, and no
> error of any sort is generated in our failure case.
> 
> I believe GL_OUT_OF_MEMORY only occurs during a true out-of-memory
> situation, whereas glTexImage2D can silently fail if the texture won't fit
> in vram.

That's not true on any conformant GL. If no GL error is generated, then per spec the GL must ensure correct semantics in all respects for this texture with the image data specified by that texImage2D call.

I have definitely seen GL_OUT_OF_MEMORY generated by texImage2D for large sizes before; if this particular driver has a notion of "fitting in VRAM" such that it can't honor a texImage2D call that wouldn't fit in there, then it has no choice but to generate GL_OUT_OF_MEMORY there: anything else is non-conformant.

So it seems that we are dealing with a non-conformant GL implementation there (implied by above "glTexImage2D can silently fail if the texture won't fit in vram").

> 
> My reference for the proxy texture is
> http://www.glprogramming.com/red/chapter09.html, see the heading 'Texture
> Proxy'.
> 
> It provides an alternate texture target to GL_TEXTURE_2D,
> GL_PROXY_TEXTURE_2D. You can use this target for a glTexImage2D call, and
> then query if it was successful. If it was, then you need to do the actual
> glTexImage2D call with GL_TEXTURE_2D as the target.
> 
> The suggested query to use is glGetTexLevelParameteriv to check the width,
> height or format.

Thanks, that sounds interesting. However, this doesn't seem to be available in core OpenGL ES 2.0, so, depending on the existence and availability of any OES extension for that, that might or might not be usable everywhere we need it.
Comment on attachment 760271 [details] [diff] [review]
Don't crash if the framebuffer fails v2

Review of attachment 760271 [details] [diff] [review]:
-----------------------------------------------------------------

This patch seems correct & something we want, but to be honest: I am not familiar enough with the semantics around ContainerRender and its aPreviousFramebuffer argument to be certain that this will correctly just skip the rendering of this container.

Maybe Joe can provide some confirmation?
Attachment #760271 - Flags: review?(bjacob)
Attachment #760271 - Flags: review+
Attachment #760271 - Flags: feedback?(joe)
(Assignee)

Comment 30

5 years ago
Landing this without waiting for joe's feedback since this is a high priority to try get into beta.

I can back it out again later if he strongly disagrees.

https://hg.mozilla.org/integration/mozilla-inbound/rev/92b758cba0f1
Whiteboard: [leave open]
The other caveat with GL_OOM is that unlike other GL errors, the operation that triggers the GL_OOM is not guaranteed to be rolled back, or that it is harmless. In theory, the implementation can set up a texture's vars, init the data pointer to null, and only then attempt to alloc the memory. If GL_OOM hits at this point, that texture will appear completely valid, but have a data offset of null.

The final caveat is that GL_OOM can come at surprising times, if the driver is allocating lazily. You could conceivably have a driver for which you can create an OOM texture, bind it to an FB, clear it to a color, but only hit an OOM when you attempt to read or sample from the texture. It's not a nice thing to do, but it is theoretically possible. I believe we have seen some degree of lazy allocations in the past, though not quite that thorough.
(Assignee)

Comment 33

5 years ago
Comment on attachment 760271 [details] [diff] [review]
Don't crash if the framebuffer fails v2

[Approval Request Comment]
Bug caused by (feature/regressing bug #): Not a regression, I believe
User impact if declined: Crashes on some website
Testing completed (on m-c, etc.): Only just landed on m-c, tested locally and confirmed that it fixes the bug.
Risk to taking this patch (and alternatives if risky): Should be very low risk, just skips rendering if it's going to fail
String or IDL/UUID changes made by this patch: None
Attachment #760271 - Flags: approval-mozilla-beta?
Attachment #760271 - Flags: approval-mozilla-aurora?
(Reporter)

Updated

5 years ago
Crash Signature: [@ mozalloc_abort(char const*) | NS_DebugBreak ] [@ mozalloc_abort(char const*) | Abort | NS_DebugBreak | mozilla::layers::LayerManagerOGL::CreateFBOWithTexture(nsIntRect const&, mozilla::layers::LayerManagerOGL::InitMode, unsigned int, unsigned … → [@ mozalloc_abort(char const*) | NS_DebugBreak ] [@ mozalloc_abort(char const*) | Abort | NS_DebugBreak | mozilla::layers::LayerManagerOGL::CreateFBOWithTexture(nsIntRect const&, mozilla::layers::LayerManagerOGL::InitMode, unsigned int, unsigned …
Comment on attachment 760271 [details] [diff] [review]
Don't crash if the framebuffer fails v2

Review of attachment 760271 [details] [diff] [review]:
-----------------------------------------------------------------

This looks good.

::: gfx/layers/opengl/LayerManagerOGL.cpp
@@ +1274,5 @@
>      msg.Append(", aRect.width ");
>      msg.AppendInt(aRect.width);
>      msg.Append(", aRect.height ");
>      msg.AppendInt(aRect.height);
> +    NS_WARNING(msg.get());

Is there any purpose to us keeping this warning, I wonder?
Attachment #760271 - Flags: feedback?(joe) → feedback+
I agree that we should remove it now.
Comment on attachment 760271 [details] [diff] [review]
Don't crash if the framebuffer fails v2

Thanks for driving this through! Our Mac users will be very happy (hopefully ignorantly so, if we've fixed the topcrash)
Attachment #760271 - Flags: approval-mozilla-beta?
Attachment #760271 - Flags: approval-mozilla-beta+
Attachment #760271 - Flags: approval-mozilla-aurora?
Attachment #760271 - Flags: approval-mozilla-aurora+
(Assignee)

Comment 37

5 years ago
> Is there any purpose to us keeping this warning, I wonder?

I think there is. This is a fairly serious error, and it's caused us to give up trying to render the page correctly.


https://hg.mozilla.org/releases/mozilla-aurora/rev/98eab2d6e5d6
https://hg.mozilla.org/releases/mozilla-beta/rev/4cdd817a97b8
status-firefox22: affected → fixed
status-firefox23: affected → fixed
(Assignee)

Comment 38

5 years ago
Created attachment 761281 [details] [diff] [review]
Use the proxy texture to check if allocation will succeed v2

I only added calling this for create FBO's (and not normal texture creation), since it wasn't obvious what to do in the failure case.

Calling glTexImage2D even if it fails seems really weird, but not calling it is a change in behaviour (and might result in new glErrors being generated).
Attachment #759584 - Attachment is obsolete: true
Attachment #761281 - Flags: review?(jgilbert)
Verified fixed with Firefox 22 beta 5 (build ID: 20130612084701) on Mac OSX 10.8.3, using the testcase from comment 9. (I wasn't able to reproduce the crash using the STR from comment 4)

There aren't any reports regarding the second signature of this bug, in Socorro, from last month.

Reports from the last month, regarding the other 5 signatures are available here: 

https://crash-stats.mozilla.com/report/list?product=Firefox&query_search=signature&query_type=contains&query=mozalloc_abort%28char%20const*%29%20|%20NS_DebugBreak&reason_type=contains&date=06%2F13%2F2013%2007%3A42%3A13&range_value=4&range_unit=weeks&hang_type=any&process_type=any&do_query=1&signature=mozalloc_abort%28char%20const*%29%20|%20NS_DebugBreak

https://crash-stats.mozilla.com/report/list?product=Firefox&query_search=signature&query_type=contains&query=mozalloc_abort%28char%20const*%29%20|%20NS_DebugBreak%20|%20HexEncode%28void%20const*%2C%20unsigned%20long%29%3A%3AkHexChars&reason_type=contains&date=06%2F13%2F2013%2007%3A42%3A14&range_value=4&range_unit=weeks&hang_type=any&process_type=any&do_query=1&signature=mozalloc_abort%28char%20const*%29%20|%20NS_DebugBreak%20|%20HexEncode%28void%20const*%2C%20unsigned%20long%29%3A%3AkHexChars

https://crash-stats.mozilla.com/report/list?product=Firefox&query_search=signature&query_type=contains&query=mozalloc_abort%28char%20const*%29%20|%20ExceptionHandling%400x122b&reason_type=contains&date=06%2F13%2F2013%2007%3A42%3A14&range_value=4&range_unit=weeks&hang_type=any&process_type=any&do_query=1&signature=mozalloc_abort%28char%20const*%29%20|%20ExceptionHandling%400x122b

https://crash-stats.mozilla.com/report/list?product=Firefox&query_search=signature&query_type=contains&query=mozalloc_abort%28char%20const*%29%20|%20double_conversion%3A%3ABignumDtoa%28double%2C%20double_conversion%3A%3ABignumDtoaMode%2C%20int%2C%20double_conversion%3A%3AVector%26lt%3Bchar%26gt%3B%2C%20int*%2C%20int*%29&reason_type=contains&date=06%2F13%2F2013%2007%3A42%3A14&range_value=4&range_unit=weeks&hang_type=any&process_type=any&do_query=1&signature=mozalloc_abort%28char%20const*%29%20|%20double_conversion%3A%3ABignumDtoa%28double%2C%20double_conversion%3A%3ABignumDtoaMode%2C%20int%2C%20double_conversion%3A%3AVector%3Cchar%3E%2C%20int*%2C%20int*%29

https://crash-stats.mozilla.com/report/list?product=Firefox&query_search=signature&query_type=contains&query=mozalloc_abort%28char%20const*%29%20|%20NS_DebugBreak%20|%20GeForceGLDriver%400x1625a8&reason_type=contains&date=06%2F13%2F2013%2007%3A44%3A45&range_value=4&range_unit=weeks&hang_type=any&process_type=any&do_query=1&signature=mozalloc_abort%28char%20const*%29%20|%20NS_DebugBreak%20|%20GeForceGLDriver%400x1625a8
status-firefox22: fixed → verified
QA Contact: manuela.muntean
(Reporter)

Updated

5 years ago
status-firefox24: affected → fixed
(Reporter)

Updated

5 years ago
Duplicate of this bug: 886963

Updated

5 years ago
status-firefox25: --- → affected
(Reporter)

Comment 41

5 years ago
(Ioana Chiorean wrote in bug 886963 comment #0)
> Steps to reproduce:
> 1. Go to Foursquare - log in 
> 2. Go to history  - left panel search
> 3. Add a term in first field - enter
Keywords: verifyme

Comment 42

5 years ago
There are no post-fix crashes in Socorro with any of the signatures associated to this bug:
https://crash-stats.mozilla.com/report/list?product=Firefox&query_search=signature&query_type=contains&query=mozalloc_abort%28char%20const*%29%20|%20NS_DebugBreak&reason_type=contains&date=06%2F13%2F2013%2007%3A42%3A13&range_value=4&range_unit=weeks&hang_type=any&process_type=any&do_query=1&signature=mozalloc_abort%28char%20const*%29%20|%20NS_DebugBreak
https://crash-stats.mozilla.com/report/list?product=Firefox&query_search=signature&query_type=contains&query=mozalloc_abort%28char%20const*%29%20|%20NS_DebugBreak%20|%20HexEncode%28void%20const*%2C%20unsigned%20long%29%3A%3AkHexChars&reason_type=contains&date=06%2F13%2F2013%2007%3A42%3A14&range_value=4&range_unit=weeks&hang_type=any&process_type=any&do_query=1&signature=mozalloc_abort%28char%20const*%29%20|%20NS_DebugBreak%20|%20HexEncode%28void%20const*%2C%20unsigned%20long%29%3A%3AkHexChars
https://crash-stats.mozilla.com/report/list?product=Firefox&query_search=signature&query_type=contains&query=mozalloc_abort%28char%20const*%29%20|%20NS_DebugBreak%20|%20GeForceGLDriver%400x1625a8&reason_type=contains&date=06%2F13%2F2013%2007%3A44%3A45&range_value=4&range_unit=weeks&hang_type=any&process_type=any&do_query=1&signature=mozalloc_abort%28char%20const*%29%20|%20NS_DebugBreak%20|%20GeForceGLDriver%400x1625a8
https://crash-stats.mozilla.com/query/query?product=Firefox&version=ALL%3AALL&range_value=4&range_unit=weeks&date=06%2F26%2F2013+13%3A00%3A23&query_search=signature&query_type=contains&query=mozalloc_abort%28char+const*%29+%7C+Abort+%7C+NS_DebugBreak+%7C+mozilla%3A%3Alayers%3A%3ALayerManagerOGL%3A%3ACreateFBOWithTexture%28nsIntRect+const%26%2C+mozilla%3A%3Alayers%3A%3ALayerManagerOGL%3A%3AInitMode%2C+unsigned+int%2C+unsigned+int*%2C+unsigned+int*%29&reason=&build_id=&process_type=any&hang_type=any&do_query=1
https://crash-stats.mozilla.com/report/list?product=Firefox&query_search=signature&query_type=contains&query=mozalloc_abort%28char%20const*%29%20|%20ExceptionHandling%400x122b&reason_type=contains&date=06%2F13%2F2013%2007%3A42%3A14&range_value=4&range_unit=weeks&hang_type=any&process_type=any&do_query=1&signature=mozalloc_abort%28char%20const*%29%20|%20ExceptionHandling%400x122b
https://crash-stats.mozilla.com/report/list?product=Firefox&query_search=signature&query_type=contains&query=mozalloc_abort%28char%20const*%29%20|%20double_conversion%3A%3ABignumDtoa%28double%2C%20double_conversion%3A%3ABignumDtoaMode%2C%20int%2C%20double_conversion%3A%3AVector%26lt%3Bchar%26gt%3B%2C%20int*%2C%20int*%29&reason_type=contains&date=06%2F13%2F2013%2007%3A42%3A14&range_value=4&range_unit=weeks&hang_type=any&process_type=any&do_query=1&signature=mozalloc_abort%28char%20const*%29%20|%20double_conversion%3A%3ABignumDtoa%28double%2C%20double_conversion%3A%3ABignumDtoaMode%2C%20int%2C%20double_conversion%3A%3AVector%3Cchar%3E%2C%20int*%2C%20int*%29
status-firefox23: fixed → verified
status-firefox24: fixed → verified
(Reporter)

Comment 43

5 years ago
(In reply to Ioana Budnar, QA [:ioana] from comment #42)
> There are no post-fix crashes in Socorro with any of the signatures
> associated to this bug:
I disagree for 24.0 and above. See bp-4b8bb855-95e9-445f-bbaf-aa4e72130624, bp-2a3588c2-64b4-4bb7-9b80-133552130625, bp-0efe9a8b-62fc-489a-8b7b-2cd292130625 and the STR in comment 41.
status-firefox24: verified → affected
(Reporter)

Updated

5 years ago
Keywords: topcrash, verifyme

Comment 44

5 years ago
(In reply to Scoobidiver from comment #43)
> I disagree for 24.0 and above. See bp-4b8bb855-95e9-445f-bbaf-aa4e72130624,
> bp-2a3588c2-64b4-4bb7-9b80-133552130625,
> bp-0efe9a8b-62fc-489a-8b7b-2cd292130625 and the STR in comment 41.

These are indeed post-fix and they have the second signature, but somehow they don't show up in the query for this signature. How did you find them?
(Reporter)

Comment 45

5 years ago
(In reply to Ioana Budnar, QA [:ioana] from comment #44)
> but somehow they don't show up in the query for this signature.
All your queries in comment 42 have a cutoff date set to June 13 so any crashes after this date won't show up. The best syntax for such queries is https://crash-stats.mozilla.com/report/list?signature=signature-name then extended to four weeks.

Comment 46

5 years ago
Thanks Scoobidiver! The updated queries show some post-fix crashes for the first two signatures:
https://crash-stats.mozilla.com/report/list?query_search=signature&query_type=contains&reason_type=contains&range_value=4&range_unit=weeks&hang_type=any&process_type=any&signature=mozalloc_abort%28char%20const%2A%29%20%7C%20NS_DebugBreak
https://crash-stats.mozilla.com/report/list?query_search=signature&query_type=contains&reason_type=contains&range_value=4&range_unit=weeks&hang_type=any&process_type=any&signature=mozalloc_abort%28char%20const%2A%29%20%7C%20NS_DebugBreak%20%7C%20HexEncode%28void%20const%2A%2C%20unsigned%20long%29%3A%3AkHexChars
https://crash-stats.mozilla.com/report/list?query_search=signature&query_type=contains&reason_type=contains&range_value=4&range_unit=weeks&hang_type=any&process_type=any&signature=mozalloc_abort%28char%20const%2A%29%20%7C%20NS_DebugBreak%20%7C%20GeForceGLDriver%400x1625a8
https://crash-stats.mozilla.com/report/list?query_search=signature&query_type=contains&reason_type=contains&range_value=4&range_unit=weeks&hang_type=any&process_type=any&signature=mozalloc_abort%28char+const*%29+%7C+Abort+%7C+NS_DebugBreak+%7C+mozilla%3A%3Alayers%3A%3ALayerManagerOGL%3A%3ACreateFBOWithTexture%28nsIntRect+const%26%2C+mozilla%3A%3Alayers%3A%3ALayerManagerOGL%3A%3AInitMode%2C+unsigned+int%2C+unsigned+int*%2C+unsigned+int*%29&reason=&build_id=&process_type=any&hang_type=any&do_query=1
https://crash-stats.mozilla.com/report/list?query_search=signature&query_type=contains&reason_type=contains&range_value=4&range_unit=weeks&hang_type=any&process_type=any&signature=mozalloc_abort%28char%20const%2A%29%20%7C%20double_conversion%3A%3ABignumDtoa%28double%2C%20double_conversion%3A%3ABignumDtoaMode%2C%20int%2C%20double_conversion%3A%3AVector%3Cchar%3E%2C%20int%2A%2C%20int%2A%29
https://crash-stats.mozilla.com/report/list?query_search=signature&query_type=contains&reason_type=contains&range_value=4&range_unit=weeks&hang_type=any&process_type=any&signature=mozalloc_abort%28char%20const%2A%29%20%7C%20ExceptionHandling%400x122b
Given there are post-fix crashes for some of the signatures I'm not sure we can call this "verified" for Firefox 23.
(Reporter)

Comment 48

5 years ago
(In reply to Anthony Hughes, Mozilla QA (:ashughes) from comment #47)
> Given there are post-fix crashes for some of the signatures I'm not sure we
> can call this "verified" for Firefox 23.
The topcrash part is verified and the matching keyword was removed accordingly.
Comment on attachment 761281 [details] [diff] [review]
Use the proxy texture to check if allocation will succeed v2

Review of attachment 761281 [details] [diff] [review]:
-----------------------------------------------------------------

Man, this review really slipped through the cracks, sorry!

::: gfx/gl/GLContext.cpp
@@ +2130,5 @@
> +                          GLsizei width, GLsizei height,
> +                          GLint border, GLenum format,
> +                          GLenum type)
> +{
> +    // If we can't do a proxy test, then we have to assume it will

You can actually try to do the allocation instead, and check for GL_OOM and INVALID_VALUE. (The spec seems to indicate that even if width and height are < MAX_TEX_SIZE, TexImage calls can generate INVALID_VALUE if it knows that it can't allocate something of that size, for example for a RGBA32f texture) Don't skip this check unless we have a plan for handling the errors it might result in. (In which case, there's not too much point in doing this check except as an optimization, I suppose)

@@ +2134,5 @@
> +    // If we can't do a proxy test, then we have to assume it will
> +    // work.
> +    if (mIsGLES2 ||
> +        (target != LOCAL_GL_TEXTURE_2D &&
> +         target != LOCAL_GL_TEXTURE_RECTANGLE)) {

Assert that we have a target we can handle, don't early-exit.

@@ +2153,5 @@
> +   
> +    // Failed to allocate a texture of this size. Halve the maximum texture size
> +    // and return false.
> +    
> +    mMaxTextureSize /= 2;

This isn't the right way to handle this, since a 8192x1024 allocation may still work even if our biggest square caps out at 4096.

I recommend keeping track of this separately in the layer manager, and halving the desired size until you get a size that works. Keep track of a MaxSquareTexSize, since that meaning differs from what MaxTexSize means.

Additionally, the max size for a TEXTURE_RECTANGLE is actually MAX_RECTANGLE_TEXTURE_SIZE.

::: gfx/layers/opengl/ContainerLayerOGL.cpp
@@ +236,5 @@
>          aContainer->gl()->fBindFramebuffer(LOCAL_GL_FRAMEBUFFER, aPreviousFrameBuffer);
>          return;
>        }
> +      if (clampedSize) {
> +        aContainer->mSupportsComponentAlphaChildren = false;

This is not something I can review, unless it is explained to me.

::: gfx/layers/opengl/LayerManagerOGL.cpp
@@ +1258,5 @@
> +                                    LOCAL_GL_RGBA,
> +                                    aRect.width, aRect.height,
> +                                    0, LOCAL_GL_RGBA,
> +                                    LOCAL_GL_UNSIGNED_BYTE)) {
> +      // Texture was too big, max texture size will have been updated to reflect this.

See the comment about how this should all be done in here, not in GLContext. Also, you may have to iterate more than once, particularly for square textures with MaxTexSizes starting way up at 16k.

::: gfx/layers/opengl/LayerManagerOGL.h
@@ +216,3 @@
>                              GLuint aCurrentFrameBuffer,
> +                            GLuint *aFBO, GLuint *aTexture,
> +                            bool *aClamped);

Consider `aWasClamped`.
Attachment #761281 - Flags: review?(jgilbert) → review-

Comment 50

4 years ago
(In reply to Jeff Gilbert [:jgilbert] from comment #49)
> Comment on attachment 761281 [details] [diff] [review]
> Use the proxy texture to check if allocation will succeed v2
> 
> Review of attachment 761281 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Man, this review really slipped through the cracks, sorry!
> ...
Flags: needinfo?(matt.woodrow)
(Assignee)

Updated

3 years ago
Flags: needinfo?(matt.woodrow)
Matt, can we close this out at this point?
Flags: needinfo?(matt.woodrow)
(Assignee)

Comment 52

2 years ago
Yes, this code no longer exists.
Status: NEW → RESOLVED
Last Resolved: 2 years ago
Flags: needinfo?(matt.woodrow)
Resolution: --- → INVALID
Keywords: regressionwindow-wanted
Whiteboard: [leave open]
You need to log in before you can comment on or make changes to this bug.