Closed Bug 1368963 Opened 3 years ago Closed 3 years ago

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

Categories

(Core :: JavaScript Engine, enhancement)

enhancement
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla55
Tracking Status
firefox55 --- fixed

People

(Reporter: anba, Assigned: anba)

References

Details

Attachments

(1 file, 1 obsolete file)

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().
Attached 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.
(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
Attached patch bug1368963.patchSplinter Review
Rebased patch for checkin.
Attachment #8873787 - Attachment is obsolete: true
Attachment #8875262 - Flags: review+
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: 3 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.