User Agent: Mozilla/5.0 (X11; Linux x86_64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/74.0.3729.169 Safari/537.36

Steps to reproduce:

HTML Standard change: https://github.com/whatwg/html/pull/4665.
WebIDL Standard change: https://github.com/heycam/webidl/pull/732

Tests: https://github.com/web-platform-tests/wpt/pull/17095.

Hello! I've taken the time to investigate & implement this, please see my attached diff as a first pass. I have a couple notes / questions, all feedback is appreciated!

1. I'm not currently serializing the .stack property. I had attempted to do this as a serialized savedFrame, bu ran into issues during deserialization. Recommendations on an approach for this would be appreciated!

2. As indicated in the spec, the Error type being serialized is selected based on prototype, so ErrorObject.value() cannot be used directly. I created GetSerializedExceptionType() to iterate through Error prototypes and convert to the corresponding JSExnType per the spec. If there's a better way to test the Error prototype than what I've done here, I'd appreciate any input -- There was no immediately obvious better way to do this.

3. It seems to me that we can't currently create ErrorObjects without fileName, sourceId, lineNumber or columnNumber. Given this, I have interpreted the following from the spec to include those fields:

User agents should attach a serialized representation of any interesting accompanying data which are not yet specified

4. Testing: What should be covered in jsapi tests vs "non262" vs wpt ? I had written some ad-hoc tests locally during development and left in a simple one. I'm curious what the correct approach is, considering overlap with the wpt tests?

Thank you!

I'll aim to look on Monday. Thank you for working on this!

I'm not currently serializing the .stack property

That's OK for now, but please file a followup on that? We should figure out how we want to do this... I do wonder whether some of the other deserialization issues I think I ran into while reviewing might have been an issue there.

If there's a better way to test the Error prototype than what I've done here, I'd appreciate any input

That's really a question for a SpiderMonkey peer. I asked Jeff to review the patch too.

Given this, I have interpreted the following from the spec to include those fields:

Yes, we should include them.

What should be covered in jsapi tests vs "non262" vs wpt ?

That's a good question. My gut feeling is that if it needs the web platform to test in general (i.e. it can't be a pure tes262 test), it should likely go in wpt. But again, this is largely a question for a SpiderMonkey peer.

Note that we are tweaking the semantics slightly at https://github.com/whatwg/html/pull/5150. I will work on updating the web platform tests (and will edit that HTML pull request to point to them when they're available.)

This seems to have stalled. I looked at the patch and it seems like a generally good implementation. However as mentioned by Domenic the spec changed. We definitely should be relying on GetPropertyPure, because that method doesn't implement any specified semantics.

I started looking into this a little bit and I think we won't be able to follow the spec closely.

• First we should wait on https://github.com/whatwg/html/pull/5749. Having to errors on AggregateError is a relatively big change at least for us. We would need to do something like we do for Map or SavedFrame to handle nested objects.
• We have quite a few custom properties like lineNumber etc that should also be cloned. Otherwise the created error object is going to look weird.
• More important we should clone stack. I think in that case it would have to clone the SavedFrame as well.

There are various complication to this patch and I suspect this approach won't actually work. This
all resolves around the problem of not being able to access principals on Workers.

1. Handling sending SavedFrames from content to a Worker.

The stack for an Error object, represented by SavedFrames, created by a normal web can contain
frames with many different principals (think chrome calling content). This sort of complicated
principal handling isn't possible in worker threads.

To worker around this problem we only send SavedFrames that are subsumed by the current realm principal.
This is the same result as doing |(new Error).stack|, which can already be send to a worker anyway.

On the worker side we we will just use worker's current running principal.

2. Sending SavedFrames from the Worker to content

We already seem to have something that is able to handle this use case, namely AutoSetActiveWorkerPrincipal.
But because SavedFrames could only appear in a limited number of places before we were missing some callers.
I am probably still missing some.

Problems/Questions:

• Can we now suddenly try to persist (i.e. indexedDB) SavedFrames with the runtime-only SCTAG_DOM_WORKER_PRINCIPAL tag?
• Can we run Workers on non-worker threads?
• Does this work with ServiceWorkers / SharedWorkers?
• What should happen when sending from Worker to Worker. Currently this just changes the principal to the target Workers' principal.

Depends on D120116

Hi Simon! Could you take a look at the approach https://phabricator.services.mozilla.com/D120326? I am curious what your thoughts are on how to support sending principals to workers.

I'm probably a better contact for this. (:sg no longer works on Workers & Storage) I need to take a look more deeply to understand the spec, use-cases involved, and our current behavior but do not have time at the immediate moment.

(In reply to Tom Schuster [:evilpie] from comment #9)

Problems/Questions:

• Can we now suddenly try to persist (i.e. indexedDB) SavedFrames with the runtime-only SCTAG_DOM_WORKER_PRINCIPAL tag?

It does seem like structured serialization of errors is allowed with forStorage=true, but I need to understand when we are sending principal tags over the wire.

• Can we run Workers on non-worker threads?

No, never. Note, however, that in this space, devtools is allowed to run a global on the worker thread, however, that runs in a privileged position that is able to reach into the worker content global.

• Does this work with ServiceWorkers / SharedWorkers?

Again, need to understand the nuances more here, but in general the only semantic and implementation differences between normal Dedicated Workers and ServiceWorkers and SharedWorkers are that the 1) latter exist in their own agent clusters (per spec), 2) they don't have the same-process/same-agent-cluster structured-serialization dataflow from Worker.postMessage, and 3) parent relationships (Dedicated workers have windows or workers for parents, ServiceWorkers and SharedWorkers are always owned by the main thread currently (will change) but don't semantically have a parent).

(In reply to Andrew Sutherland [:asuth] (he/him) from comment #11)

• Can we run Workers on non-worker threads?

No, never. Note, however, that in this space, devtools is allowed to run a global on the worker thread, however, that runs in a privileged position that is able to reach into the worker content global.

A big asterisk here is probably worklets.

Perhaps a different WorkerPrincipal::write() implementation could make sending principals from workers just work without AutoSetActiveWorkerPrincipal. (This doesn't help with receiving foreign principals in Workers.)

(In reply to Andrew Sutherland [:asuth] (he/him) from comment #12)

A big asterisk here is probably worklets.

Worklets also always run script in their own thread, without any other kind of JSContext on the same thread.

Thanks for taking a look.

It does seem like structured serialization of errors is allowed with forStorage=true, but I need to understand when we are sending principal tags over the wire.

I think we can detect this case in WorkerPrincipal::write and just disallow it, that is going to cause some surprises when trying to use IndexedDB from a Worker ... Not sure how to fix this case for real without having access to the real content principal in the Worker.

Perhaps a different WorkerPrincipal::write() implementation could make sending principals from workers just work without >AutoSetActiveWorkerPrincipal. (This doesn't help with receiving foreign principals in Workers.)

While AutoSetActiveWorkerPrincipal seems like a somewhat hacky approach, at least that part already works :)

I guess I am just going to wait for some more feedback.

Because it seems unlikely that support for cloning principals is going to happen
anytime soon we now require an opt-in. This means errors will be cloned without
their stack.

Depends on D120116

Hi Jan! With this patch stack we will now structure clone Error objects. That means they can be potentially persisted in indexedDB. (I intentionally did not allow the saving of SavedFrames, i.e. what is required for Error.stack, because that would mean writing principals to disk)

Anyway. I am running into reproducible issue with the test toolkit/components/extensions/test/xpcshell/test_ext_storage_idb_data_migration.js. It seems like the issue is related to indexedDB somehow. Luca kindly helped me with some debugging here and came up with the follwing:

It seems that the key cursor created internally ExtensionStorageIDB isEmpty is returning a key but at a first glance it looks like garbage data, in BackgroundCursorChild<CursorType>::RecvResponse at line 3472 (https://searchfox.org/mozilla-central/rev/d4b9c457db637fde655592d9e2048939b7ab2854/dom/indexedDB/ActorsChild.cpp#3472) inspecting aResponse.get_ArrayOfObjectStoreKeyCursorResponse() does show the following:

(rr) p aResponse.get_ArrayOfObjectStoreKeyCursorResponse()
$7 = nsTArray<mozilla::dom::indexedDB::ObjectStoreKeyCursorResponse> & = {{key_ = {mBuffer = "0gblfjowbmjelfz", static kMaxArrayCollapse = 3 '\003', static kMaxRecursionDepth = 64 '@'}}}

Hm, I would need to take a closer look. All I have to do is apply those 3 patches and run:

mach test toolkit/components/extensions/test/xpcshell/test_ext_storage_idb_data_migration.js

?

I just marked the one patch as obsolete, so only the two remaining patches are required. And that mach invocation should be enough.

Depends on D136267

Steve, I wonder what you think of changing the SavedFrame cloning as well? The main idea here is that making it not depend on the ChildCounter / "fake" propties, we can more easily re-use the code for DOMExceptions.

(In reply to Tom Schuster [:evilpie] from comment #20)

Steve, I wonder what you think of changing the SavedFrame cloning as well? The main idea here is that making it not depend on the ChildCounter / "fake" propties, we can more easily re-use the code for DOMExceptions.

I like your new SavedFrame cloning approach much better.

Oh, I had totally forgotten about that indexedDB failures. Sadly it still seems to be happening. D136788 and D120116 need to be applied in that order for it to reproduce.

(In reply to Tom Schuster [:evilpie] from comment #23)

Oh, I had totally forgotten about that indexedDB failures. Sadly it still seems to be happening. D136788 and D120116 need to be applied in that order for it to reproduce.

D'uh. The failure was actually super obvious when looking at the test in more detail and easy to fix.

  // Store a fake invalid value which is going to fail to be saved into IndexedDB
  // (because it can't be cloned and it is going to raise a DataCloneError), which
  // will trigger a data migration failure that we expect to increment the related
  // telemetry histogram.
  jsonFile.data.set("fake_invalid_key", new Error());

At this point I am convinced that not changing the SavedFrame code is the best way forward.

• We need to maintain the SavedFrame code for backwards compat.
• We already use the ChildCounter anyway for errors and cause.
• DOM Exception ended up landing before this without stack cloning support.

Hi Tom, I think I found a better way forward for this stuff. I put up my patch stack for you to look at. The 3rd patch is still owned by you in phabricator, so either I can mark it as reviewed when you're ok with it, or I can commandeer and you can mark it as reviewed. (I believe you will maintain authorship either way.)

What's different: first, I replaced the ChildCounter mechanism with a simpler setup where I keep a parallel but sparse objState stack alongside the objs stack. It's sparse in that I only push/pop for objects that need it (initially SavedFrame, later Error as well). The basic idea is that any class that has a known, fixed set of fields that can be Objects or similar (anything requiring recursion) will push to it. In order to make it sparse, I also push the object that it applies to. To check whether an object has state, you compare the top objects on the two stacks.

As it turns out, the only state ever needed is a single "finished" flag, which I only recently realized. So an unconditional bit vector would work too; it wouldn't use enough space to worry about. In theory, you could store more complex state here (it's the equivalent of making read call itself recursively, which would save away state in the C stack frame. Here, we're using explicit stacks, so you need a stack for the state as well.)

The other difference is that Error objects now do startWrite(cause); startWrite(errors); startWrite(stack) while writing, and correspondingly do startRead; startRead; startRead when reading. That means it's pushing the headers of those 3 objects (or null) one after another, then recursing exactly as if they were regular Object properties. There will be an SCTAG_END_OF_KEYS (oops, I meant to rename that to SCTAG_END_OF_FIELDS, maybe I'll still do that) after the "recursive data" for each object. I'm probably not making sense. Anyway, the startRead calls read in enough to construct a possibly-incomplete object which is immediately stored in the containing Error object.

Because SavedFrame has exactly one field (the parent), this turns out to be exactly the same storage format as it was before, so there are no backwards compatibility problems. Phew! The code for reading is slightly different, but unless I confused myself (again), there's nothing different necessary for reading saved data. (Then again, I have a couple of jobs on try that are telling me I might have missed something...)

Please take a look and tell me what you think.

https://hg.mozilla.org/mozilla-central/rev/d8aa1c42b5b7
https://hg.mozilla.org/mozilla-central/rev/f59ef8ec67fe
https://hg.mozilla.org/mozilla-central/rev/34cf3bd84994

FF103 docs work for this can be tracked in https://github.com/mdn/content/issues/18208.

As I understand it, this delivers support for structured cloning of standard error types. What I think needs to happen:

1. Update to all the error types browser compatibility information to mark handling as supported in FF103 - i.e. see the line "RangeError is serializable" RangeError
2. For each case add some text like "XXXError is a Serializable object, so it can be cloned with structuredClone() or copied between Workers using postMessage()."
3. Add the errors to the list of types that can be used with the structured clone algorithm here: https://developer.mozilla.org/en-US/docs/Web/API/Web_Workers_API/Structured_clone_algorithm#supported_types

Some questions

1. This appears to work with all the types listed here: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Error#error_types (looking at the test code). Are we missing any?
2. This appears to serialise the name and the message - is there anything else it must serialize? For example, does it include the error.cause?
3. This appears to serialize the error stack - correct? This is optional right? I'm a little confused by the fact that the stack seems to be both non-standard and implemented by everyone: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Error/Stack
4. Is there anything we need to say about things that might not serialize?
5. Presumably this will work for derived/custom error classes derived from Error? Anything people need to be aware of.

Thanks!

(In reply to Hamish Willee from comment #32)

FF103 docs work for this can be tracked in https://github.com/mdn/content/issues/18208.
Some questions

1. This appears to work with all the types listed here: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Error#error_types (looking at the test code). Are we missing any?

I don't think so. InternalError isn't cloneable. AggregateError is cloneablee in Firefox, but not yet part of the spec https://github.com/whatwg/html/pull/5749.

2. This appears to serialise the name and the message - is there anything else it must serialize? For example, does it include the error.cause?

Must, no. The name must be one of the known Error classes like "TypeError" and can't be arbitrary.

We also serialize the cause property for all errors and errors on AggregateError. This is also waiting on the specification PR above. We also support serializing the non-standard fileName, lineNumber and columnNumber properties.

The current spec has the following comment btw:

User agents should attach a serialized representation of any interesting accompanying data which are not yet specified, notably the stack property, to serialized.

https://html.spec.whatwg.org/multipage/structured-data.html#structuredserializeinternal

3. This appears to serialize the error stack - correct? This is optional right? I'm a little confused by the fact that the stack seems to be both non-standard and implemented by everyone: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Error/Stack

We only support serializing the stack when using the structuredClone API in Nightly at the time of writing this. You want to follow bug 1774866. It's indeed not specified, but supported in every browser. The stack is actually not the same across browsers.

4. Is there anything we need to say about things that might not serialize?
5. Presumably this will work for derived/custom error classes derived from Error? Anything people need to be aware of.

It does work with derived classes that (e.g. class MyError extends Error {}). After cloning it just looks like a normal Error object. Custom properties are lost, like in almost all other cases e.g. when subclassing Map.