fix: panic, unwrap to unwrap_or

[peko-thunder]

I created a PR in deno_core to correct the issue.

#744

Sometimes panic occurs with unwrap().
So I set the default value.

Do I need to write test code?
I'm reading the code right now, it's going to take some time.

[CLAassistant]

All committers have signed the CLA.

[ry]

Thanks! Could you add a test? Maybe use unwrap_or_else instead?

[peko-thunder]

@ry
I will add a test to integration_test in testing/lib.rs.
Test that there is no panic when executing code like issue.

[peko-thunder]

@ry
I added an integration test.
I don't use unwrap_or_else(), is that a problem?

[bartlomieju]

@peko-thunder can you please run cargo fmt on your branch?

[0f-0b]

Instead of concealing the bug like this, can we fix ArrayPrototypePush so that it does not accidentally call user code?

[bartlomieju]

@0f-0b can you elaborate more on the suggested solution?

[peko-thunder]

@bartlomieju
I’m sorry. I’ll be careful next time.

[0f-0b]

can you elaborate more on the suggested solution?

The ultimate cause of this bug is that Array.prototype.push calls the [[Set]] internal method on the receiver (step 5.a) which executes the user-defined setter, and that ArrayPrototypePush is currently just uncurryThis(Array.prototype.push). My suggestion is to change ArrayPrototypePush to a custom implementation that calls [[DefineOwnProperty]] to avoid executing user code. A precedent of providing a custom implemention to avoid unintentional side-effects is ArrayPrototypeToString.

[bartlomieju]

@devsnek what do you think of the approach above?

[devsnek]

assuming you are referring to this

deno_core/core/02_error.js

Line 172 in 87e5ab2

ArrayPrototypePush(error.__callSiteEvals, callSite);

i think applying both approaches make sense. runtime internal js should have a way to push an element to an array without the prototype breaking things, and runtime internal rust should probably be able to cope with weird things happening in js without crashing, unless the weird thing is truly a case where the runtime cannot safely continue execution.

[peko-thunder]

Thanks for the suggestion.
I may not understand it yet, but I confirmed that if I handle the push() pseudo, I won't get the error.
I'll do some more testing.

  primordials.ArrayPrototypePush = (thisArray, ...variableArgs) =>
    thisArray = [...thisArray, ...variableArgs];

[peko-thunder]

Confirmed that all tests pass.
If you see a problem, point it out.

  primordials.ArrayPrototypePush = (thisArray, ...variableArgs) => {
    const args = [...variableArgs];
    for (let i = 0; i < args.length; i++) {
      ObjectDefineProperty(thisArray, thisArray.length, {
        value: args[i],
        enumerable: true,
        writable: true,
        configurable: true,
      });
    }
    return thisArray.length;
  };

[0f-0b]

The panic may still happen if user code overwrites __callSiteEvals. I will investigate if it is possible to hide __callSiteEvals from user code later.

throw Object.defineProperty(new Error(), "__callSiteEvals", { value: [0] });

ObjectDefineProperty is really slow, but we can always replace ArrayPrototypePush(array, value) with array[array.length] = value in affected code. The latter is not only much faster, but also makes it explicit that [[Set]] is called.

[devsnek]

The panic may still happen if user code overwrites __callSiteEvals.

Yeah I would suggest keeping the rust unwrap_or in this PR as well.

I will investigate if it is possible to hide __callSiteEvals from user code later.

For hiding __callSiteEvals I think you could replace it with a WeakMap that is shared with the rust code. Using a v8 private symbol is a fun trick as well but it requires a transmute :(

[0f-0b]

Why wasn't v8::Exception::get_stack_trace used here?

Edit: I see that a lot of useful information is only available to the prepareStackTrace callback. Was this the only reason?

[bartlomieju]

Given the fix in #754, do we really want to change this?

@0f-0b @devsnek