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

Status: RESOLVED FIXED

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

Description

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

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.

Comment 1

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

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.

Comment 2

[(no longer active)]

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.)

Comment 3

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

(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.

Comment 4

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

Feels weird to ship first and then immediately unship in the next release.

Comment 5

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

(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?

Comment 6

[(no longer active)]

(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.

Comment 7

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

I agree with ehsan. Once we ship, we really should ship, and not turn off almost immediately.

Comment 8

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

Attachment: Part 1: Disable the largeAllocation header by default outside of Win32 builds

MozReview-Commit-ID: 4jUFLv4fMZa

Comment 9

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

Attachment: Part 2: Add a new string console message for non-win32 large allocation loads

MozReview-Commit-ID: J4Zyt8Ygnfb

Comment 10

[(no longer active)]

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.

Comment 11

[(no longer active)]

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.

Comment 12

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

Attachment: Part 1: Disable the largeAllocation header by default outside of Win32 builds

MozReview-Commit-ID: 4jUFLv4fMZa

Comment 13

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

Attachment: Part 2: Add a new string console message for non-win32 large allocation loads

MozReview-Commit-ID: J4Zyt8Ygnfb

Comment 14

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

Attachment: Part 1: Disable the largeAllocation header by default outside of Win32 builds

MozReview-Commit-ID: 4jUFLv4fMZa

Comment 15

[Pulsebot]

Pushed by [e-mail]:
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

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/ff60a5f2e0c0
https://hg.mozilla.org/mozilla-central/rev/e52daa859232

Comment 17

[Francesco Lodolo [:flod]]

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 18

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

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

Comment 19

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

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

Comment 20

[Francesco Lodolo [:flod]]

(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.

Comment 21

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

(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 22

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

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.

Comment 23

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/c422e37f1667

Comment 24

[Francesco Lodolo [:flod]]

(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).