Jim Mathies [:jimm] Reporter
	Description • 7 years ago

[Tracking Requested - why for this release]:

There's been some discussion about SAB compat with e10s multi which will ship in 54 or 55. If the SAB feature isn't ready to ship with e10s multi, we should consider disabling it in beta so there's no risk the removal of the feature regresses something in beta builds. Adding this tracking bug so that we make sure and make a decision here before we're ~3 weeks out from 54 release. 

https://dxr.mozilla.org/mozilla-beta/source/js/src/moz.build#685

	Marcia Knous [:marcia]
	Comment 1 • 7 years ago

Tracking 54/55+. Adding Erin to the bug. Not sure if there are others we need to add to the bug?

	Lars T Hansen [:lth] Assignee
	Comment 2 • 7 years ago

(In reply to Jim Mathies [:jimm] from comment #0)
> [Tracking Requested - why for this release]:
> 
> There's been some discussion about SAB compat with e10s multi which will
> ship in 54 or 55. If the SAB feature isn't ready to ship with e10s multi, 

What evidence is there for that claim?  I'm not aware of any.

	Jim Mathies [:jimm] Reporter
	Comment 3 • 7 years ago

Reading over bug 1232973 it sounds like our implementation is incomplete, or the spec is incomplete, and that we've disabled functionality such that certain bits fail which avoids issues with e10s-multi. Can you clear up the situation here Lars? Is this feature ready to ship, or do we have development that still needs to happen?

Also I'm curious how our implementation looks compared to other browsers. Is Chrome shipping this? Do they have the same issues with multiple child processes?

	Lars T Hansen [:lth] Assignee
	Comment 4 • 7 years ago

(In reply to Jim Mathies [:jimm] from comment #3)

> Reading over bug 1232973 it sounds like our implementation is incomplete, or
> the spec is incomplete, and that we've disabled functionality such that
> certain bits fail which avoids issues with e10s-multi. Can you clear up the
> situation here Lars? Is this feature ready to ship, or do we have
> development that still needs to happen?

The feature is ready to ship and should ship.  We support every aspect of the feature specified in ECMAScript 2017 and a common (among the browsers) subset of the to-be-standardized HTML/DOM support.

There is more development that should happen to enable additional functionality that is in the process of being added to the HTML/DOM specs, but that development should not block the feature shipping.

Specifically (now excerpting from the not-yet-posted intent-to-ship message), we have at least these known deficiencies wrt to the upcoming changes to HTML/DOM:

- We support WebGL 1.0 API integration only at this time, not WebGL 2.0, for the APIs that accept shared memory; there is not yet any agreed spec for the WebGL 2.0 APIs

- We disallow blocking waits in SharedWorker, see bug 1359745 for details

- We disallow passing SharedArrayBuffers on MessageChannels and BroadcastChannels, see bug 1360190 for details

(That last bullet allows us not to worry about e10s, so far as I know.)

None of those problems materially impact the use of shared memory for most of its intended tasks.  There is no web content for shared memory at this time and none will be affected by this deficiency.

> Also I'm curious how our implementation looks compared to other browsers. Is
> Chrome shipping this? Do they have the same issues with multiple child
> processes?

Safari and Chrome are both shipping this, supposedly in Safari 10.1 and Chrome 60.  What I have heard is that they, too, fail the (upcoming) HTML test cases in various ways because the HTML integration has been a work in progress with all of us.  Microsoft has implemented SAB but I have not heard a ship date.

	Jim Mathies [:jimm] Reporter
	Comment 5 • 7 years ago

Ok, sounds good. Thanks for the detail.

	Marcia Knous [:marcia]
	Comment 6 • 7 years ago

Based on the invalid status, updating the tracking flags.

	Lars T Hansen [:lth] Assignee
	Comment 7 • 7 years ago

By general consensus we are shipping in FF55, so we need to set the flag's default value to false in FF54.  Patch coming up; this will be identical to the patch in bug 1354160 (for FF53).

	Lars T Hansen [:lth] Assignee
	Comment 8 • 7 years ago

Same drill as for FxF2 and Fx53: patch to disable SAB/Atomics on Beta.

	Lars T Hansen [:lth] Assignee
	Comment 9 • 7 years ago

Comment on attachment 8866691 [details] [diff] [review]
bug1361520-disable-sab-and-atomics-on-54beta.patch

Approval Request Comment
[Feature/Bug causing the regression]:
Disable SharedArrayBuffer / Atomics for Fx54 Beta/Release because we're shipping in Fx55

[User impact if declined]:
We will ship the API too soon

[Is this code covered by automated tests?]:
Yes

[Has the fix been verified in Nightly?]:
N/A, a Beta-only fix to the default setting of a config flag

[Needs manual test from QE? If yes, steps to reproduce]: 
No

[List of other uplifts needed for the feature/fix]:
None

[Is the change risky?]:
No

[Why is the change risky/not risky?]:
We did this for Fx52 and Fx53 without ill effect, this is the same patch, modulo some line numbers

[String changes made/needed]:
None

	Gerry Chang [:gchang]
	Comment 10 • 7 years ago

Comment on attachment 8866691 [details] [diff] [review]
bug1361520-disable-sab-and-atomics-on-54beta.patch

 Disable SharedArrayBuffer in Fx54. Beta54+. Should be in 54 beta 8.

	Ryan VanderMeulen [:RyanVM]
	Comment 11 • 7 years ago
bugherder uplift

https://hg.mozilla.org/releases/mozilla-beta/rev/30426b6130f6

	Florian Scholz (Open Web Docs)
	Comment 12 • 7 years ago

Added SAB to developer release notes for Firefox 55
https://developer.mozilla.org/en-US/Firefox/Releases/55#JavaScript

Updated SharedArrayBuffer and Atomics compat info (also on the sub pages)
https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/SharedArrayBuffer#Browser_compatibility
https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Atomics#Browser_compatibility