Bug 1608760 Comment 18 Edit History

Note: The actual edited comment in the bug view page will always show the original commenter’s name and original timestamp.

(In reply to Boris Zbarsky [:bzbarsky, bz on IRC] from comment #5)
> Now what's unclear to me is whether `JS::CreateError` should handle a null filename (like it handles a null message, if you dig down into things, at the level of `js::ErrorObject::init`) or not.  Jason, any opinions?

It should not.

There are two problem to fix here:

1. something is producing invalid serialized data somehow
2. deserialization must either accept or reject all serialized data, not crash

To fix problem 2, we should probably take mccr8's first patch. A more principled patch would treat that condition as invalid serialized input and refuse to create an Error.

On to problem 1:

> If it should, then we should fix that.  If it should not, we need to figure out what to do in `ClonedErrorHolder::ToErrorValue` if `mFilename` is void.  Which is its default state (see `ClonedErrorHolder::ClonedErrorHolder`) until either `ClonedErrorHolder::Init` happens with a non-null `err->filename` or we do a `ReadStructuredClone`.
>
> Now in this case, we have `ClonedErrorHolder::ReadStructuredClone` on the stack, so presumably the thing we serialized on the other end had a null filename for some reason... Presumably because _it_ was initialized from a `JSErrorReport` with a null filename?

We could add a release assertion on initialization and try to catch this closer to the source.
(In reply to Boris Zbarsky [:bzbarsky, bz on IRC] from comment #5)
> Now what's unclear to me is whether `JS::CreateError` should handle a null filename (like it handles a null message, if you dig down into things, at the level of `js::ErrorObject::init`) or not.  Jason, any opinions?

It should not.

There are two problems to fix here:

1. something is producing invalid serialized data somehow
2. deserialization must either accept or reject all serialized data, not crash

To fix problem 2, we should probably take mccr8's first patch. A more principled patch would treat that condition as invalid serialized input and refuse to create an Error.

On to problem 1:

> If it should, then we should fix that.  If it should not, we need to figure out what to do in `ClonedErrorHolder::ToErrorValue` if `mFilename` is void.  Which is its default state (see `ClonedErrorHolder::ClonedErrorHolder`) until either `ClonedErrorHolder::Init` happens with a non-null `err->filename` or we do a `ReadStructuredClone`.
>
> Now in this case, we have `ClonedErrorHolder::ReadStructuredClone` on the stack, so presumably the thing we serialized on the other end had a null filename for some reason... Presumably because _it_ was initialized from a `JSErrorReport` with a null filename?

We could add a release assertion on initialization and try to catch this closer to the source.

Back to Bug 1608760 Comment 18