1691941 - Hide the rest-array in default derived class constructors

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect, P1)

Description

[André Bargull [:anba]]

The debugger can cause havoc when it modifies the rest-array in default derived class constructors: https://phabricator.services.mozilla.com/D104406#3397378

It seems like we can avoid this issue when we change the argument name from args to .args, which makes it impossible for the debugger to access the parameter.

Comment 1

[André Bargull [:anba]]

Attachment: Bug 1691941: Hide the rest-array in default derived class constructors. r=tcampbell!

Hide the rest-array in default derived class constructors, so the debugger can't
modify it.

Comment 2

[Ted Campbell [:tcampbell]]

This seems quite sensible.

In the old model with self-hosting, would that have prevented debugger from getting involved?

Comment 3

[André Bargull [:anba]]

Unless I'm misunderstanding your question:
Before bug 1681567, the iteration protocol when evaluating super(...args) would have handled this case gracefully (either by throwing a TypeError when effectively evaluating super(...null) or using normal array iteration when evaluating super(...[/* hole */, 1])). Only after bug 1681567 we have to worry about modifications of the args binding or the args array itself.

Comment 4

[Ted Campbell [:tcampbell]]

Can the debugger break in self-hosting code and mutate variables? If not, would extending the filter there to also exclude synthetic constructors solve this?

(I still think what you have is a better answer. Simply trying to understand the problem space)

Comment 5

[André Bargull [:anba]]

I don't think the debugger can modify variables in self-hosting code. But the old, self-hosted default constructors were changed to non-selfhosted functions when exposed to the user. And then it was possible to change any variables.

(This is all probably controlled in Debugger::observesScript, which hides self-hosted scripts from the debuggger.)

Comment 6

[BugBot [:suhaib / :marco/ :calixte]]

Set release status flags based on info from the regressing bug 1681567

Comment 7

[André Bargull [:anba]]

Using .args makes devtools unhappy:

JavaScript error: resource://devtools/shared/protocol/Front.js, line 361: Error: Protocol error (TypeError): name is not an identifier from: server0.conn0.child3/frame95 (resource://devtools/server/actors/environment.js:108:30)

The error results in only showing "Loading..." under the "Scopes" panel.

So I guess we should instead go with your idea and hide the synthetic constructors.

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/8bd9afc1c9eb
Hide the rest-array in default derived class constructors. r=tcampbell

Comment 9

[Alexandru Michis [:malexandru]]

https://hg.mozilla.org/mozilla-central/rev/8bd9afc1c9eb

Comment 10

[BugBot [:suhaib / :marco/ :calixte]]

The patch landed in nightly and beta is affected.
:anba, is this bug important enough to require an uplift?
If not please set status_beta to wontfix.

For more information, please visit auto_nag documentation.

Comment 11

[André Bargull [:anba]]

No, it's not important enough for an uplift, because the bug can only be triggered through the debugger.