Closed Bug 867226 Opened 12 years ago Closed 9 years ago

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

Categories

(Core :: Graphics: Layers, defect)

Product:
Core
Component:
Graphics: Layers
Version:
22 Branch
Platform:
All
macOS
Type:
defect
Priority:
Not set
Severity:
critical

Tracking

()

Status:
RESOLVED INVALID
Tracking Flags:
Tracking Status
firefox21 --- unaffected
firefox22 + verified
firefox23 + verified
firefox24 --- affected
firefox25 --- affected

People

(Reporter: scoobidiver, Assigned: mattwoodrow)

References

(
URL
)

Details

(Keywords: crash, regression, reproducible)

Crash Data

Attachments

(3 files, 2 obsolete files)

Reduced test case
480 bytes, text/html
Details
Don't crash if the framebuffer fails v2
4.74 KB, patch
bjacob
: review+
joe
: feedback+
akeybl
: approval-mozilla-aurora+
akeybl
: approval-mozilla-beta+
Details | Diff | Splinter Review
Use the proxy texture to check if allocation will succeed v2
11.03 KB, patch
jgilbert
: review-
Details | Diff | Splinter Review
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
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
URL: http://www.theatlanticcities.com/tech...
btw, I am running Mac OS X 10.8.3 on a Retina MacBook Pro.
(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
Over to roc given this. Can we see if a backout helps?
Assignee: nobody → roc
After the fix of bug 855221, it's #1 browser crasher in 22.0b3 on Mac OS X.
Keywords: regressionwindow-wanted
Crash Signature: , unsigned int*)] [@ mozalloc_abort(char const*) | NS_DebugBreak | HexEncode(void const*, unsigned long)::kHexChars ] → , unsigned int*)] [@ mozalloc_abort(char const*) | NS_DebugBreak | HexEncode(void const*, unsigned long)::kHexChars ] [@ mozalloc_abort(char const*) | ExceptionHandling@0x122b ] [@ mozalloc_abort(char const*) | double_conversion::BignumDtoa(double, dou…
Assignee: roc → matt.woodrow
Attached file 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.
Reducing the max texture size to 8192 fixes the issue, so we probably should do that for this card.
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)
(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.
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?
Attached patch Don't crash if the framebuffer fails (obsolete) — — Splinter Review
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)
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+
(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.
(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.
> 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.
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)
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.
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?
Crash Signature: , double_conversion::BignumDtoaMode, int, double_conversion::Vector<char>, int*, int*) ] → , double_conversion::BignumDtoaMode, int, double_conversion::Vector<char>, int*, int*) ] [@ mozalloc_abort(char const*) | NS_DebugBreak | GeForceGLDriver@0x1625a8 ]
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+
> 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
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
(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
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
(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
Keywords: topcrash, verifyme
(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?
(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.
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.
(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-
(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)
Flags: needinfo?(matt.woodrow)
Matt, can we close this out at this point?
Flags: needinfo?(matt.woodrow)
Yes, this code no longer exists.
Status: NEW → RESOLVED
Closed: 9 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.

Attachment

General

Creator:
Created:
Updated:
Size: