Skip to content

util: fix inspection of module namespaces #20962

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
devsnek wants to merge 1 commit into from
Closed

util: fix inspection of module namespaces #20962

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
20 changes: 18 additions & 2 deletions lib/util.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -478,7 +478,23 @@ function formatValue(ctx, value, recurseTimes) {
if (ctx.showHidden) {
keys = Object.getOwnPropertyNames(value);
} else {
keys = Object.keys(value);
// This might throw if `value` is a Module Namespace Object from an
// unevaluated module, but we don't want to perform the actual type
// check because it's expensive.

This comment was marked as resolved.

Sign in to view

// TODO(devsnek): track https://github.com/tc39/ecma262/issues/1209
// and modify this logic as needed.
try {
Copy link
Member

@jdalton jdalton May 29, 2018 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  • Object.keys and Object.getOwnPropertyNames references should be cherry-picked above.
  • The instanceof ReferenceError check will fail from errors of a different realm (context).

Copy link
Member Author

@devsnek devsnek May 29, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

for the first point, we purposely leave those as-is because some polyfills for things use them to mess with util.

for the second point, i guess types.isNativeError(err) && err.name === 'ReferenceError' would work?

Copy link
Member

@jdalton jdalton May 29, 2018 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

for the first point, we purposely leave those as-is because some polyfills for things use them to mess with util.

Ah cool! Do you have any examples at hand?

for the second point, i guess types.isNativeError(err) && err.name === 'ReferenceError' would work?

Yep.

Copy link
Member Author

@devsnek devsnek May 29, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't know what package does it, but it was brought up during discussions about hardening core against tampering with builtin prototypes.

keys = Object.keys(value);
} catch (err) {
if (types.isNativeError(err) &&
err.name === 'ReferenceError' &&
types.isModuleNamespaceObject(value)) {
keys = Object.getOwnPropertyNames(value);
} else {
throw err;
}
}

if (symbols.length !== 0)
symbols = symbols.filter((key) => propertyIsEnumerable.call(value, key));
}
Expand Down Expand Up @@ -772,7 +788,7 @@ function formatNamespaceObject(ctx, value, recurseTimes, keys) {
try {
output[i] = formatProperty(ctx, value, recurseTimes, keys[i], 0);
} catch (err) {
if (!(err instanceof ReferenceError)) {
if (!(types.isNativeError(err) && err.name === 'ReferenceError')) {
throw err;
Copy link
Member

@jdalton jdalton May 29, 2018 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could the error checks be simplified all up? If namespace objects can only error by throwing a reference error then we can assume that the error accessing it is because of the reference error and remove the error checks and rethrow code above. We could also simplify the initial check by doing:

try {
  keys = Object.keys(value);
} catch (err) {
  if (types.isModuleNamespaceObject(value)) {
    keys = Object.getOwnPropertyNames(value);
  } else {
    throw err;
  }
}

Here erroring should be rare and when it does error it is likely a namespace object so the check up-front on error should be fine.

Copy link
Member Author

@devsnek devsnek May 30, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i think its best to explicitly check if its a reference error, do you feel strongly about this?

Copy link
Member

@jdalton jdalton May 30, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I dig it if code could be simpler, and still correct, that it be simpler is all.

}
// Use the existing functionality. This makes sure the indentation and
Expand Down