1251527 - [e10s] loading https://500px.com/popular and clicking a photo causes Nightly to hang/use large amount of memory

Status: RESOLVED FIXED

(Core :: Layout, defect)

Description

[:Gavin Sharp [email: [e-mail]]]

STR:
1) Start Nightly (I'm currently testing the Feb 25 build, built off mozilla-central c1e0d1890cfee9d86c8d566b0490053f21e0afc6) with a new profile
2) load https://500px.com/popular
3) click one of the photos to "zoom in"

Nightly hangs. At first just the content process is non-responsive (switching tabs results in content spinner), but eventually the whole app gets unresponsive too. This has also caused OS X to freak out and pop up the "System out of memory" dialog, which in one case caused all my other open apps to be paused and hang and required a reboot.

Comment 1

[:Gavin Sharp [email: [e-mail]]]

Seems to only be an issue on Mac, markh says Windows worked fine.

Comment 2

[Mark Hammond [:markh] [:mhammond]]

I can repro this on OSX - content process seems to grow by 4GB per photo. I can't repro it on Windows - browser stays responsive, content process memory doesn't grow significantly.

Comment 3

[Timothy Nikkel (:tnikkel)]

Only happens with e10s for me.

Comment 4

[Timothy Nikkel (:tnikkel)]

Regression window:

https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=3f41d7d0f544ebd98273e39bd945c28878a47427&tochange=c0ba5835ca489d15f8f170d5deb01f8dad92709a

Comment 5

[:Gavin Sharp [email: [e-mail]]]

First bad revision:
changeset:   281578:c99e8abb5e00
user:        Matt Woodrow <[e-mail]>
date:        Tue Jan 26 13:36:48 2016 +1300
summary:     Bug 1231538 - Build a ContainerLayer for position:fixed and background-attachment:fixed content. r=roc

Comment 6

[Jim Mathies [:jimm]]

[Tracking Requested - why for this release]:
regression from recent landing

Comment 7

[Jim Mathies [:jimm]]

This is waiting on a backout of bug 1231538.

Comment 8

[Matt Woodrow (:mattwoodrow)]

Backing that out will likely be difficult.

I'm on a work week this week, I'll fix this early next week.

Comment 9

[Benjamin Kerensa [:bkerensa]]

Pretty bad regression lets track this can we assign you Matt?

Comment 10

[Matt Woodrow (:mattwoodrow)]

Yes, absolutely.

Comment 11

[Matt Woodrow (:mattwoodrow)]

Attachment: Don't set the visible region to the bounds

This is clearly wrong.

I can't see what purpose it served, and removing it doesn't make it fail any tests.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=4571c99eda5b

Might be worth waiting a bit before uplifting, in case I missed a valid reason for this existing.

Comment 12

[Markus Stange [:mstange]]

Comment on attachment 8728182 [details] [diff] [review]
Don't set the visible region to the bounds

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

Yeah this seems very strange. Have you checked the history of this line?

Comment 13

[Matt Woodrow (:mattwoodrow)]

(In reply to Markus Stange [:mstange] from comment #12)
> Comment on attachment 8728182 [details] [diff] [review]
> Don't set the visible region to the bounds
> 
> Review of attachment 8728182 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Yeah this seems very strange. Have you checked the history of this line?

Yes, I wrote it for the regressing patch. I have no idea why.

Comment 14

[Markus Stange [:mstange]]

Ah, probably for background-attachment:fixed then. But if removing it doesn't fail any tests then we should be fine.

Comment 15

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/735e3eec00ad

Comment 16

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/735e3eec00ad

Comment 17

[:Felipe Gomes (needinfo for replies!)]

Good ol' fixing a bug with just code removal!

Comment 18

[Chris Peterson [:cpeterson]]

Matt, can we uplift this e10s fix to Beta 46? We're running an e10s experiment on Beta 46, so we want to be testing with all the e10s fixes for known issues.

Comment 19

[Matt Woodrow (:mattwoodrow)]

(In reply to Chris Peterson [:cpeterson] from comment #18)
> Matt, can we uplift this e10s fix to Beta 46? We're running an e10s
> experiment on Beta 46, so we want to be testing with all the e10s fixes for
> known issues.

46 isn't affected by this bug.

We should probably uplift to 47, but I'd rather let it sit for a bit to make sure it doesn't introduce new regressions.

Comment 20

[Chris Peterson [:cpeterson]]

Sounds good. We'll keep tracking this for 47.

Comment 21

[Kartikaya Gupta (email:[e-mail])]

It's been close to a month with no regressions, should be safe to uplift to 47?

Comment 22

[Chris Peterson [:cpeterson]]

Matt, should this fix be uplifted to Aurora 47 in preparation for our e10s experiments on Beta 47?

Comment 23

[Matt Woodrow (:mattwoodrow)]

Comment on attachment 8728182 [details] [diff] [review]
Don't set the visible region to the bounds

Approval Request Comment
[Feature/regressing bug #]: Bug 1231538
[User impact if declined]: Massive memory usage (with e10s/apz) on sites with large background-attachment:fixed images.
[Describe test coverage new/current, TreeHerder]: Tested manually.
[Risks and why]: Fairly low risk, just removes some unnecessary code that the previous bug added.
[String/UUID change made/needed]: None

Comment 24

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Comment on attachment 8728182 [details] [diff] [review]
Don't set the visible region to the bounds

Regression in e10s, Aurora47+

Comment 25

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/80ee357092b5