Closed Bug 1251527 Opened 4 years ago Closed 4 years ago

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

Categories

(Core :: Layout, defect)

Unspecified
macOS
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla48
Tracking Status
e10s m9+ ---
firefox45 --- unaffected
firefox46 --- unaffected
firefox47 + fixed
firefox48 --- fixed

People

(Reporter: Gavin, Assigned: mattwoodrow)

References

Details

(Keywords: crash, Whiteboard: [MemShrink])

Attachments

(1 file)

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.
Keywords: crash
Seems to only be an issue on Mac, markh says Windows worked fine.
OS: Unspecified → Mac OS X
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.
Whiteboard: [MemShrink]
Only happens with e10s for me.
tracking-e10s: --- → ?
Summary: loading https://500px.com/popular and clicking a photo causes Nightly to hang/use large amount of memory → [e10s] loading https://500px.com/popular and clicking a photo causes Nightly to hang/use large amount of memory
First bad revision:
changeset:   281578:c99e8abb5e00
user:        Matt Woodrow <mwoodrow@mozilla.com>
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
Component: General → Layout
[Tracking Requested - why for this release]:
regression from recent landing
This is waiting on a backout of bug 1231538.
Backing that out will likely be difficult.

I'm on a work week this week, I'll fix this early next week.
Pretty bad regression lets track this can we assign you Matt?
Flags: needinfo?(matt.woodrow)
Yes, absolutely.
Assignee: nobody → matt.woodrow
Flags: needinfo?(matt.woodrow)
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.
Attachment #8728182 - Flags: review?(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?
Attachment #8728182 - Flags: review?(mstange) → review+
(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.
Ah, probably for background-attachment:fixed then. But if removing it doesn't fail any tests then we should be fine.
https://hg.mozilla.org/mozilla-central/rev/735e3eec00ad
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
Good ol' fixing a bug with just code removal!
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.
Flags: needinfo?(matt.woodrow)
(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.
Flags: needinfo?(matt.woodrow)
Sounds good. We'll keep tracking this for 47.
It's been close to a month with no regressions, should be safe to uplift to 47?
Matt, should this fix be uplifted to Aurora 47 in preparation for our e10s experiments on Beta 47?
Flags: needinfo?(matt.woodrow)
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
Flags: needinfo?(matt.woodrow)
Attachment #8728182 - Flags: approval-mozilla-aurora?
Comment on attachment 8728182 [details] [diff] [review]
Don't set the visible region to the bounds

Regression in e10s, Aurora47+
Attachment #8728182 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
No longer blocks: 1273024
You need to log in before you can comment on or make changes to this bug.