Closed Bug 1333936 Opened 5 years ago Closed 5 years ago

Lift the Large-Allocation header process cap to 10

Categories

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

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla54
Tracking Status
firefox53 --- fixed
firefox54 --- fixed

People

(Reporter: nika, Assigned: nika)

References

Details

Attachments

(1 file, 1 obsolete file)

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.
MozReview-Commit-ID: L3dvIhhV2Ga
Attachment #8830850 - Flags: review?(ehsan)
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.
Attachment #8830850 - Flags: review?(ehsan)
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?
Attachment #8830850 - Flags: review?(bugs)
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?
Attachment #8830850 - Flags: review?(bugs) → review-
(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?
Flags: needinfo?(ehsan)
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.
Flags: needinfo?(ehsan)
MozReview-Commit-ID: L3dvIhhV2Ga
Attachment #8831258 - Flags: review?(bugs)
Attachment #8830850 - Attachment is obsolete: true
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.
Attachment #8831258 - Flags: review?(bugs) → review+
(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.
Summary: Lift the Large-Allocation header process cap to 100 → Lift the Large-Allocation header process cap to 10
Pushed by michael@thelayzells.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/37a6637a50e2
Lift the Large-Allocation header process cap to 10, r=smaug
https://hg.mozilla.org/mozilla-central/rev/37a6637a50e2
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla54
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
Attachment #8831258 - Flags: approval-mozilla-aurora?
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.
Attachment #8831258 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.