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

RESOLVED FIXED in Firefox 55

Status

()

enhancement
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: anba, Assigned: anba)

Tracking

unspecified
mozilla55
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox55 fixed)

Details

Attachments

(1 attachment, 1 obsolete attachment)

Assignee

Description

2 years ago
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().
Assignee

Comment 1

2 years ago
Posted patch bug1368963.patch (obsolete) — Splinter Review
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.
Assignee: nobody → andrebargull
Status: NEW → ASSIGNED
Attachment #8873787 - Flags: review?(jdemooij)
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.
Attachment #8873787 - Flags: review?(jdemooij) → review+
(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.
Assignee

Comment 4

2 years ago
(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!
Needs rebasing.
Keywords: checkin-needed
Assignee

Comment 7

2 years ago
Rebased patch for checkin.
Attachment #8873787 - Attachment is obsolete: true
Attachment #8875262 - Flags: review+

Comment 9

2 years ago
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/48078fb0fcc2
Avoid extra calls to GetPropertyKeys() in Object.freeze/seal/preventExtensions. r=jandem
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/48078fb0fcc2
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
Depends on: 1394523
Depends on: 1390856
You need to log in before you can comment on or make changes to this bug.