Deal with windows that are resized to unreasonably large sizes

RESOLVED FIXED in Firefox 46

Status

()

defect
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: bas.schouten, Assigned: bas.schouten)

Tracking

unspecified
mozilla46
Points:
---

Firefox Tracking Flags

(firefox46 fixed)

Details

(Whiteboard: [gfx-noted])

Attachments

(1 attachment)

In practice some situations can cause widgets to be resized to unreasonably large sizes. (Larger than 16384)

Possible solutions:

1. We could refuse resizing a window to a size larger than supported by its current compositor. (This size should always be a reasonable maximum size for a window) (easy)
2. We could attempt to make compositors switchable for open windows. (hard)

I recommend going for option 1.
Flags: needinfo?(milan)
Comment on attachment 8708031 [details]
MozReview Request: Bug 1239743: Do not allow windows to be resized to sizes above the maximum texture size. We don't know how to draw to these anyway. r=jmathies

https://reviewboard.mozilla.org/r/30949/#review27773

lets add an NS_WARNING message here so we get something in debug builds.
Attachment #8708031 - Flags: review?(jmathies) → review+
(In reply to Jim Mathies [:jimm] from comment #3)
> Comment on attachment 8708031 [details]
> MozReview Request: Bug 1239743: Do not allow windows to be resized to sizes
> above the maximum texture size. We don't know how to draw to these anyway.
> r=jmathies
> 
> https://reviewboard.mozilla.org/r/30949/#review27773
> 
> lets add an NS_WARNING message here so we get something in debug builds.

That's a little tricky as we don't really check the sizes here.. You mean in ConstrainSize where we actually do the constriction put in a warning when we're constraining the size?
Flags: needinfo?(jmathies)
https://reviewboard.mozilla.org/r/30949/#review27853

::: widget/windows/nsWindow.cpp:1420
(Diff revision 1)
> +    int32_t maxSize = clientLayerManager->GetMaxTextureSize();

something here if maxSize < c.mMaxSize so that we get a warning when we adjust down from what the caller requested.

::: widget/windows/nsWindow.cpp:1425
(Diff revision 1)
> +    c.mMaxSize.height = std::min(c.mMaxSize.height, maxSize);

I was thinking of something in here where if maxSize < c.mMaxSize, we get a warning indicating we've adjusted down from what the caller requested.
bleh, mozreview. ignore the first.
Flags: needinfo?(jmathies)
(In reply to Jim Mathies [:jimm] from comment #5)
> https://reviewboard.mozilla.org/r/30949/#review27853
> 
> ::: widget/windows/nsWindow.cpp:1420
> (Diff revision 1)
> > +    int32_t maxSize = clientLayerManager->GetMaxTextureSize();
> 
> something here if maxSize < c.mMaxSize so that we get a warning when we
> adjust down from what the caller requested.
> 
> ::: widget/windows/nsWindow.cpp:1425
> (Diff revision 1)
> > +    c.mMaxSize.height = std::min(c.mMaxSize.height, maxSize);
> 
> I was thinking of something in here where if maxSize < c.mMaxSize, we get a
> warning indicating we've adjusted down from what the caller requested.

I would expect that to almost always be the case.. as the maximum texture size is almost always going to be smaller than the default super-large constraints that are specified :).
Flags: needinfo?(jmathies)
Yeah, I got confused reading this the first time as well.  We're not adjusting the window size, we're adjusting the limits to be used in the future.  Those limits get used somewhere else to adjust/limit the window size.

I can see us warning (if we're not already) that the user has attempted to resize the window beyond the maximum limits set here, and it could be interesting to have a warning that says "we're not allowing this size based on the limits, but the reason for it is the maximum texture size, not the other system limits".  I'm not quite sure where to do that, but it probably isn't around the code in this patch.

If we decide to warn when the size request is denied because of the maximum texture size, gfxDebug (or even gfxCriticalNote or gfxCriticalNoteOnce so that they show up in crash reports) may be enough for us, but if we really want to know how often this happens, telemetry may be a better choice.  Probably a separate bug if we go that far.
Flags: needinfo?(milan)
(In reply to Milan Sreckovic [:milan] from comment #1)
> For example,
> https://crash-stats.mozilla.com/report/index/de278270-9313-4ab2-9b54-
> ebfaa2160106

Who responsible for setting these crazy sizes, content? addons?

|[0][GFX1-]: Invalid draw target type specified: 7|[6][GFX1-]: Failed GetAsDrawTarget true, 0x1f78f000 + 16, Size(17301,1), 69204, 0|[7][GFX1-]: Failed in UpdateRenderTarget 0x887a0001, Size(17301,592), 0|[8][GFX1]: [CompositorD3D11] error code: 0x887a0001, 0x0, 0|[9][GFX1 1]: Invalid D3D11 api call|[5][GFX1-]: Invalid draw target type specified: 7
Flags: needinfo?(jmathies)
(In reply to Jim Mathies [:jimm] from comment #9)
> (In reply to Milan Sreckovic [:milan] from comment #1)
> > For example,
> > https://crash-stats.mozilla.com/report/index/de278270-9313-4ab2-9b54-
> > ebfaa2160106
> 
> Who responsible for setting these crazy sizes, content? addons?
> 
> |[0][GFX1-]: Invalid draw target type specified: 7|[6][GFX1-]: Failed
> GetAsDrawTarget true, 0x1f78f000 + 16, Size(17301,1), 69204, 0|[7][GFX1-]:
> Failed in UpdateRenderTarget 0x887a0001, Size(17301,592), 0|[8][GFX1]:
> [CompositorD3D11] error code: 0x887a0001, 0x0, 0|[9][GFX1 1]: Invalid D3D11
> api call|[5][GFX1-]: Invalid draw target type specified: 7

I would say the most likely candidate is listboxes with insanely long entries. But obviously there's a wide variety of possibilities. (Like even a user could find ways to resize a desktop window to that size by simply dragging the window to the left and extending it to the right repeatedly.
We're certainly had bugs filed with giant listboxes...
(In reply to Milan Sreckovic [:milan] from comment #11)
> We're certainly had bugs filed with giant listboxes...

Sounds like a toolkit bug, which should limit such things. 

I don't want to hold this work up. If there's no use or way of warning when we hit this, ok.
https://hg.mozilla.org/mozilla-central/rev/4f897d0f494b
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla46
You need to log in before you can comment on or make changes to this bug.