Closed Bug 1361520 Opened 3 years ago Closed 3 years ago

Disable SharedArrayBuffer in Fx54

Categories

(Core :: JavaScript Engine, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla54
Tracking Status
firefox54 + fixed
firefox55 - wontfix

People

(Reporter: jimm, Assigned: lth)

References

(Blocks 1 open bug)

Details

(Keywords: dev-doc-complete)

Attachments

(1 file)

[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
Tracking 54/55+. Adding Erin to the bug. Not sure if there are others we need to add to the bug?
(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.
Flags: needinfo?(jmathies)
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?
Flags: needinfo?(jmathies) → needinfo?(lhansen)
(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.
Flags: needinfo?(lhansen)
Ok, sounds good. Thanks for the detail.
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → INVALID
Based on the invalid status, updating the tracking flags.
Status: RESOLVED → REOPENED
Resolution: INVALID → ---
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).
Assignee: nobody → lhansen
Status: REOPENED → ASSIGNED
Same drill as for FxF2 and Fx53: patch to disable SAB/Atomics on Beta.
Attachment #8866691 - Flags: review?(shu)
Attachment #8866691 - Flags: review?(shu) → review+
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
Attachment #8866691 - Flags: approval-mozilla-beta?
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.
Attachment #8866691 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Status: ASSIGNED → RESOLVED
Closed: 3 years ago3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla54
You need to log in before you can comment on or make changes to this bug.