Bug 254844 – Intermittent removal of adoptedStyleSheet CSSStyleSheet instances when assigning adoptedStyleSheet array

• Component: New Bugs
• Status: RESOLVED FIXED

[Nicholas Rice]

Created attachment 465713 [details]
Failure case

Safari Version: Safari 16.4 (18615.1.26.11.22)
Platform: Macintosh
OS: MacOS 13.3

Bug Description:
We noticed with the release of DocumentOrShadowRoot.adoptedStyleSheets in 16.4 that, intermittently, when assigning the adoptedStyleSheets array to a value that contains existing and new CSSStyleSheet instances, existing CSSStyleSheet instances in the adoptedStyleSheets array are removed.

Repro Steps:
1. Clone https://github.com/microsoft/fast
2. Checkout https://github.com/microsoft/fast/tree/users/nirice/safari-investigation (`git checkout users/nirice/safari-investigation`)
3. Open https://github.com/microsoft/fast/blob/users/nirice/safari-investigation/sites/fast-website/dist/index.html in Safari 16.4
4. The issue is intermittent. In the non-repro case, you will see two pink custom-element buttons. In the failure case, the second button will have no padding and blue underlined text (See attached screen-shots). Keep opening new tabs and loading the HTML file until you see the repro (this can sometimes take 30 or 40 tries to get the repro).

Additional information:
- The code that adds and removes CSSStyleSheet instances to a shadowRoot or Document is here: https://github.com/microsoft/fast/blob/users/nirice/safari-investigation/sites/fast-website/dist/main.js#L7727. To add sheets, a new array is assigned with the values of the adoptedStyleSheets array plus the new CSSStyleSheets. This is where we see the bug.
- Using push and splice to mutate adoptedStyleSheets instead of new array assignment seems to circumvent the issue.
- In the failure case (see the attached screen-shots), reading the array length before and after assignment results in no change to the array length, and the original CSSStyleSheet in the array is missing upon inspection of the array.
- The issue can also be observed at https://www.fast.design/ more frequently. During investigation, it seemed that the more JavaScript I removed or disabled, the less frequently it would repro.

[Nicholas Rice]

Created attachment 465714 [details]
no-repro case

[Radar WebKit Bug Importer]

<rdar://problem/107768559>

[Ryosuke Niwa]

Is this affecting any popular Microsoft website?

[Chris Holt]

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

[Ryosuke Niwa]

I cannot get it to reproduce on my computer.

[Ryosuke Niwa]

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.

[Ryosuke Niwa]

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.

[Ryosuke Niwa]

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.

[Chris Dumez]

(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.

[Chris Dumez]

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);
}   
```

[Chris Dumez]

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).

[Chris Dumez]

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.

[Chris Dumez]

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

[Chris Dumez]

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

[Chris Dumez]

(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.

[Chris Dumez]

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.

[Nayden Naydenov]

(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.

[Chris Holt]

(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.

[Chris Holt]

(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 :)

[Mark Lam]

(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.

[Yusuke Suzuki]

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

[EWS]

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

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

[Milan]

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)