Closed Bug 1210245 Opened 5 years ago Closed 5 years ago

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

Categories

(Core :: Graphics: Layers, defect)

defect
Not set
critical

Tracking

()

RESOLVED FIXED
mozilla44
Tracking Status
e10s + ---
firefox42 --- fixed
firefox43 --- fixed
firefox44 --- fixed

People

(Reporter: arni2033, Assigned: mstange)

References

()

Details

(5 keywords, Whiteboard: [post-critsmash-triage][adv-main42-])

Crash Data

Attachments

(3 files)

Attached file 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  ._.
Attached file A deadly one.html
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.
Group: core-security → core-security-release
Component: Untriaged → Graphics: Layers
Crash Signature: [@ gfxContext::gfxContext(mozilla::gfx::DrawTarget*, mozilla::gfx::PointTyped<T> const&) ]
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.
It's actually a regression between 2014-08-21 and 2014-08-22.
https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=dac8b4a0bd7c&tochange=0b9dd32d1e16
Keywords: regression
Jet mention Markus is taking a look.
Assignee: nobody → mstange
(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.
Crash Signature: [@ gfxContext::gfxContext(mozilla::gfx::DrawTarget*, mozilla::gfx::PointTyped<T> const&) ] → [@ gfxContext::gfxContext(mozilla::gfx::DrawTarget*, mozilla::gfx::PointTyped<T> const&) ] [@ gfxContext::gfxContext ]
Group: gfx-core-security
Attached patch patchSplinter Review
Attachment #8674286 - Flags: review?(dao)
Attachment #8674286 - Flags: review?(dao) → review+
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.
(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?
https://hg.mozilla.org/integration/mozilla-inbound/rev/5f36da2c064eb1969af22785901f3b3f0a9a2e3e
Bug 1210245 - Don't let the form validation anchor impact browser layout. r=Gijs
https://hg.mozilla.org/mozilla-central/rev/5f36da2c064e
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla44
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
Attachment #8674286 - Flags: approval-mozilla-beta?
Attachment #8674286 - Flags: approval-mozilla-aurora?
(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.
(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/>
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>
(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 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.
Attachment #8674286 - Flags: approval-mozilla-beta?
Attachment #8674286 - Flags: approval-mozilla-beta+
Attachment #8674286 - Flags: approval-mozilla-aurora?
Attachment #8674286 - Flags: approval-mozilla-aurora+
Group: gfx-core-security
Does this cause a crash if e10s is disabled?
Whiteboard: [post-critsmash-triage]
See Also: → 1219065
Whiteboard: [post-critsmash-triage] → [post-critsmash-triage][adv-main42-]
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.