Closed Bug 1587007 Opened 4 months ago Closed 2 months ago

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

Categories

(Core :: JavaScript Engine, enhancement, P3)

enhancement

Tracking

()

RESOLVED FIXED
mozilla72
Tracking Status
firefox72 --- fixed

People

(Reporter: lth, Assigned: tt)

References

(Blocks 1 open bug)

Details

Attachments

(4 files, 2 obsolete files)

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.

See Also: → 1562667

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

Assignee: lhansen → ttung
Attached image Screen Shot 2019-10-21 at 11.29.11.png (obsolete) —

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

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

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

Newer error message

Attachment #9102907 - Attachment is obsolete: true

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

Flags: needinfo?(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)

Flags: needinfo?(ttung) → needinfo?(annevk)

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.

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.

Flags: needinfo?(annevk)
Pushed by ttung@mozilla.com:
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

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)

Keywords: leave-open

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

Flags: needinfo?(lhansen)

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

Flags: needinfo?(lhansen)

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

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.

Flags: needinfo?(tcampbell)
Attachment #9104958 - Attachment is obsolete: true

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.

Blocks: resab
No longer blocks: 1594748
Flags: needinfo?(tcampbell)
Keywords: leave-open
Pushed by ttung@mozilla.com:
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
Status: ASSIGNED → RESOLVED
Closed: 2 months ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla72
You need to log in before you can comment on or make changes to this bug.