Bug 254844

Created attachment 465714 [details]
no-repro case

<rdar://problem/107768559>

Is this affecting any popular Microsoft website?

It’s been mitigated, but it impacted several different applications and sites.

I cannot get it to reproduce on my computer.

I can finally reproduce it locally. The key is to keep opening new tabs in Safari as opposed to keep reloading the same tab. Confirmed the issue still reproduces at 262851@main.

Alright, I can reproduce it in mini browser as well. I've attached the test case with slight modifications to the radar for those following at home.

The problem occurs on this line (in AdoptedStyleSheetsStyles's addStylesTo):
target.adoptedStyleSheets = [...target.adoptedStyleSheets, ...this.styleSheets];

Here, somehow ...target.adoptedStyleSheets is turning out to be empty even though the length of it is initially 1 immediately before it. Storing items in a real array and spreading that array fixes the problem so this is some GC issue with observable array.

(In reply to Ryosuke Niwa from comment #8)
> The problem occurs on this line (in AdoptedStyleSheetsStyles's addStylesTo):
> target.adoptedStyleSheets = [...target.adoptedStyleSheets,
> ...this.styleSheets];
> 
> Here, somehow ...target.adoptedStyleSheets is turning out to be empty even
> though the length of it is initially 1 immediately before it. Storing items
> in a real array and spreading that array fixes the problem so this is some
> GC issue with observable array.

I am using [CachedAttribute] in WebIDL to cache the observable array in the JSShadowRoot js wrapper.

The generated bindings code looks like:
```
static inline JSValue jsShadowRoot_adoptedStyleSheetsGetter(JSGlobalObject& lexicalGlobalObject, JSShadowRoot& thisObject)
{   
    auto& vm = JSC::getVM(&lexicalGlobalObject);
    auto throwScope = DECLARE_THROW_SCOPE(vm);
    if (JSValue cachedValue = thisObject.m_adoptedStyleSheets.get())
        return cachedValue; 
    auto& impl = thisObject.wrapped();
    JSValue result = toJS<IDLAny>(lexicalGlobalObject, throwScope, impl.adoptedStyleSheetWrapper(*jsCast<JSDOMGlobalObject*>(&lexicalGlobalObject)));
    RETURN_IF_EXCEPTION(throwScope, { });
    thisObject.m_adoptedStyleSheets.set(JSC::getVM(&lexicalGlobalObject), &thisObject, result);
    return result;
}   
        
JSC_DEFINE_CUSTOM_GETTER(jsShadowRoot_adoptedStyleSheets, (JSGlobalObject* lexicalGlobalObject, EncodedJSValue thisValue, PropertyName attributeName))
{       
    return IDLAttribute<JSShadowRoot>::get<jsShadowRoot_adoptedStyleSheetsGetter, CastedThisErrorBehavior::Assert>(*lexicalGlobalObject, thisValue, attributeName);
}       
    
static inline bool setJSShadowRoot_adoptedStyleSheetsSetter(JSGlobalObject& lexicalGlobalObject, JSShadowRoot& thisObject, JSValue value)
{   
    auto& vm = JSC::getVM(&lexicalGlobalObject);
    UNUSED_PARAM(vm);
    thisObject.setAdoptedStyleSheets(lexicalGlobalObject, value);
    return true;
}       
    
JSC_DEFINE_CUSTOM_SETTER(setJSShadowRoot_adoptedStyleSheets, (JSGlobalObject* lexicalGlobalObject, EncodedJSValue thisValue, EncodedJSValue encodedValue, PropertyName attributeName))
{   
    return IDLAttribute<JSShadowRoot>::set<setJSShadowRoot_adoptedStyleSheetsSetter>(*lexicalGlobalObject, thisValue, encodedValue, attributeName);
}
```

With m_adoptedStylesheets being declared like so:
```
mutable JSC::WriteBarrier<JSC::Unknown> m_adoptedStyleSheets;
```

Also note that the generated visit function does visit `m_adoptedStyleSheets`:
```
template<typename Visitor>
void JSShadowRoot::visitChildrenImpl(JSCell* cell, Visitor& visitor)
{
    auto* thisObject = jsCast<JSShadowRoot*>(cell);
    ASSERT_GC_OBJECT_INHERITS(thisObject, info());
    Base::visitChildren(thisObject, visitor);
    visitor.append(thisObject->m_adoptedStyleSheets);
}   
```

And the type of m_adoptedStyleSheet is a JSC::JSObservableArray (which is a new type I wrote so I could have gotten something wrong there).

Created attachment 465867 [details]
Fix attempt (not working)

I have tried to rewrite this using JSValueInWrappedObject, like we do for some other DOM objects but it didn't address the issue. Attaching the patch for reference.

I have also tried playing with the StructureFlags on JSObservableArray but without success.

Clearing the cached m_adoptedStyleSheets whenever setAdoptedStyleSheets() gets called seems to work.

(In reply to Chris Dumez from comment #14)
> Clearing the cached m_adoptedStyleSheets whenever setAdoptedStyleSheets()
> gets called seems to work.

I spoke too soon. The issue still occurs, just less frequently for me.

I also tried modifying setAdoptedStyleSheetsOnTreeScope() to modify the existing JSObservableArray (deleting all properties in it then setting the new ones one by one) but it didn't help either:
```
void setAdoptedStyleSheetsOnTreeScope(TreeScope& treeScope, JSC::JSObservableArray* existingArray, JSC::JSGlobalObject& lexicalGlobalObject, JSC::JSValue value)
{
    auto& vm = JSC::getVM(&lexicalGlobalObject);
    auto throwScope = DECLARE_THROW_SCOPE(vm);
    if (existingArray) {
        auto* jsObject = jsDynamicCast<JSObject*>(value);
        if (!jsObject)
            return;

        while (existingArray->length()) {
            jsCast<JSObject*>(existingArray)->deleteProperty((JSGlobalObject*)&lexicalGlobalObject, (uint64_t)0);
            RETURN_IF_EXCEPTION(throwScope, void());
        }

        unsigned index = 0;
        forEachInIterable(&lexicalGlobalObject, jsObject, [existingArray, &index](JSC::VM&, JSC::JSGlobalObject* lexicalGlobalObject, JSC::JSValue nextValue) {
            if (existingArray->putByIndexInline(lexicalGlobalObject, index, nextValue, false))
                ++index;
        });
    } else {
        auto nativeValue = convert<IDLFrozenArray<IDLInterface<CSSStyleSheet>>>(lexicalGlobalObject, value);
        RETURN_IF_EXCEPTION(throwScope, void());
        invokeFunctorPropagatingExceptionIfNecessary(lexicalGlobalObject, throwScope, [&] {
            return treeScope.setAdoptedStyleSheets(WTFMove(nativeValue));
        });
    }
}
```

At this point, I think I'll need help with someone with more JSC knowledge.

(In reply to Chris Dumez from comment #16)
> I also tried modifying setAdoptedStyleSheetsOnTreeScope() to modify the
> existing JSObservableArray (deleting all properties in it then setting the
> new ones one by one) but it didn't help either:
> ```
> void setAdoptedStyleSheetsOnTreeScope(TreeScope& treeScope,
> JSC::JSObservableArray* existingArray, JSC::JSGlobalObject&
> lexicalGlobalObject, JSC::JSValue value)
> {
>     auto& vm = JSC::getVM(&lexicalGlobalObject);
>     auto throwScope = DECLARE_THROW_SCOPE(vm);
>     if (existingArray) {
>         auto* jsObject = jsDynamicCast<JSObject*>(value);
>         if (!jsObject)
>             return;
> 
>         while (existingArray->length()) {
>            
> jsCast<JSObject*>(existingArray)-
> >deleteProperty((JSGlobalObject*)&lexicalGlobalObject, (uint64_t)0);
>             RETURN_IF_EXCEPTION(throwScope, void());
>         }
> 
>         unsigned index = 0;
>         forEachInIterable(&lexicalGlobalObject, jsObject, [existingArray,
> &index](JSC::VM&, JSC::JSGlobalObject* lexicalGlobalObject, JSC::JSValue
> nextValue) {
>             if (existingArray->putByIndexInline(lexicalGlobalObject, index,
> nextValue, false))
>                 ++index;
>         });
>     } else {
>         auto nativeValue =
> convert<IDLFrozenArray<IDLInterface<CSSStyleSheet>>>(lexicalGlobalObject,
> value);
>         RETURN_IF_EXCEPTION(throwScope, void());
>         invokeFunctorPropagatingExceptionIfNecessary(lexicalGlobalObject,
> throwScope, [&] {
>             return treeScope.setAdoptedStyleSheets(WTFMove(nativeValue));
>         });
>     }
> }
> ```
> 
> At this point, I think I'll need help with someone with more JSC knowledge.

Do you have any updates? Our customers started to update their macOS devices and they are facing the issue more and more often? We are making an UI library similar to @microsoft/fast and many applications are with broken styles.

(In reply to Ryosuke Niwa from comment #8)
> The problem occurs on this line (in AdoptedStyleSheetsStyles's addStylesTo):
> target.adoptedStyleSheets = [...target.adoptedStyleSheets,
> ...this.styleSheets];
> 
> Here, somehow ...target.adoptedStyleSheets is turning out to be empty even
> though the length of it is initially 1 immediately before it. Storing items
> in a real array and spreading that array fixes the problem so this is some
> GC issue with observable array.

We had this mitigated and are again seeing the issue intermittently on the FAST site on 16.6 Beta - I don’t see anything related on the release notes, did something change with this implementation.

(In reply to Chris Holt from comment #18)
> (In reply to Ryosuke Niwa from comment #8)
> > The problem occurs on this line (in AdoptedStyleSheetsStyles's addStylesTo):
> > target.adoptedStyleSheets = [...target.adoptedStyleSheets,
> > ...this.styleSheets];
> > 
> > Here, somehow ...target.adoptedStyleSheets is turning out to be empty even
> > though the length of it is initially 1 immediately before it. Storing items
> > in a real array and spreading that array fixes the problem so this is some
> > GC issue with observable array.
> 
> We had this mitigated and are again seeing the issue intermittently on the
> FAST site on 16.6 Beta - I don’t see anything related on the release notes,
> did something change with this implementation.

Disregard this - I don’t think the current build has it deployed so this is on our end, the mitigation seems to still be working. 

With that, any update from the WebKit team would be greatly appreciated :)

(In reply to Chris Holt from comment #19)
> With that, any update from the WebKit team would be greatly appreciated :)

We're still investigating the issue.

Pull request: https://github.com/WebKit/WebKit/pull/16265

Committed 266464@main (a347abe159d3): <https://commits.webkit.org/266464@main>

Reviewed commits have been landed. Closing PR #16265 and removing active labels.

For those interested, looks like this fix was included in Safari 17.1 per the release notes: https://developer.apple.com/documentation/safari-release-notes/safari-17_1-release-notes#Web-API

> Fixed intermittent removal of adoptedStyleSheet CSSStyleSheet instances when assigning an adoptedStyleSheet array. (107768559)