src: use proper errors as coming from StringBytes

[addaleax]

The previous errors were incorrect here, as the code
only failed in situations where strings exceeded size limits or
an OOM situation was encountered, not for invalid encodings
(which aren’t even detected explicitly).

Unfortunately, these situations are hard to test for.

Checklist

• make -j4 test (UNIX), or vcbuild test (Windows) passes
• commit message follows commit guidelines

Affected core subsystem(s)

src

[bnoordhuis]

return env->isolate()->ThrowException(error);? The call to rc.ToLocalChecked() two lines down will fail if you fall through.

Same issue appears a few more times further down in this file.

[addaleax]

done

[bnoordhuis]

Stylistic change.

[addaleax]

fixed up, this was not intentional

[bnoordhuis]

Suggestion: move the .IsEmpty() checks to after the StringBytes::Encode() calls, then you don't have to do the goto jump-around thing.

[addaleax]

error is cleared by StringBytes::Encode() if the call succeeded… I’ve thought about doing that, would you prefer to change that behaviour? I can see the advantage that would have.

[bnoordhuis]

Good idea. I was thinking of guarding the Encode() calls with an error.IsEmpty() check but that's a good idea.

[addaleax]

@bnoordhuis Done, PTAL!

[addaleax]

@bnoordhuis Mind taking another look?

[bnoordhuis]

Mildly unrelated change.

[addaleax]

CI: https://ci.nodejs.org/job/node-test-commit/11985/

This should be ready.

[jasnell]

@addaleax ... fyi https://ci.nodejs.org/job/node-test-commit-aix/8108/nodes=aix61-ppc64/console

Can you check to see if this failure is related to the change?

[addaleax]

Ugh … a lot of the Linux failures are the same, so, probably? test-commit-linux on master for comparison: https://ci.nodejs.org/job/node-test-commit-linux/12023/

[addaleax]

Sigh. Yes, also a problem on master, so I’ll take a look.

[jasnell]

I'm digging in also. :-) This wasn't a problem earlier today so it has to be one of the PRs landed today.... and I can't get to Jenkins... sigh. Will keep trying

[addaleax]

@jasnell I think it’s only 32-bit systems (?) My guess would be

node/src/node_perf.cc

Lines 318 to 327 in c6da5c8

	#define SET_STATE_TYPEDARRAY(name, type, field) \
	target->Set(context, \
	FIXED_ONE_BYTE_STRING(isolate, (name)), \
	type::New(state_ab, \
	offsetof(performance_state, field), \
	arraysize(state->field))) \
	.FromJust()
	SET_STATE_TYPEDARRAY("observerCounts", v8::Uint32Array, observers);
	SET_STATE_TYPEDARRAY("milestones", v8::Float64Array, milestones);
	#undef SET_STATE_TYPEDARRAY

I realize that’s kind of my code because it mirrors the HTTP2 one, so … uuh. 😄

[addaleax]

Heh, okay, I think I got this.

[jasnell]

Um.. that's confusing then :-/ ... the CI was good for that PR even on 32 bit system.

What do you have?

[addaleax]

Landed in 784c6d4

(for the record, the above conversation was finished in #14996)