1302037 - StructuredClone should prohibit SAB in transfer map

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect, P1)

Description

[Lars T Hansen [:lth]]

See bug #1302036 - This is the bug to prohibit a SAB from being in the transfer map.  We will land this when the world is ready for it, but at the earliest one full release after the previous bug.

Comment 1

[Lars T Hansen [:lth]]

The change that allows the SAB to be omitted from the transfer map is in FF52 (and hopefully in FF51).  So this one should land no sooner than FF54.

Comment 2

[Lars T Hansen [:lth]]

Every indication is that partner content has been updated.  Tutorial materials and specs and test suites have been updated for sure.  Emscripten shipped the change in October 2016.

So we want this change for FF53 and FF54.  The spec calls for a DataCloneError to be thrown, which may require a new string (not sure - do we localize JS error messages?)

Comment 3

[Lars T Hansen [:lth]]

Attachment: bug1302037-no-sab-in-transfer.patch

Just convert the existing warning to an error.

I have verified that this throws a DOMException with name DataCloneError in the browser.

(There may be more test cases to adapt here, TBD.)

Comment 4

[Lars T Hansen [:lth]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=5f3e2b8ea633

Comment 5

[Lars T Hansen [:lth]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/ba8f78a8b0e58fa75939aaf4bf2470b0c29b74dd
Bug 1302037 - Don't allow SAB in transfer map. r=shu

Comment 6

[Lars T Hansen [:lth]]

Comment on attachment 8835929 [details] [diff] [review]
bug1302037-no-sab-in-transfer.patch

Approval Request Comment
[Feature/Bug causing the regression]:
Shared memory for JS

[User impact if declined]:
Some incorrect programs will be accepted; compatibility risk.

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

[Has the fix been verified in Nightly?]:
On inbound as I file this.

[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?]:
There is little content on the web using this feature because no browsers have shipped it enabled-by-default yet; we control most of the test cases and tool chains, and they have been updated already.

[String changes made/needed]:
None.

Comment 7

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/ba8f78a8b0e5

Comment 8

[Kohei Yoshino [:kohei]]

SharedArrayBuffer has been disabled so far; no real site-compat effect.

Comment 9

[Gerry Chang [:gchang]]

Hi :lth,
Per comment #8, do we need to uplift this to Aurora53?

Comment 10

[Lars T Hansen [:lth]]

Yes, please.  See comment #6.  Comments 7 and 8 appear to be just noise.

Comment 11

[Gerry Chang [:gchang]]

Comment on attachment 8835929 [details] [diff] [review]
bug1302037-no-sab-in-transfer.patch

Reduce the compatibility risk. Aurora53+.

Comment 12

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/44312adc8981

Comment 13

[Florian Scholz (Open Web Docs)]

Updated
https://developer.mozilla.org/en-US/Firefox/Releases/53#JavaScript
https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/SharedArrayBuffer