1368963 - Can we remove the GetPropertyKeys call in SetIntegrityLevel when we iterate over shapes?

Status: RESOLVED FIXED

(Core :: JavaScript Engine, enhancement)

Description

[André Bargull [:anba]]

SetIntegrityLevel() calls GetPropertyKeys() to retrieve the property keys, but the keys aren't used when we iterate over the properties using the shapes. The GetPropertyKeys() call is also not necessary to resolve lazy properties, because PreventExtensions() also calls GetPropertyKeys() to resolve lazy properties.

So it looks like to me, we could move the GetPropertyKeys() call into the else-branch in SetIntegrityLevel().

Comment 1

[André Bargull [:anba]]

Attachment: bug1368963.patch

Changes PreventExtensions to directly use getClass()->getEnumerate() to create lazy properties instead of doing this indirectly through GetPropertyKeys(). In case obj->getOpsEnumerate() is defined and the JSNewEnumerateOp hook is allowed to create properties, this patch needs to be updated to handle this case. Do you know if JSNewEnumerateOp is allowed to this kind of side-effect?

Similarly update SetIntegrityLevel to only call GetPropertyKeys() when we actually use the keys to iterate over the properties. And furthermore use a vector with a bit of inline-storage, so we don't waste time in malloc when freezing an object with non-zero properties.

Comment 2

[Jan de Mooij [:jandem]]

Comment on attachment 8873787 [details] [diff] [review]
bug1368963.patch

Review of attachment 8873787 [details] [diff] [review]:
-----------------------------------------------------------------

Great!

::: js/src/jsobj.cpp
@@ +2767,5 @@
>      // element can be added without a call to isExtensible, at the cost of
>      // performance.
>      if (obj->isNative()) {
> +        // Force lazy properties to be resolved.
> +        if (JSEnumerateOp enumerate = obj->getClass()->getEnumerate()) {

I think this is okay. The newEnumerate hook on window can resolve properties but that's not an issue because Object.preventExtensions(window) throws an exception.

Once we convert more code to the new hook we should probably call newEnumerate here and then resolve all properties it returns. We can cross that bridge when we get there though.

Comment 3

[Jan de Mooij [:jandem]]

(In reply to Jan de Mooij [:jandem] from comment #2)
> Once we convert more code to the new hook we should probably call
> newEnumerate here and then resolve all properties it returns. We can cross
> that bridge when we get there though.

I can do that as part of bug 1370608. I hope we can change JS_EnumerateStandardClasses to JS_NewEnumerateStandardClasses but Object.freeze(this) still has to work in the shell, so we will have to add the "resolve lazy props" logic for that.

Comment 4

[André Bargull [:anba]]

(In reply to Jan de Mooij [:jandem] from comment #3)
> (In reply to Jan de Mooij [:jandem] from comment #2)
> > Once we convert more code to the new hook we should probably call
> > newEnumerate here and then resolve all properties it returns. We can cross
> > that bridge when we get there though.
> 
> I can do that as part of bug 1370608. I hope we can change
> JS_EnumerateStandardClasses to JS_NewEnumerateStandardClasses but
> Object.freeze(this) still has to work in the shell, so we will have to add
> the "resolve lazy props" logic for that.

Okay, great!

Comment 5

[André Bargull [:anba]]

Try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=5c2f4711c9d7c5e898216a8a1a893dded9170aa5

Comment 6

[Ryan VanderMeulen [:RyanVM]]

Needs rebasing.

Comment 7

[André Bargull [:anba]]

Attachment: bug1368963.patch

Rebased patch for checkin.

Comment 8

[André Bargull [:anba]]

New Try just to be sure: https://treeherder.mozilla.org/#/jobs?repo=try&revision=0852e59493a1fcd73f9c80229f633f3770caebe9

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/48078fb0fcc2
Avoid extra calls to GetPropertyKeys() in Object.freeze/seal/preventExtensions. r=jandem

Comment 10

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/48078fb0fcc2