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

RESOLVED FIXED in Firefox 53

Status

()

defect
RESOLVED FIXED
3 years ago
4 months ago

People

(Reporter: Nika, Assigned: Nika)

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)

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.
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
Blocks: 1333936
Blocks: 1331083
No longer depends on: 1331083
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.)
(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.
(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)
(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)
MozReview-Commit-ID: 4jUFLv4fMZa
Attachment #8831310 - Flags: review?(ehsan)
MozReview-Commit-ID: J4Zyt8Ygnfb
Attachment #8831311 - Flags: review?(ehsan)
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 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-
Attachment #8831310 - Attachment is obsolete: true
Attachment #8831311 - Attachment is obsolete: true
Attachment #8832100 - Attachment is obsolete: true
Attachment #8832101 - Flags: review?(ehsan) → review+
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
https://hg.mozilla.org/mozilla-central/rev/ff60a5f2e0c0
https://hg.mozilla.org/mozilla-central/rev/e52daa859232
Status: NEW → RESOLVED
Closed: 3 years ago
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."
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?
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.
(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.
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+
(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).
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.