Consider ignoring the Large-Allocation header in 64-bit builds

RESOLVED FIXED in Firefox 53

Status

()

Core
DOM
RESOLVED FIXED
a year ago
a year ago

People

(Reporter: mystor, Assigned: mystor)

Tracking

unspecified
mozilla54
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(firefox53 fixed, firefox54 fixed)

Details

Attachments

(2 attachments, 3 obsolete attachments)

(Assignee)

Description

a year ago
This could help avoid creating unnecessary processes, but may hide bugs from developers who are using the header as they will likely be testing on 64-bit OSes.
(Assignee)

Comment 1

a year ago
After talking with :ehsan and :overholt, I think we're going to do this, and instead report warning messages when problematic APIs (such as ServiceWorkers and the localStorage storage events) are used on Large-Allocation documents in 64-bit builds, as this reduces the appeal of abusing the Large-Allocation header to get a clean event loop.
Assignee: nobody → michael
(Assignee)

Updated

a year ago
Blocks: 1333936
(Assignee)

Updated

a year ago
Blocks: 1331083
No longer depends on: 1331083

Comment 2

a year ago
It would be nice to report something to the console to the effect that we aren't honoring the header, but we enable some extra checks to ensure compatibility with a 32-bit Firefox.  (Note that string changes can have an impact on approvals, so you should probably check with the release manager for beta if you decide to do this.  At worst we'll ship the warning a release later.)
(Assignee)

Comment 3

a year ago
(In reply to :Ehsan Akhgari from comment #2)
> It would be nice to report something to the console to the effect that we
> aren't honoring the header, but we enable some extra checks to ensure
> compatibility with a 32-bit Firefox.  (Note that string changes can have an
> impact on approvals, so you should probably check with the release manager
> for beta if you decide to do this.  At worst we'll ship the warning a
> release later.)

These changes seem larger in scope - so I am thinking of shipping the Large-Allocation header enabled on 64-bit builds in 52, and disabling it in 64-bit builds with the new warnings (and thus new strings) in 53.
Feels weird to ship first and then immediately unship in the next release.
(Assignee)

Comment 5

a year ago
(In reply to Olli Pettay [:smaug] from comment #4)
> Feels weird to ship first and then immediately unship in the next release.

I agree - but if we want to ship this in 52 I don't want to be adding the strings required to add the important x64 diagnostics. If we want to we could ship it only on 32-bit and then add the diagnostics in 53/4?

How do you feel about that ehsan/olli?
Flags: needinfo?(ehsan)
Flags: needinfo?(bugs)

Comment 6

a year ago
(In reply to Michael Layzell [:mystor] from comment #5)
> (In reply to Olli Pettay [:smaug] from comment #4)
> > Feels weird to ship first and then immediately unship in the next release.
> 
> I agree - but if we want to ship this in 52 I don't want to be adding the
> strings required to add the important x64 diagnostics.

Like I suggested before, please check with the release manager for the beta channel; it may not be too late yet, or there may be other options.  What I meant before was that I don't know what the current rules are here, I was just trying to make sure you won't get an approval- in the end.  :-)

> If we want to we
> could ship it only on 32-bit and then add the diagnostics in 53/4?

Yes, this is my preferred option if we can't backport the console warning in prompt.  Shipping and then unshipping is a no-no.
Flags: needinfo?(ehsan)
I agree with ehsan. Once we ship, we really should ship, and not turn off almost immediately.
Flags: needinfo?(bugs)
(Assignee)

Comment 8

a year ago
Created attachment 8831310 [details] [diff] [review]
Part 1: Disable the largeAllocation header by default outside of Win32 builds

MozReview-Commit-ID: 4jUFLv4fMZa
Attachment #8831310 - Flags: review?(ehsan)
(Assignee)

Comment 9

a year ago
Created attachment 8831311 [details] [diff] [review]
Part 2: Add a new string console message for non-win32 large allocation loads

MozReview-Commit-ID: J4Zyt8Ygnfb
Attachment #8831311 - Flags: review?(ehsan)

Comment 10

a year ago
Comment on attachment 8831310 [details] [diff] [review]
Part 1: Disable the largeAllocation header by default outside of Win32 builds

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

r=me with the below fixed.  Thanks!

::: dom/base/nsContentUtils.cpp
@@ +9748,5 @@
> +    // header, instead we simply emit diagnostics into the console.
> +    outer->SetLargeAllocStatus(LargeAllocStatus::NON_WIN32);
> +    return false;
> +  }
> +#endif

I think it's not great that we're #ifdef'ing code out here.  Instead let's make the #ifdef only affect a boolean variable and use that as the default for the pref value that you're reading, I think that would be better.

::: dom/tests/browser/browser_largeAllocation.js
@@ +67,4 @@
>        ["dom.largeAllocationHeader.enabled", true],
> +      // Force-enable process creation with large-allocation, such that non
> +      // win32 builds can test the behavior.
> +      ["dom.largeAllocation.forceEnable", true],

To really test this, you should set this pref to false in the test on Win32.
Attachment #8831310 - Flags: review?(ehsan) → review+

Comment 11

a year ago
Comment on attachment 8831311 [details] [diff] [review]
Part 2: Add a new string console message for non-win32 large allocation loads

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

::: dom/locales/en-US/chrome/dom/dom.properties
@@ +318,5 @@
>  LargeAllocationNotOnlyToplevelInTabGroup=A Large-Allocation header was ignored due to the presence of windows which have a reference to this browsing context through the frame hierarchy or window.opener.
>  # LOCALIZATION NOTE: Do not translate "Large-Allocation", as it is a literal header name
>  LargeAllocationNonE10S=A Large-Allocation header was ignored due to the document not being loaded out of process.
> +# LOCALIZATION NOTE: Do not translate "Large-Allocation", as it is a literal header name. Do not translate "dom.largeAllocation.forceEnable" as it is a prefernce name.
> +LargeAllocationNonWin32=This page would be loaded in a new process due to a Large-Allocation header, but was not as firefox is not being built as a Win32 binary, and the dom.largeAllocation.forceEnable pref is not enabled.

You shouldn't use firefox in the string, you should use brandShortName.

I also suggest a slightly different text, something more like "... but was not in this version of brandshortname.  This feature is only available in Win32 versions of brandshortname."

Also please don't mention the pref name here.  The documentation for this belongs on MDN.
Attachment #8831311 - Flags: review?(ehsan) → review-
(Assignee)

Comment 12

a year ago
Created attachment 8832100 [details] [diff] [review]
Part 1: Disable the largeAllocation header by default outside of Win32 builds

MozReview-Commit-ID: 4jUFLv4fMZa
(Assignee)

Updated

a year ago
Attachment #8831310 - Attachment is obsolete: true
(Assignee)

Updated

a year ago
Attachment #8831311 - Attachment is obsolete: true
(Assignee)

Comment 13

a year ago
Created attachment 8832101 [details] [diff] [review]
Part 2: Add a new string console message for non-win32 large allocation loads

MozReview-Commit-ID: J4Zyt8Ygnfb
Attachment #8832101 - Flags: review?(ehsan)
(Assignee)

Comment 14

a year ago
Created attachment 8832122 [details] [diff] [review]
Part 1: Disable the largeAllocation header by default outside of Win32 builds

MozReview-Commit-ID: 4jUFLv4fMZa
(Assignee)

Updated

a year ago
Attachment #8832100 - Attachment is obsolete: true

Updated

a year ago
Attachment #8832101 - Flags: review?(ehsan) → review+

Comment 15

a year ago
Pushed by michael@thelayzells.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/ff60a5f2e0c0
Part 1: Disable the largeAllocation header by default outside of Win32 builds, r=ehsan
https://hg.mozilla.org/integration/mozilla-inbound/rev/e52daa859232
Part 2: Add a new string console message for non-win32 large allocation loads, r=ehsan

Comment 16

a year ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/ff60a5f2e0c0
https://hg.mozilla.org/mozilla-central/rev/e52daa859232
Status: NEW → RESOLVED
Last Resolved: a year ago
status-firefox54: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla54
Passing by question: isn't this string quite convoluted? I'd expect at least the first "would" to be "should", also non-win32 forced my brain to interpret the sentence.

This page would be loaded in a new process due to a Large-Allocation header, however Large-Allocation process creation is disabled on non-Win32 platforms.

For example: "This page should be loaded in a new process due to a Large-Allocation header, however Large-Allocation process creation is available only on Win32 platforms."
(Assignee)

Comment 18

a year ago
Comment on attachment 8832122 [details] [diff] [review]
Part 1: Disable the largeAllocation header by default outside of Win32 builds

Approval Request Comment

This is part 6 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 #8832122 - Flags: approval-mozilla-aurora?
(Assignee)

Comment 19

a year ago
Comment on attachment 8832101 [details] [diff] [review]
Part 2: Add a new string console message for non-win32 large allocation loads

Approval Request Comment

This is part 7 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 #8832101 - Flags: approval-mozilla-aurora?
(In reply to Michael Layzell [:mystor] from comment #19)
> [String changes made/needed]: A string was added to dom.properties in bug
> 1331087

I strongly suggest to make a patch hard-coding the string for aurora, and let the proper fix ride the trains.
(Assignee)

Comment 21

a year ago
(In reply to Francesco Lodolo [:flod] from comment #20)
> (In reply to Michael Layzell [:mystor] from comment #19)
> > [String changes made/needed]: A string was added to dom.properties in bug
> > 1331087
> 
> I strongly suggest to make a patch hard-coding the string for aurora, and
> let the proper fix ride the trains.

The patches on this bug are separate such that if the string change is not approved, we can land it anyway (by simply not landing part 2) with suboptimal messaging.
status-firefox53: --- → affected
Comment on attachment 8832122 [details] [diff] [review]
Part 1: Disable the largeAllocation header by default outside of Win32 builds

Help with win32 OOM crashes, ok to uplift to aurora.
Attachment #8832122 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Attachment #8832101 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+

Comment 23

a year ago
bugherderuplift
https://hg.mozilla.org/releases/mozilla-aurora/rev/c422e37f1667
status-firefox53: affected → fixed
Flags: in-testsuite+
(In reply to Michael Layzell [:mystor] from comment #21)
> The patches on this bug are separate such that if the string change is not
> approved, we can land it anyway (by simply not landing part 2) with
> suboptimal messaging.

Thanks, that was a very smart approach. Hopefully we won't need it for much longer (q2).
You need to log in before you can comment on or make changes to this bug.