1210245 - <input>'s validation message changes browser content's width if it forced to appear outside of visible area

Status: RESOLVED FIXED

(Core :: Graphics: Layers, defect)

Description

[arni2033]

Attachment: A harmless one.html

// I marked it as secure so that information didn't leak. Please correct me if that's not how it works

STR:   (Win7_64, Nightly 44, 32bit, ID 20150930030231, new profile, safe mode)
1. Switch window in maximized mode
2. Open the following "data:" url or click URL in the form above
>   data:text/html,<form>Simply press/hold Enter<input autofocus required style="width:100px; background:tan; position:fixed; right:-50px;"/>
3. Make sure that input is focused. Press Enter.
4. Open attached page "A harmless one.html" to see how a page is able to do that by its own.
5. Open attached page "A deadly one.html" to crash the browser even in [e10s] mode. At least it crashes the browser for me. I guess, that happens because main process has to render content with insane width.

Result: Web page changes content's width by displaying a notification. I mean, it changes width of an
     element in browser's XUL. This also happens on wide pages with many inputs in a row, each of them
     is actually accessible by scrolling document to the right and to the left, unlike my example

Expectations: (1) and (2)
           1) Notification must appear within content area,
              even if the target element is placed outside of visible area
           2) content area must have some minimum width and minimum height for notification to display.
              Also, user can open devtools on a very small page, making it even smaller,
              and that must be taken into account. This could also fix bug 1208489.
           I think it's better to create another bug for implementing (2).

Notes:
Huh, I wanted to spread this testcase in the internet, just like those Googlechrome-crashing bugs, but decided not to do that. This could be so fun  ._.

Comment 1

[arni2033]

Attachment: A deadly one.html

Comment 2

[Daniel Veditz [:dveditz]]

I get crashes like the following in Developer Edition:
bp-0e54636e-5086-4ff0-8aaf-882aa2151005

The crash has the same signature as bug 1189715 but the stack is completely different. Doesn't seem useful to combine them at this point (and without a testcase I doubt bug 1189715 is going anywhere). The crash is a null deref.

Comment 3

[arni2033]

No... Sorry for spam, but I already explained that in this bug, no graphics is affected. window#main-window 's width is being increased by popup (by 50*100000px per second in "A deadly one.html" testcase)
So it's expected result that the browser crashes when trying to display content with 10 million px width. The only unusual thing is that page content is able to crash the browser instead of tab.

Comment 4

[arni2033]

It's actually a regression between 2014-08-21 and 2014-08-22.
https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=dac8b4a0bd7c&tochange=0b9dd32d1e16

Comment 5

[Milan Sreckovic [:milan] (needinfo for best results)]

Jet mention Markus is taking a look.

Comment 6

[:Gijs (he/him)]

(In reply to arni2033 from comment #4)
> It's actually a regression between 2014-08-21 and 2014-08-22.
> https://hg.mozilla.org/mozilla-central/
> pushloghtml?fromchange=dac8b4a0bd7c&tochange=0b9dd32d1e16

That'd be bug 691601. Pulling in Jim, Dão and Felipe who worked on that bug so they're aware.

I don't know about the graphics angle, Markus can speak to that - but it does also seem like the browser/frontend side should be preventing the form validation popup from appearing completely outside the viewing box of the <browser>, even if it's in a stack. The fact that that validation popup lives in the parent process presumably explains why this takes down the entire browser in e10s mode.

Comment 7

[Markus Stange [:mstange]]

Attachment: patch

Comment 8

[Markus Stange [:mstange]]

First I thought the browser UI was growing because of the position of the popup, because I didn't know about the existence of the anchor element, so I spent some time debugging the wrong thing.

(In reply to :Gijs Kruitbosch from comment #6)
> but it
> does also seem like the browser/frontend side should be preventing the form
> validation popup from appearing completely outside the viewing box of the
> <browser>

I think it's fine for the anchor not to be completely contained in the window; it should be in the place where the textbox is, and if that's currently clipped by the window, then it still makes sense to use the actual position and size as the anchor.

> I don't know about the graphics angle, Markus can speak to that

First I thought we were creating gigantic layers in the parent process, but I'm not so sure about that any more. I think we might instead be creating gigantic layers in the content process, because we're resizing the whole browser. We could debug that some more, in a separate bug.

Comment 9

[arni2033]

(In reply to Markus Stange [:mstange] from comment #8)
> it should be in the place where the textbox is, and if that's currently clipped by the window,
> then it still makes sense to use the actual position and size as the anchor.
Even if it overlaps minimize/maximize/close buttons OR buttons on toolbar OR items on desktop?

Comment 10

[Markus Stange [:mstange]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/5f36da2c064eb1969af22785901f3b3f0a9a2e3e
Bug 1210245 - Don't let the form validation anchor impact browser layout. r=Gijs

Comment 11

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/5f36da2c064e

Comment 12

[Markus Stange [:mstange]]

Comment on attachment 8674286 [details] [diff] [review]
patch

Approval Request Comment
[Feature/regressing bug #]: bug 691601
[User impact if declined]: web sites can temporarily break browser layout and crash the browser
[Describe test coverage new/current, TreeHerder]: there are existing tests for the code that the patch touches, but no new test was added
[Risks and why]: extremely low, very simple css fix
[String/UUID change made/needed]: none

Comment 13

[:Gijs (he/him)]

(In reply to arni2033 from comment #9)
> (In reply to Markus Stange [:mstange] from comment #8)
> > it should be in the place where the textbox is, and if that's currently clipped by the window,
> > then it still makes sense to use the actual position and size as the anchor.
> Even if it overlaps minimize/maximize/close buttons OR buttons on toolbar OR
> items on desktop?

Just for completeness: it won't actually be displayed if it's outside the browser area (that is, it will only be visible if it shows up inside the browser area). It already isn't displayed, we just make that determination differently after Markus' patch, which should avoid the gfx crash.

Comment 14

[arni2033]

(In reply to :Gijs Kruitbosch from comment #13)
> Just for completeness: it won't actually be displayed if it's outside the browser area (that is,
> it will only be visible if it shows up inside the browser area). It already isn't displayed
In case there's a misunderstanding... Here're screenshots of notifications overlapping buttons & stuff:
 1) http://ssmaker.ru/e916349d.png
 2) http://ssmaker.ru/354df821.png
So from comments here I see that this behavior is intentional. I personally think it's not perfect.
> url:   data:text/html,<form><input style="position:fixed; top:-90px; right:-85px; width:10px; height:10px;" required autofocus/>

Comment 15

[arni2033]

Oh, to reproduce the screenshots you simply press Enter when testcase is loaded - because of [autofocus]. Actually both cases are reproducible w/ "position:static;" and scrollable <body>

Comment 16

[:Gijs (he/him)]

(In reply to arni2033 from comment #14)
> (In reply to :Gijs Kruitbosch from comment #13)
> > Just for completeness: it won't actually be displayed if it's outside the browser area (that is,
> > it will only be visible if it shows up inside the browser area). It already isn't displayed
> In case there's a misunderstanding... Here're screenshots of notifications
> overlapping buttons & stuff:
>  1) http://ssmaker.ru/e916349d.png
>  2) http://ssmaker.ru/354df821.png
> So from comments here I see that this behavior is intentional. I personally
> think it's not perfect.
> > url:   data:text/html,<form><input style="position:fixed; top:-90px; right:-85px; width:10px; height:10px;" required autofocus/>

I see what you mean. Please file a separate bug.

Comment 17

[Sylvestre Ledru [:Sylvestre]]

Comment on attachment 8674286 [details] [diff] [review]
patch

Low risk but it causes a crash, it is a single css change, taking it.
Should be in 42 rc1.

Comment 18

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/fc5c56a6e318
https://hg.mozilla.org/releases/mozilla-beta/rev/f25c880e125d

Comment 19

[Al Billings [:abillings - ex-MoCo]]

Does this cause a crash if e10s is disabled?