1333936 - Lift the Large-Allocation header process cap to 10

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[Nika Layzell [:nika] (ni? for response)]

This should greatly reduce the chances that pages will deny other pages access to address space. As the Large-Allocation header won't be supported on non-32 bit windows builds, we think that it will not be abused and thus having a low limit won't be as important.

Comment 1

[Nika Layzell [:nika] (ni? for response)]

Attachment: Lift the Large-Allocation header process cap to 100

MozReview-Commit-ID: L3dvIhhV2Ga

Comment 2

[(no longer active)]

Comment on attachment 8830850 [details] [diff] [review]
Lift the Large-Allocation header process cap to 100

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

This is fine as far I'm concerned, but I think you should ask the person who asked you to set the original limit to 2 to weigh in on it.

Comment 3

[Nika Layzell [:nika] (ni? for response)]

Comment on attachment 8830850 [details] [diff] [review]
Lift the Large-Allocation header process cap to 100

Olli, you reviewed the original patch where I set the limit to the low number of 2 - how do you feel about the decision to increase it to 100?

Comment 4

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8830850 [details] [diff] [review]
Lift the Large-Allocation header process cap to 100

Isn't this crazy high. Have you done any testing what happens when tons of new child processes are created?

If web pages misuse the header, we shouldn't let them to create more and more processes, IMO.
Use 10 and have a test what happens when there are 10 LA processes and what happens when 11 is tried to be created?

Comment 5

[Nika Layzell [:nika] (ni? for response)]

(In reply to Olli Pettay [:smaug] from comment #4)
> Comment on attachment 8830850 [details] [diff] [review]
> Lift the Large-Allocation header process cap to 100
> 
> Isn't this crazy high. Have you done any testing what happens when tons of
> new child processes are created?
> 
> If web pages misuse the header, we shouldn't let them to create more and
> more processes, IMO.
> Use 10 and have a test what happens when there are 10 LA processes and what
> happens when 11 is tried to be created?

I am adding tests for LA process exhaustion in bug 1334210. I can move the number back down to ~10ish because we're very unlikely to ever exceed that number of tabs performing large allocations, especially on an old 32-bit machine.

Ehsan, you liked the idea of us using a high limit and relying on physical resource exhaustion more than currently. How do you feel about that option?

Comment 6

[(no longer active)]

10 and 100 are the same thing IMO, because most of our users have much fewer maximum number of tabs than 10, so if we want to use that value which is more conservative that's fine by me.

Comment 7

[Nika Layzell [:nika] (ni? for response)]

Attachment: Lift the Large-Allocation header process cap to 10

MozReview-Commit-ID: L3dvIhhV2Ga

Comment 8

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8831258 [details] [diff] [review]
Lift the Large-Allocation header process cap to 10

And please test at least locally that nothing super bad happens with 10 or more LA processes.

Comment 9

[Nika Layzell [:nika] (ni? for response)]

(In reply to Olli Pettay [:smaug] from comment #8)
> Comment on attachment 8831258 [details] [diff] [review]
> Lift the Large-Allocation header process cap to 10
> 
> And please test at least locally that nothing super bad happens with 10 or
> more LA processes.

As I said in comment 5, I have an automated test to make sure nothing bad happens in that situation in bug 1334210.

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/37a6637a50e2
Lift the Large-Allocation header process cap to 10, r=smaug

Comment 11

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/37a6637a50e2

Comment 12

[Nika Layzell [:nika] (ni? for response)]

Comment on attachment 8831258 [details] [diff] [review]
Lift the Large-Allocation header process cap to 10

Approval Request Comment

This is part 4 of a 6-bug/7-patch uplift request

[Feature/Bug causing the regression]: We would like to ship the Large-Allocation header in 52/53, and these patches make some final touch-ups which will be required before we can ship.
[User impact if declined]: Increased crash rates of wasm games on 32-bit windows
[Is this code covered by automated tests?]: Yes
[Has the fix been verified in Nightly?]: Most of these fixups just landed in Nightly.
[Needs manual test from QE? If yes, steps to reproduce]: No.
[List of other uplifts needed for the feature/fix]: 
bug 1331083
bug 1332343
bug 1334210
bug 1333936
bug 1334586
bug 1331087
[Is the change risky?]: Not very
[Why is the change risky/not risky?]: This exposes and enables an opt-in low risk web API which is only supported by Firefox to help avoid OOM problems on 32-bit windows.
[String changes made/needed]: A string was added to dom.properties in bug 1331087

Comment 13

[Liz Henry (:lizzard) (relman/hg->git project)]

Comment on attachment 8831258 [details] [diff] [review]
Lift the Large-Allocation header process cap to 10

Should help with OOM crashes, let's take this in aurora.

Comment 14

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/06cd952eecee