Bug 222739 – Cache cross-origin methods / accessors of Window and Location per lexical global object

• Component: DOM
• Status: RESOLVED FIXED

[Alexey Shvayka]

Cache cross-origin methods / accessors of Window and Location per lexical global object

[Alexey Shvayka]

Created attachment 422227 [details]
Patch

[EWS Watchlist]

This patch modifies the imported WPT tests. Please ensure that any changes on the tests (not coming from a WPT import) are exported to WPT. Please see https://trac.webkit.org/wiki/WPTExportProcess

[Alexey Shvayka]

Created attachment 422286 [details]
Patch

Fix & simplify CrossOriginMapKey, and include missing test file.

[Darin Adler]

Comment on attachment 422286 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=422286&action=review

> Source/WebCore/ChangeLog:18
> +        because generated JSLocation can't be easily extended.

Is this a long term decision to never fix for location, or just a short term decision to not do it at this time?

> Source/WebCore/bindings/js/JSDOMGlobalObject.cpp:314
> +    JSFunction* jsFunction = m_crossOriginFunctionMap.get(key);
> +    if (!jsFunction) {
> +        jsFunction = JSFunction::create(lexicalGlobalObject->vm(), lexicalGlobalObject, length, propertyName.publicName(), nativeFunction);
> +        m_crossOriginFunctionMap.set(key, jsFunction);
> +    }
> +    return jsFunction;

This is the less-efficient, double hashing, idiom. The more efficient idiom uses either add or ensure. The ensure version is something like this:

    return m_crossOriginFunctionMap.ensure(key, [&] {
        return JSFunction::create(lexicalGlobalObject->vm(), lexicalGlobalObject, length, propertyName.publicName(), nativeFunction);
    }).iterator->value;

I am not sure why WeakGCMap implements set, but neither add nor ensure, so you’d need to deal with that first.

It also seems that WeakGCMap’s set function has a peculiar combination of a non-reference type (I would have expected an rvalue reference as in HashMap itself) but use of WTFMove.

> Source/WebCore/bindings/js/JSDOMGlobalObject.cpp:319
> +    CrossOriginMapKey key = std::make_pair(lexicalGlobalObject, getter ? reinterpret_cast<void*>(getter) : reinterpret_cast<void*>(setter));

I think for clarity we should add this assertion:

    ASSERT(getter || setter);

> Source/WebCore/bindings/js/JSDOMGlobalObject.cpp:328
> +    GetterSetter* getterSetter = m_crossOriginGetterSetterMap.get(key);
> +    if (!getterSetter) {
> +        auto& vm = lexicalGlobalObject->vm();
> +        getterSetter = GetterSetter::create(vm, lexicalGlobalObject,
> +            getter ? JSCustomGetterFunction::create(vm, lexicalGlobalObject, propertyName, getter) : nullptr,
> +            setter ? JSCustomSetterFunction::create(vm, lexicalGlobalObject, propertyName, setter) : nullptr);
> +        m_crossOriginGetterSetterMap.set(key, getterSetter);
> +    }
> +    return getterSetter;

Same as above:

    m_crossOriginFunctionMap.ensure(key, [&] {
        auto& vm = lexicalGlobalObject->vm();
        return GetterSetter::create(vm, lexicalGlobalObject,
            getter ? JSCustomGetterFunction::create(vm, lexicalGlobalObject, propertyName, getter) : nullptr,
            JSCustomSetterFunction::create(vm, lexicalGlobalObject, propertyName, setter) : nullptr);;
    }).iterator->value;

> Source/WebCore/bindings/js/JSDOMGlobalObject.h:88
> +    JSC::JSFunction* getCrossOriginFunction(JSC::JSGlobalObject*, JSC::PropertyName, JSC::NativeFunction, unsigned length);
> +    JSC::GetterSetter* getCrossOriginGetterSetter(JSC::JSGlobalObject*, JSC::PropertyName, JSC::GetValueFunc, JSC::PutValueFunc);

WebKit coding style discourages the use of the word "get" in the names of functions like these.

> Source/WebCore/bindings/js/JSDOMWindowCustom.cpp:206
> +            && slot.isValue() && isHostFunction(slot.getValue(lexicalGlobalObject, propertyName), JSDOMWindow::info()->staticPropHashTable->entry(propertyName)->function());

Rationale for looking this up at runtime instead of link time? Just so we can avoid the need for ForwardDeclareInHeader?

[Alexey Shvayka]

Created attachment 422478 [details]
Patch

Add WeakGCMap::ensure(), rename methods, and reword ChangeLog.

[Alexey Shvayka]

Created attachment 422486 [details]
Patch

Simplify "showModalDialog" lookup a bit.

[Radar WebKit Bug Importer]

<rdar://problem/75315814>

[Alexey Shvayka]

Created attachment 422937 [details]
Patch

Attempt to fix Windows build.

[Alexey Shvayka]

(In reply to Darin Adler from comment #4)
> Comment on attachment 422286 [details]
> Patch

Thank you for very valuable feedback, Darin! 

> Is this a long term decision to never fix for location, or just a short term
> decision to not do it at this time?

The patch does fix Location; reworded ChangeLog to clarify.
It is a long term decision to have cache maps that handle both Window and Location only in one place (on JSDOMGlobalObject).

> This is the less-efficient, double hashing, idiom. The more efficient idiom
> uses either add or ensure. The ensure version is something like this:
> 
>     return m_crossOriginFunctionMap.ensure(key, [&] {
>         return JSFunction::create(lexicalGlobalObject->vm(),
> lexicalGlobalObject, length, propertyName.publicName(), nativeFunction);
>     }).iterator->value;

Wow, this is a great suggestion!
Apart from avoiding double hashing, it's also a nice cleanup.
Applied the similar change to helper methods of JSObject::getOwnPropertyDescriptor().

> I think for clarity we should add this assertion:
> 
>     ASSERT(getter || setter);

Added.

> > Source/WebCore/bindings/js/JSDOMGlobalObject.h:88
> > +    JSC::JSFunction* getCrossOriginFunction(JSC::JSGlobalObject*, JSC::PropertyName, JSC::NativeFunction, unsigned length);
> > +    JSC::GetterSetter* getCrossOriginGetterSetter(JSC::JSGlobalObject*, JSC::PropertyName, JSC::GetValueFunc, JSC::PutValueFunc);
> 
> WebKit coding style discourages the use of the word "get" in the names of
> functions like these.

Renamed to "createCrossOrigin*" in JSDOMGlobalObject.h and similar methods in JSObject.cpp to drop the "get".

> Rationale for looking this up at runtime instead of link time? Just so we
> can avoid the need for ForwardDeclareInHeader?

Yes, only that; reworded ChangeLog to make it clear.
Please note that only "showModalDialog" is affected, which is already removed from Chrome and Firefox.

[Alexey Shvayka]

Created attachment 422986 [details]
Patch

Proper Windows build fix.

[Alexey Shvayka]

Comment on attachment 422986 [details]
Patch

(In reply to Alexey Shvayka from comment #10)
> Created attachment 422986 [details]
> Patch
> 
> Proper Windows build fix.

That did the trick. Judging by other Windows builds, the failures are pre-existing, but were not confirmed as such because rebuild without patch has failed.

I am putting r+ on my own patch so the WPT export bot would (hopefully) approve my PR.
The bot doesn't consider the patch reviewed by Darin as it's now obsolete.

[Alexey Shvayka]

Committed r274379 (235246@main): <https://commits.webkit.org/235246@main>

[WebKit Commit Bot]

Re-opened since this is blocked by bug 223154

[Yusuke Suzuki]

@Alexey Hi! I temporarily reverted this patch since it looks like we are getting some crashes after this patch.

https://results.webkit.org/?suite=layout-tests&test=http%2Ftests%2Fsecurity%2Freferrer-policy-header.html
https://results.webkit.org/?suite=layout-tests&test=imported%2Fw3c%2Fweb-platform-tests%2Fhtml%2Fbrowsers%2Fwindows%2Fnested-browsing-contexts%2Fname-attribute.window.html

Could you take a look?

[Alexey Shvayka]

Created attachment 423132 [details]
Patch

Revert WeakGCMap::ensure() change.

[Alexey Shvayka]

(In reply to Yusuke Suzuki from comment #14)
> @Alexey Hi! I temporarily reverted this patch since it looks like we are
> getting some crashes after this patch.

Hey! I can confirm these tests are consistently crashing for me locally too. Not sure how the patch passed EWS.

I've tracked down the crashes to WeakGCMap::ensure() returning nullptr for (weakly referenced object) value after it was GCed but before WeakGCMap's entries were pruned.

HashMap nicely handles empty and deleted keys, but not values, so I've brought back double hashing approach that not only checks if the key exists, but also that the value is non-null.

In future, we can optimize jsDOMWindowGetOwnPropertySlotRestrictedAccess() to return a cacheable (by raw C++ pointer) slot.

[Darin Adler]

(In reply to Alexey Shvayka from comment #16)
> HashMap nicely handles empty and deleted keys, but not values, so I've
> brought back double hashing approach that not only checks if the key exists,
> but also that the value is non-null.

Do we have to bring back double hashing? Can we just add a null check?

[Alexey Shvayka]

Created attachment 423327 [details]
Patch

[Alexey Shvayka]

(In reply to Darin Adler from comment #17)
> Do we have to bring back double hashing? Can we just add a null check?

My bad, we don't: instead, I've decorated HashMap::ensure() to perform null check for existing entries.
If GCed weak object is encountered, HashMap is updated using AddResult's iterator, thus completely avoiding double hashing.
ensureValue() method name to better reflect its behavior / return value: no more `}).iterator->value.get()` needed. Neat!

[EWS]

Committed r274528: <https://commits.webkit.org/r274528>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 423327 [details].