Closed Bug 1224254 Opened 9 years ago Closed 9 years ago

Have a notion of "unreasonable" texture size

Categories

(Core :: Graphics: Layers, defect)

Product:
Core
Component:
Graphics: Layers
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla45
Tracking Flags:
Tracking Status
firefox43 --- fixed
firefox44 --- fixed
firefox45 --- fixed

People

(Reporter: nical, Assigned: nical)

References

Details

Attachments

(1 file, 1 obsolete file)

This version is less opinionated: size limit bumped to 32767 per dimension, and "reasonable" stays a separate thing from "allowed"
16.74 KB, patch
bas.schouten
: review+
lizzard
: approval-mozilla-aurora+
lizzard
: approval-mozilla-beta+
Details | Diff | Splinter Review 
A web page can easily cause us to try to allocate huge amounts of memory in just one big texture. In most cases it will fail, in some case it could trigger unexpected bugs, or even succeed and bring the browser to its knees because it doesn't have enough memory left to do other (more legit) things.

We already have a notion of maximum gpu texture size, but we don't apply it to things like ShmemTextureClient, which will tile their content into smaller gpu texture once used by the compositor, but will still try to allocate very big shmems.

Applying the max gpu texture size to BufferTextureClient is too restrictive with some gpus, but there's gotta be a compromise there. With some drawing backends like d2d we are already restricted to the max gpu texture size anyway.

How about 8192 in each dimension? or twice as much if we want to be conservative ? I we allocate anything bigger than that, there's probably a smarter decision that could have been made at the layout level, or the page is trying hard to break the browser.

I feel like we already had a similar discussion a while ago but I am not sure.
8192 in each dimension is probably too small.  Total amount of memory beyond 256mb may be OK.  So, 8192x8192x4 will just fit, but so would 16k x 4k and 32k x 2k.  Stopping a 8193 x 2 from getting allocated is likely too restrictive.
How about 16k max per dimension 512mo max total ? Having a layer near that limit is completely unreasonable (especially since most layers tend to allocate several textures for the back buffer, swap chain, etc.) and will most likely fail further down the rendering pipeline, but we could start there conservatively and tighten the limit if problems show up.
Excellent.  Now, make it a (not in .js so, slightly "hidden") preference, and we should be good to go and somewhat ready for special cases out there.
gfx::Factory::CheckSurfaceSize is already in place and properly checks overflow, so let's use it.
I added a gfx::Config struct and Factory::Init(Config) since the multi-threaded tiling stuff will add more parameters in there.
Assignee: nobody → nical.bugzilla
Attachment #8687924 - Flags: review?(bas)
Comment on attachment 8687924 [details] [diff] [review]
add prefs and use gfx::Factory::ReasonableSurfaceSize when allocating textures

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

The general idea of this patch isn't bad, but I have serious doubts about the limits chosen.

::: gfx/thebes/gfxPrefs.h
@@ +246,5 @@
>    // Note that        "gfx.logging.level" is defined in Logging.h
>    DECL_GFX_PREF(Once, "gfx.logging.crash.length",              GfxLoggingCrashLength, uint32_t, 6);
> +  // The maximums here are quite conservative, we can tighten them if problems show up.
> +  DECL_GFX_PREF(Once, "gfx.max-alloc-size",                    MaxAllocSize, int32_t, (int32_t)512000000);
> +  DECL_GFX_PREF(Once, "gfx.max-texture-size",                  MaxTextureSize, int32_t, (int32_t)16384);

For a maximum GPU texture size this is fine, for a maximum surface size this definitely is not, there's plenty of websites doing 20000x64 pixel texture maps and such, we'll have to support those. There's even a reftest in our tree which creates a canvas bigger than this I believe. I think with 30 0000 we might be okay but we'll need to do some serious testing. Also, use 536870912 or 500000000. This looks just silly :-).
Attachment #8687924 - Flags: review?(bas) → review+
Comment on attachment 8687924 [details] [diff] [review]
add prefs and use gfx::Factory::ReasonableSurfaceSize when allocating textures

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

Meant to r- this, whoops.
Attachment #8687924 - Flags: review+ → review-
(In reply to Bas Schouten (:bas.schouten) from comment #5)
> For a maximum GPU texture size this is fine, for a maximum surface size this
> definitely is not, there's plenty of websites doing 20000x64 pixel texture
> maps and such, we'll have to support those.

We don't support drawing into surfaces that are this big with D2D. I am not found of having pages that render differently on a mac and on a windows with good drivers.
We definitely should not be allocating layers of that kind of size (it will fail most of the time in different places further down the rendering pipeline, so I'd rather catch it up front and not suffer the memory pressure and the weird states that we get in when that happens).
If we allow wrapping SourceSurfaces around images that are bigger than the limit, it would satisfy this use case, right?

> There's even a reftest in our
> tree which creates a canvas bigger than this I believe. I think with 30 0000
> we might be okay but we'll need to do some serious testing.

This reftest intermittently fails often enough that it has become more annoying than useful. There's a bug open to make it use another size but I don't have the bug number handy.

The current maximum canvas size is 32767x32767. We can't support bigger surfaces with xlib because the latter uses 16 bits to store surface sizes in some places. We could bump the maximum texture size up to 32767 for all surfaces (separating what is considered reasonable from the hard limit for error reporting), and keep the maximum alloc below 500Mo. That would not help with the crashes as much, but at least we'd avoid some potential security issues. Sounds good to you?
Comment on attachment 8687924 [details] [diff] [review]
add prefs and use gfx::Factory::ReasonableSurfaceSize when allocating textures

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

::: dom/canvas/CanvasRenderingContext2D.cpp
@@ +1380,5 @@
>    mIsSkiaGL = false;
>  
>     // Check that the dimensions are sane
>    IntSize size(mWidth, mHeight);
> +  if (gfx::Factory::ReasonableSurfaceSize(size) || (size == IntSize(0, 0))) {

This is changing the logic, are you sure you want that?  Before, 128x0 would go through; after the change, 128x0 does not go through.

::: gfx/thebes/gfxPlatform.cpp
@@ +514,5 @@
> +    cfg.mLogForwarder = fwd;
> +    cfg.mMaxTextureSize = gfxPrefs::MaxTextureSize();
> +    cfg.mMaxAllocSize = gfxPrefs::MaxAllocSize();
> +
> +    gfx::Factory::Init(cfg);

Why not just leave gfxPrefs::MaxTextureSize in the Factory method itself, instead of initializing a whole structure for it?

::: gfx/thebes/gfxPrefs.h
@@ -224,5 @@
>    DECL_GFX_PREF(Live, "gfx.canvas.auto_accelerate.min_frames", CanvasAutoAccelerateMinFrames, int32_t, 30);
>    DECL_GFX_PREF(Live, "gfx.canvas.auto_accelerate.min_seconds", CanvasAutoAccelerateMinSeconds, float, 5.0f);
>    DECL_GFX_PREF(Live, "gfx.canvas.azure.accelerated",          CanvasAzureAccelerated, bool, false);
>    // 0x7fff is the maximum supported xlib surface size and is more than enough for canvases.
> -  DECL_GFX_PREF(Live, "gfx.canvas.max-size",                   MaxCanvasSize, int32_t, 0x7fff);

Why not have separate maximum sizes for canvas vs. other random textures?  Canvas can then be limited to the smaller of the two.
(In reply to Milan Sreckovic [:milan] from comment #8)
> Why not just leave gfxPrefs::MaxTextureSize in the Factory method itself,
> instead of initializing a whole structure for it?

We can't call into prefs from Factory.cpp and since I have another patch queue that will add more initialization parameters, I felt like having a struct was a tad cleaner although it doesn't really matter since there is only one call site. I don't have a strong preference.

> Why not have separate maximum sizes for canvas vs. other random textures? 
> Canvas can then be limited to the smaller of the two.

I can leave the two as separate things but they mostly exist for the same reason so one could ask why keep them both? Same here, it doesn't matter much to me.
Comment on attachment 8688462 [details] [diff] [review]
This version is less opinionated: size limit bumped to 32767 per dimension, and "reasonable" stays a separate thing from "allowed"

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

Let's see how much of the web breaks :-).
Attachment #8688462 - Flags: review?(bas) → review+
Comment on attachment 8688462 [details] [diff] [review]
This version is less opinionated: size limit bumped to 32767 per dimension, and "reasonable" stays a separate thing from "allowed"

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

::: gfx/layers/ImageDataSerializer.cpp
@@ +92,5 @@
>    }
>  
> +  int32_t bufsize = GetAlignedStride<16>(ComputeStride(aFormat, aSize.width)
> +                                         * aSize.height)
> +                  + SurfaceBufferInfo::GetOffset();

Did we lose the overflow checking here?

::: gfx/thebes/gfxPlatform.cpp
@@ +512,5 @@
> +
> +    mozilla::gfx::Config cfg;
> +    cfg.mLogForwarder = fwd;
> +    cfg.mMaxTextureSize = gfxPrefs::MaxTextureSize();
> +    cfg.mMaxAllocSize = gfxPrefs::MaxAllocSize();

These are user settable prefs, and a bad user can set some nasty values in here.  For example, negative, or so large that we overflow in the place where we dropped the CheckedInt?  Probably not, but it'd be nicer if we did some checks?

@@ +706,5 @@
>      delete mozilla::gfx::Factory::GetLogForwarder();
>      mozilla::gfx::Factory::SetLogForwarder(nullptr);
>  
> +    gfx::Factory::ShutDown();
> +

Since this method doesn't delete the log forwarder, I think we're safe.  If the Config destructor was actually deleting the log forwarder, we would lose the crash annotations between this point and the end of the shutdown, which could be bad.
(In reply to Milan Sreckovic [:milan] from comment #12)
> Comment on attachment 8688462 [details] [diff] [review]
> This version is less opinionated: size limit bumped to 32767 per dimension,
> and "reasonable" stays a separate thing from "allowed"
> 
> Review of attachment 8688462 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: gfx/layers/ImageDataSerializer.cpp
> @@ +92,5 @@
> >    }
> >  
> > +  int32_t bufsize = GetAlignedStride<16>(ComputeStride(aFormat, aSize.width)
> > +                                         * aSize.height)
> > +                  + SurfaceBufferInfo::GetOffset();
> 
> Did we lose the overflow checking here?

All possible overflow situations are caught by AllowedSurfaceSize.

> 

> These are user settable prefs, and a bad user can set some nasty values in
> here.  For example, negative, or so large that we overflow in the place
> where we dropped the CheckedInt?  Probably not, but it'd be nicer if we did
> some checks?

Good point, I updated the patch with minimums (2048 per dimension and 10000000 bytes total). With this you can always at least render something and fix your prefs back (although you may need to resize your window a bit if your screen is very big).

> 
> @@ +706,5 @@
> >      delete mozilla::gfx::Factory::GetLogForwarder();
> >      mozilla::gfx::Factory::SetLogForwarder(nullptr);
> >  
> > +    gfx::Factory::ShutDown();
> > +
> 
> Since this method doesn't delete the log forwarder, I think we're safe.  If
> the Config destructor was actually deleting the log forwarder, we would lose
> the crash annotations between this point and the end of the shutdown, which
> could be bad.
Comment on attachment 8688462 [details] [diff] [review]
This version is less opinionated: size limit bumped to 32767 per dimension, and "reasonable" stays a separate thing from "allowed"

Approval Request Comment
[Feature/regressing bug #]:
[User impact if declined]: OOM and bugs caused by unreasonable allocation sizes.
[Describe test coverage new/current, TreeHerder]:
[Risks and why]: low risk. Some bad web pages that do things that work 10% of the time and cause OOM will work 0% of the time and cause less OOM.
[String/UUID change made/needed]:
Attachment #8688462 - Flags: approval-mozilla-beta?
Attachment #8688462 - Flags: approval-mozilla-aurora?
https://hg.mozilla.org/mozilla-central/rev/29f37fd4c13e
https://hg.mozilla.org/mozilla-central/rev/4a9ebc74d62a
Status: NEW → RESOLVED
Closed: 9 years ago
status-firefox45: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla45
Comment on attachment 8688462 [details] [diff] [review]
This version is less opinionated: size limit bumped to 32767 per dimension, and "reasonable" stays a separate thing from "allowed"

Let there be reasonableness in texture sizes. 
Avoiding some crashes sounds good, OK to uplift to aurora and beta.
Attachment #8688462 - Flags: approval-mozilla-beta?
Attachment #8688462 - Flags: approval-mozilla-beta+
Attachment #8688462 - Flags: approval-mozilla-aurora?
Attachment #8688462 - Flags: approval-mozilla-aurora+
Aren't we worried about webcompat regressions with this? Uplifting directly to beta seems a bit risky.
Comment 15 seems persuasive since I would rather some pages not work than have them crash Firefox. We have such a high amount of OOM crashes in general that it sounds good to bring the crash rate down.

Depending on which pages break though. nical do you have some examples of stuff that crashes or doesn't work consistently?

My thinking was that uplifting this earlier in beta might be preferable to trying to uplift next week or the week after.  The next go to build is Monday Nov. 23 so we don't need to rush to uplift. 

We could let this ride with 45 or 44 instead of trying to stuff it into beta.
Flags: needinfo?(nical.bugzilla)
(In reply to Matt Woodrow (:mattwoodrow) from comment #19)
> Aren't we worried about webcompat regressions with this? Uplifting directly
> to beta seems a bit risky.

I am not worried at all. If you try to render stuff that approaches this limit on windows with good drivers (D2D drawing backend), it will fail with much smaller sizes already without the patch. If you happen to have bad drivers, the cairo backend will probably work if you are lucky enough to have a ton of contiguous memory available, but it won't work either on linux, because the limit I set here corresponds to the maximum xlib surface size, and I think CG's limit is just a tad higher.
So in the end, very few users have a configuration where it would have a chance to work, and when they do, it is unlikely that it would succeed anyway.

I would like to set much stricter limits which are more debatable on the web compat side of things but I haven't managed to convince the rest of the team yet :)
Flags: needinfo?(nical.bugzilla)
hi, this failed to apply to beta:

Tomcats-MacBook-Pro-2:mozilla-central Tomcat$ hg graft --edit -r c642a03ec545
grafting 315898:c642a03ec545 "Bug 1224254 - Don't try to allocate unreasonably large textures. r=Bas, a=lizzard"
merging gfx/2d/2D.h
merging gfx/2d/Factory.cpp
merging gfx/layers/ImageDataSerializer.cpp
merging gfx/layers/YCbCrImageDataSerializer.cpp
merging gfx/layers/client/TextureClient.cpp
warning: conflicts during merge.
merging gfx/layers/client/TextureClient.cpp incomplete! (edit conflicts, then use 'hg resolve --mark')
merging gfx/thebes/gfxPlatform.cpp
merging gfx/thebes/gfxPrefs.h
abort: unresolved conflicts, can't continue
(use hg resolve and hg graft --continue)
Flags: needinfo?(nical.bugzilla)
https://hg.mozilla.org/releases/mozilla-beta/rev/9d0956a9b193 (folded the 2 patches)
Flags: needinfo?(nical.bugzilla)
(In reply to Nicolas Silva [:nical] from comment #24)
> https://hg.mozilla.org/releases/mozilla-beta/rev/9d0956a9b193 (folded the 2
> patches)

setting the flags :)
status-firefox43: --- → fixed
Depends on: 1235796
Depends on: 1274991
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: