1587007 - Improve error messages when we fail to clone shared memory for policy reasons

Status: RESOLVED FIXED

(Core :: JavaScript Engine, enhancement, P3)

Description

[Lars T Hansen [:lth]]

See bug 1586217. When structured cloning rejects something with shared memory, it should be very specific about why. We can probably just fix the error message to be better; to do really well, we might need to propagate information about what failed in from the DOM.

Comment 1

[Lars T Hansen [:lth]]

Specific suggestion for how to phrase error is on https://bugzilla.mozilla.org/show_bug.cgi?id=1562667#c1 et seq.

Comment 2

[Tom Tung [:tt, :ttung]]

Attachment: Bug 1587007 - Improve error messages for failing to serialize a shared memory object duo to COOP and COEP;

Comment 3

[Tom Tung [:tt, :ttung]]

Attachment: Screen Shot 2019-10-21 at 11.29.11.png

The new warning message would be like this if applying the current patch.

Comment 4

[Tom Tung [:tt, :ttung]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=843d0302d49b77bacc1853ddb4b1fd10fecfa826

Comment 5

[Lars T Hansen [:lth]]

Message seems to have some duplicated prose, a dangling "cannot be cloned in this context." phrase at the end.

Comment 6

[Tom Tung [:tt, :ttung]]

(In reply to Lars T Hansen [:lth] from comment #5)

Message seems to have some duplicated prose, a dangling "cannot be cloned in this context." phrase at the end.

Ah, thanks for the notice! Will correct that.

Comment 7

[Tom Tung [:tt, :ttung]]

Attachment: Screen Shot 2019-10-21 at 13.59.20.png

Newer error message

Comment 8

[Lars T Hansen [:lth]]

I'm curious about the "in the future" at the end of that message. What's the intent of this phrasing?

Comment 9

[Tom Tung [:tt, :ttung]]

(In reply to Lars T Hansen [:lth] from comment #8)

I'm curious about the "in the future" at the end of that message. What's the intent of this phrasing?

Redirecting this to Anne, but if I understanding correctly, we target to ship these (reSAB, COOP, COEP stuff) or, more accurate, enabling these by default for Nightly/Beta in 72.

And at that time we wanted to avoid wording like "soon" to reduce misunderstanding (people interpret soon differently)

Comment 10

[Lars T Hansen [:lth]]

Ok, awaiting something from Anne. But this patch will land in Nightly, so that'll be 72 now, and we should consider that. The message should reflect the functionality actually in the browser where the message is. If it is different in different versions, or if it needs to change dependent on what functionality is enabled, then that's complexity we just have to cope with.

Comment 11

[Anne (:annevk)]

Folks that enable shared memory anywhere (Nightly, Dev, Beta, or Release) will end up seeing these messages due to recent postMessage() changes. The sooner they get a message with something to search for the better. However, we're not yet sure what release the headers will end up shipping in. The idea is/was 72, but that's likely too early as not everything is complete at this point and the whole setup also needs some time to bake.

Alternative idea: if the COOP and COEP preferences are enabled, the message reads:

The {X} object cannot be serialized. The Cross-Origin-Opener-Policy and Cross-Origin-Embedder-Policy HTTP headers can be used to enable this.

If either preference is not enabled:

The {X} object cannot be serialized. The Cross-Origin-Opener-Policy and Cross-Origin-Embedder-Policy HTTP headers will enable this in the future.

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/e7bb97bf40c1
Improve error messages for failing to serialize a shared memory object duo to COOP and COEP; r=nika

Comment 13

[Tom Tung [:tt, :ttung]]

Marking leave-open to address comment #11 or something similar.

(I assume "in the future" now meaning enabling the pref by default; Although this is expected to happen in the same release (71), we don't enable them by default at this moment. Another point is that, we might want to uplift this to beta)

Comment 14

[Tom Tung [:tt, :ttung]]

(In reply to Anne (:annevk) from comment #11)

Folks that enable shared memory anywhere (Nightly, Dev, Beta, or Release) will end up seeing these messages due to recent postMessage() changes. The sooner they get a message with something to search for the better. However, we're not yet sure what release the headers will end up shipping in. The idea is/was 72, but that's likely too early as not everything is complete at this point and the whole setup also needs some time to bake.

Alternative idea: if the COOP and COEP preferences are enabled, the message reads:

The {X} object cannot be serialized. The Cross-Origin-Opener-Policy and Cross-Origin-Embedder-Policy HTTP headers can be used to enable this.

If either preference is not enabled:

The {X} object cannot be serialized. The Cross-Origin-Opener-Policy and Cross-Origin-Embedder-Policy HTTP headers will enable this in the future.

Lars, does the alternative idea look good to you? Or, do you have another suggestion? I can create a patch for that quickly.

Comment 15

[Natalia Csoregi [:nataliaCs]]

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

Comment 16

[Lars T Hansen [:lth]]

The proposal from Anne looks OK to me. Though I'm not sure why, in the second case, we couldn't point them to the pref to enable it. But I think I'm probably over-thinking it :-)

Comment 17

[Tom Tung [:tt, :ttung]]

(In reply to Tom Tung [:tt, :ttung] from comment #14)

Lars, does the alternative idea look good to you? Or, do you have another suggestion? I can create a patch for that quickly.

When I wrote this, I was thinking I can check the pref of COOP and COEP value right before warning. But, it seems it doesn't apply to the current design (I suspect that increasing dependence for the gecko to js/src/ doesn't seem to be a good idea).

Thus, I will try to pass the warning message on CloneDataPolicy to bypass this issue.

Comment 18

[Tom Tung [:tt, :ttung]]

Attachment: Bug 1587007 - P1 - Introduce a boolean on CloneDataPolicy to tunnel the information from dom to js;

Comment 19

[Tom Tung [:tt, :ttung]]

Attachment: Bug 1587007 - P2 - Having two different error message when the prefs for COOP and COEP are enabled or not;

Depends on D50965

Comment 20

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

I'm guessing all of this is being done because the JS engine isn't allowed to read prefs. IIRC there are already a bunch of prefs which the JS engine reads through some other ambient mechanism. I'm not a big fan of having this information on the CloneDataPolicy object.

ni? :tcampbell because he probably knows the cleanest way to make something like this happen.

Comment 21

[Tom Tung [:tt, :ttung]]

Attachment: Bug 1587007 - P1 - Tunnel the prefs for Coop and Coep to js;

Comment 22

[Anne (:annevk)]

We discussed this a bit more and decided that the improved error message (that is riding the trains) is good enough for the Nightly pref flip happening in bug 1594748. We should have better error messages before bug 1477743 is over though and consider fixing this before we enable everything on early beta or earlier.

Comment 23

[Tom Tung [:tt, :ttung]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=7af8b79777a45b0964cba6bfc7a6d9e7e04ee68e

Comment 24

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/d3f5489f0bb6
P1 - Tunnel the prefs for Coop and Coep to js; r=jandem
https://hg.mozilla.org/integration/autoland/rev/db3e7ef5babd
P2 - Having two different error message when the prefs for COOP and COEP are enabled or not; r=jandem

Comment 25

[Andrei Ciure[:aciure]]

https://hg.mozilla.org/mozilla-central/rev/d3f5489f0bb6
https://hg.mozilla.org/mozilla-central/rev/db3e7ef5babd