Cache cross-origin methods / accessors of Window and Location per lexical global object
Created attachment 422227 [details] Patch
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
Created attachment 422286 [details] Patch Fix & simplify CrossOriginMapKey, and include missing test file.
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?
Created attachment 422478 [details] Patch Add WeakGCMap::ensure(), rename methods, and reword ChangeLog.
Created attachment 422486 [details] Patch Simplify "showModalDialog" lookup a bit.
<rdar://problem/75315814>
Created attachment 422937 [details] Patch Attempt to fix Windows build.
(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.
Created attachment 422986 [details] Patch Proper Windows build fix.
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.
Committed r274379 (235246@main): <https://commits.webkit.org/235246@main>
Re-opened since this is blocked by bug 223154
@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?
Created attachment 423132 [details] Patch Revert WeakGCMap::ensure() change.
(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.
(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?
Created attachment 423327 [details] Patch
(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!
Committed r274528: <https://commits.webkit.org/r274528> All reviewed patches have been landed. Closing bug and clearing flags on attachment 423327 [details].