642205 - Do not build layers for asynchronous scrolling if element shouldn't be scrollable

Status: VERIFIED FIXED

(Core :: Graphics, defect)

Description

[Benjamin Stover (:stechz)]

For instance in Fennec, the text on:
http://blogs.msdn.com/b/ie/archive/2011/03/14/web-tracking-protection-minimum-standards-and-opportunities-to-innovate.aspx

Is scrollable when it shouldn't be.

Comment 1

[Benjamin Stover (:stechz)]

Attachment: Do not build layers for asynchronous scrolling if element shouldn't be scrollable

Comment 2

[Brad Lassey [:blassey] (use needinfo?)]

screen cast of the issue http://lassey.us/moving-content.webm

this looks pretty bad and may lead to some potentially bad memory churn, marking as blocking

Comment 4

[Brad Lassey [:blassey] (use needinfo?)]

this patch fixes the problem in my local build

Comment 5

[Timothy Nikkel (:tnikkel)]

Comment on attachment 519720 [details] [diff] [review]
Do not build layers for asynchronous scrolling if element shouldn't be scrollable

I think you want the condition to be != NS_STYLE_OVERFLOW_HIDDEN as I think you want to include overflow: auto.

Comment 6

[Benjamin Stover (:stechz)]

Attachment: Do not build layers for asynchronous scrolling if element shouldn't be scrollable

Good call.

Comment 7

[Benjamin Stover (:stechz)]

http://hg.mozilla.org/mozilla-central/rev/54cb5591480e

Comment 8

[Benjamin Stover (:stechz)]

Pushed followup for now bogus assertion: http://hg.mozilla.org/mozilla-central/rev/821740bc1b4d

Comment 9

[Matt Brubeck (:mbrubeck)]

I am still seeing this in today's nightly build and local trunk builds on Android (but not on desktop Linux).

Comment 11

[Benjamin Stover (:stechz)]

Attachment: Followup: workaround for Android compiler bug

I think I've stumbled upon my first ever compiler bug! We are seeing bogus values for result that is returned from GetScrollbarStylesFromFrame. Returning the result directly inside the conditional fixes the problem for me. Can others verify?

Comment 12

[Matt Brubeck (:mbrubeck)]

Comment on attachment 520052 [details] [diff] [review]
Followup: workaround for Android compiler bug

This fixes the bug for me, in my local Android opt build.

Comment 13

[Brad Lassey [:blassey] (use needinfo?)]

this also fixes the problem for me

Comment 14

[Matt Brubeck (:mbrubeck)]

Without this patch I get a reproducible OOM crash on multiple pages.  For example:
1. Open http://demos.mozilla.org
2. Pinch zoom in as far as possible on the "sliding cards"
3. Pan left and right.

This results in an OOM content-process crash that also leaves the browser unable to pan until it is force-killed.

The followup patch fixes this.

Comment 15

[Brad Lassey [:blassey] (use needinfo?)]

pushed:
https://hg.mozilla.org/mozilla-central/rev/9c3ef79b8eba
https://hg.mozilla.org/releases/mozilla-2.1/rev/43ac87e18a5d
https://hg.mozilla.org/releases/mozilla-2.1/rev/a0181e3f66fe

Comment 16

[Aakash Desai [:aakashd]]

verified FIXED on builds:
    Mozilla/5.0 (Android; Linux armv7l; rv:2.1) Gecko/20110318 Firefox/4.0b13pre Fennec/4.0 ID:20110318114419

Comment 17

[Aakash Desai [:aakashd]]

(In reply to comment #4)
> this patch fixes the problem in my local build

verified FIXED with the use case mentioned in comment #4 as well.