Bug 191443 – CSS Painting API should pass 'this' correctly to paint callback, and repaint when properties change.

• Component: Layout and Rendering
• Status: RESOLVED FIXED

[Justin Michaud]

CSS Painting API should pass 'this' correctly to paint callback, and repaint when properties change.

[Justin Michaud]

Created attachment 354301 [details]
Patch

[Justin Michaud]

Created attachment 354303 [details]
Patch

[Justin Michaud]

Created attachment 354306 [details]
Patch

[Dean Jackson]

Comment on attachment 354306 [details]
Patch

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

> Source/WebCore/ChangeLog:9
> +        Also, this patch makes sure that custon paint elements get repainted when properties that they care about get changed.

Typo: custom

> Source/WebCore/bindings/js/JSDOMWrapper.cpp:38
>  namespace WebCore {
> +using namespace JSC;

We usually put a blank line between these.

> Source/WebCore/rendering/style/RenderStyle.h:1877
> +    std::unique_ptr<HashSet<String>> m_customPaintWatchProperties;

I wonder if you should "using" HashSet<String> to something like CustomPaintNameSet

> Source/WebCore/worklets/PaintWorkletGlobalScope.cpp:56
> -    if (!responsableDocument() || !responsableDocument()->domWindow())
> +    if (!responsibleDocument() || !responsibleDocument()->domWindow())
>          return 1.0;
> -    return responsableDocument()->domWindow()->devicePixelRatio();
> +    return responsibleDocument()->domWindow()->devicePixelRatio();

Very nice!

[Chris Dumez]

Comment on attachment 354306 [details]
Patch

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

> Source/WebCore/platform/graphics/CustomPaintImage.h:59
> +    JSC::Strong<JSC::JSObject> m_paintConstructor;

Why is it OK to use JSC::Strong here?

[Justin Michaud]

(In reply to Chris Dumez from comment #5)
> Comment on attachment 354306 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=354306&action=review
> 
> > Source/WebCore/platform/graphics/CustomPaintImage.h:59
> > +    JSC::Strong<JSC::JSObject> m_paintConstructor;
> 
> Why is it OK to use JSC::Strong here?

It is created in JS and provided by the user, so it shouldn't have any references back, right? It is passed to the bindings in PaintWorkletGlobalScope::registerPaint as a Strong<>, so I assumed that it was fine.

[Chris Dumez]

(In reply to Justin Michaud from comment #6)
> (In reply to Chris Dumez from comment #5)
> > Comment on attachment 354306 [details]
> > Patch
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=354306&action=review
> > 
> > > Source/WebCore/platform/graphics/CustomPaintImage.h:59
> > > +    JSC::Strong<JSC::JSObject> m_paintConstructor;
> > 
> > Why is it OK to use JSC::Strong here?
> 
> It is created in JS and provided by the user, so it shouldn't have any
> references back, right? It is passed to the bindings in
> PaintWorkletGlobalScope::registerPaint as a Strong<>, so I assumed that it
> was fine.

It is very easy for JS to pass us an object which refers to one of our implementation wrappers, thus keeping it alive. If our implementation uses a JSStrong then we potentially create a cycle and a leak. This is why we do not use JSStrong in our implementation.

[Saam Barati]

Comment on attachment 354306 [details]
Patch

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

> Source/WebCore/platform/graphics/CustomPaintImage.cpp:112
> +    JSC::JSLockHolder lock(vm);

Please do this before declaring throw scope and getting the global object

> Source/WebCore/platform/graphics/CustomPaintImage.cpp:116
> +    JSC::JSValue thisObject = { JSC::construct(&state, { m_paintConstructor.get() }, noArgs, "Failed to construct paint class") };

Why { }?

[Darin Adler]

(In reply to Chris Dumez from comment #7)
> (In reply to Justin Michaud from comment #6)
> > (In reply to Chris Dumez from comment #5)
> > > Comment on attachment 354306 [details]
> > > Patch
> > > 
> > > View in context:
> > > https://bugs.webkit.org/attachment.cgi?id=354306&action=review
> > > 
> > > > Source/WebCore/platform/graphics/CustomPaintImage.h:59
> > > > +    JSC::Strong<JSC::JSObject> m_paintConstructor;
> > > 
> > > Why is it OK to use JSC::Strong here?
> > 
> > It is created in JS and provided by the user, so it shouldn't have any
> > references back, right? It is passed to the bindings in
> > PaintWorkletGlobalScope::registerPaint as a Strong<>, so I assumed that it
> > was fine.
> 
> It is very easy for JS to pass us an object which refers to one of our
> implementation wrappers, thus keeping it alive. If our implementation uses a
> JSStrong then we potentially create a cycle and a leak. This is why we do
> not use JSStrong in our implementation.

That’s right. This use of JSC::Strong is not OK.

We can, and should, create a test case demonstrating that this leads to a reference cycle and a memory leak.

[Justin Michaud]

Created attachment 354629 [details]
Patch

[Justin Michaud]

I fixed two leaks that caused the WorkletGlobalScope to never be destroyed, and added a leak test. I also moved the new RenderStyle properties to a side table, so hopefully this shouldn't cause any performance regressions.

(In reply to Saam Barati from comment #8)
> > Source/WebCore/platform/graphics/CustomPaintImage.cpp:116
> > +    JSC::JSValue thisObject = { JSC::construct(&state, { m_paintConstructor.get() }, noArgs, "Failed to construct paint class") };
> 
> Why { }?

It needs to turn the JSObject* into a JSValue.

[Saam Barati]

Comment on attachment 354629 [details]
Patch

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

> Source/WebCore/platform/graphics/CustomPaintImage.cpp:61
> +    if (!m_element || !m_element->element() || !m_paintConstructor || !m_paintCallback)

This feels super weird. Why is this constructor a weak pointer?

[Saam Barati]

Comment on attachment 354629 [details]
Patch

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

>> Source/WebCore/platform/graphics/CustomPaintImage.cpp:61
>> +    if (!m_element || !m_element->element() || !m_paintConstructor || !m_paintCallback)
> 
> This feels super weird. Why is this constructor a weak pointer?

Same w/ the callback

[Justin Michaud]

(In reply to Saam Barati from comment #13)
> Comment on attachment 354629 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=354629&action=review
> 
> >> Source/WebCore/platform/graphics/CustomPaintImage.cpp:61
> >> +    if (!m_element || !m_element->element() || !m_paintConstructor || !m_paintCallback)
> > 
> > This feels super weird. Why is this constructor a weak pointer?
> 
> Same w/ the callback

The worklet global scope keeps the callback alive. The image might get cached, so Chris suggested it should take a weak ptr. This also means that I don't have to keep track of all of the CustomPaintImages to clear their strong references in Document::prepareForDestruction.

[Saam Barati]

Comment on attachment 354629 [details]
Patch

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

> Source/WebCore/platform/graphics/CustomPaintImage.cpp:122
> +    auto result = m_paintCallback->handleEvent(WTFMove(thisObject), *context, size, propertyMap, m_arguments);

I think your code is suffering from TOCTOU bug here. It's totally conceivable your constructor will cause the collection of your m_painCallback. If you really want these to be weak, you need to assign them into local variables in this function. You can't repeatedly load the fields from memory (conservative GC is not guaranteed to see them).

[Saam Barati]

Comment on attachment 354629 [details]
Patch

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

>> Source/WebCore/platform/graphics/CustomPaintImage.cpp:122
>> +    auto result = m_paintCallback->handleEvent(WTFMove(thisObject), *context, size, propertyMap, m_arguments);
> 
> I think your code is suffering from TOCTOU bug here. It's totally conceivable your constructor will cause the collection of your m_painCallback. If you really want these to be weak, you need to assign them into local variables in this function. You can't repeatedly load the fields from memory (conservative GC is not guaranteed to see them).

Oh sorry, I was thinking m_paintCallbac was a JSCell. It appears not to be. Ignore me here. Though it's generally go practice when dealing with a Weak<SomeKindOfJSCell> to store it in a local variable, and test that for nulllness.

[Saam Barati]

Comment on attachment 354629 [details]
Patch

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

>>>> Source/WebCore/platform/graphics/CustomPaintImage.cpp:61
>>>> +    if (!m_element || !m_element->element() || !m_paintConstructor || !m_paintCallback)
>>> 
>>> This feels super weird. Why is this constructor a weak pointer?
>> 
>> Same w/ the callback
> 
> The worklet global scope keeps the callback alive. The image might get cached, so Chris suggested it should take a weak ptr. This also means that I don't have to keep track of all of the CustomPaintImages to clear their strong references in Document::prepareForDestruction.

What keeps m_paintConstructor alive?

[Saam Barati]

Comment on attachment 354629 [details]
Patch

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

>>> Source/WebCore/platform/graphics/CustomPaintImage.cpp:122
>>> +    auto result = m_paintCallback->handleEvent(WTFMove(thisObject), *context, size, propertyMap, m_arguments);
>> 
>> I think your code is suffering from TOCTOU bug here. It's totally conceivable your constructor will cause the collection of your m_painCallback. If you really want these to be weak, you need to assign them into local variables in this function. You can't repeatedly load the fields from memory (conservative GC is not guaranteed to see them).
> 
> Oh sorry, I was thinking m_paintCallbac was a JSCell. It appears not to be. Ignore me here. Though it's generally go practice when dealing with a Weak<SomeKindOfJSCell> to store it in a local variable, and test that for nulllness.

typo:
"go practice" => "good practice"

[Justin Michaud]

(In reply to Saam Barati from comment #17)
> Comment on attachment 354629 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=354629&action=review
> 
> >>>> Source/WebCore/platform/graphics/CustomPaintImage.cpp:61
> >>>> +    if (!m_element || !m_element->element() || !m_paintConstructor || !m_paintCallback)
> >>> 
> >>> This feels super weird. Why is this constructor a weak pointer?
> >> 
> >> Same w/ the callback
> > 
> > The worklet global scope keeps the callback alive. The image might get cached, so Chris suggested it should take a weak ptr. This also means that I don't have to keep track of all of the CustomPaintImages to clear their strong references in Document::prepareForDestruction.
> 
> What keeps m_paintConstructor alive?

The PaintworkletGlobalScope paint definition map does. Although to avoid TOCTTOU, should I be storing the JSObject in a local variable before doing the check?

[Justin Michaud]

Created attachment 354636 [details]
Patch

[Saam Barati]

(In reply to Justin Michaud from comment #19)
> (In reply to Saam Barati from comment #17)
> > Comment on attachment 354629 [details]
> > Patch
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=354629&action=review
> > 
> > >>>> Source/WebCore/platform/graphics/CustomPaintImage.cpp:61
> > >>>> +    if (!m_element || !m_element->element() || !m_paintConstructor || !m_paintCallback)
> > >>> 
> > >>> This feels super weird. Why is this constructor a weak pointer?
> > >> 
> > >> Same w/ the callback
> > > 
> > > The worklet global scope keeps the callback alive. The image might get cached, so Chris suggested it should take a weak ptr. This also means that I don't have to keep track of all of the CustomPaintImages to clear their strong references in Document::prepareForDestruction.
> > 
> > What keeps m_paintConstructor alive?
> 
> The PaintworkletGlobalScope paint definition map does. Although to avoid
> TOCTTOU, should I be storing the JSObject in a local variable before doing
> the check?

Why would that map keep it alive and we also have a Weak<> to it? If the map keeps it alive why will this check ever fail?

To avoid TOCTOU (which I don't think you have anymore), you should assign it to a local variable instead of keeping it in memory. No need for strong. A local variable will be scanned by the conservative GC.

[Saam Barati]

What am I missing? I don't see how that map keeps alive the m_paintConstructor...

[Justin Michaud]

(In reply to Saam Barati from comment #21)
> Why would that map keep it alive and we also have a Weak<> to it? If the map
> keeps it alive why will this check ever fail?

I think I understand what you mean now. The map (PaintWorkletGlobalScope.h:51) has a JSC Strong<JSObject> to hold the paint constructor. This will keep the constructor JSObject alive up until document has prepareForDestruction called. Once prepareForDestruction is called, it will die, but it is fine that the image does not paint correctly. If the image kept the paint constructor alive, then if ever the image could be referenced from the JS code (such as the typed om), I suspect it could create a reference cycle.

[Darin Adler]

If this is a typical example of the DOM needing to hold a reference to an arbitrary JavaScript object, then the correct alternative to Strong<> is not Weak<>, it is JSValueInWrappedObject.

It’s not trivial to use JSValueInWrappedObject correctly, but it’s designed for this sort of situation. Search for classes that use it for examples of how it works.

[Justin Michaud]

Created attachment 354758 [details]
Patch

[Justin Michaud]

(In reply to Darin Adler from comment #24)
> If this is a typical example of the DOM needing to hold a reference to an
> arbitrary JavaScript object, then the correct alternative to Strong<> is not
> Weak<>, it is JSValueInWrappedObject.
> 
> It’s not trivial to use JSValueInWrappedObject correctly, but it’s designed
> for this sort of situation. Search for classes that use it for examples of
> how it works.

I am not sure what benefit JSValueInWrappedObject provides in this case. 

I changed the strong to use a Raw pointer based off Saam's advice, since Weak is not safe to use after the vm has been destroyed. JSPaintWorkletGlobalScope now visits the pointer, and is itself kept alive by the Strong in WorkletScriptController. Now, the ownership looks like this:

Document has a Ref to PaintWorkletGlobalScope. PaintWorkletGlobalScope is a WorkletGlobalScope. WorkletGlobalScope has a WorkletScriptController, which has a VM and a JSC::Strong pointing to the wrapper JSPaintWorkletGlobalScope (a JSWorkletGlobalScope). 

JSPaintWorkletGlobalScope visits the paint constructors to keep them alive. They are stored in a PaintDefinition in the paintDefinitionMap inside PaintWorkletGlobalScope. CustomPaintImage has a WeakPtr to its PaintDefinition.

In prepareForDestruction, we clear the paint definition map and then destroy the WorkletScriptController, which takes the vm and strong JSWorkletGlobalScope with it. There is a test that confirms that there are no memory leaks.

In theory, the VM does not need to be destroyed to remove the reference cycle. If the strong JSWorkletGlobalScope is gone, then the WorkletGlobalScope cannot visit any of the constructors. There seems to be no other way for the user to keep a reference to the JSPaintWorkletGlobalScope, so I suspect that if we called the gc enough times, eventually the WorkletGlobalScope would be freed. I have not confirmed this though.

[Darin Adler]

Sounds fine. I think the best way to prove this works is with tests.

[Justin Michaud]

Created attachment 354829 [details]
Patch

[Chris Dumez]

Comment on attachment 354829 [details]
Patch

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

> Source/WebCore/rendering/style/RenderStyle.cpp:86
> +    bool hasCustomPaintWatchProperties;

Is it really OK to increase the size of RenderStyle? Clearly we care about its size given the static_assert.

> Source/WebCore/rendering/style/RenderStyle.cpp:157
> +        m_hasCustomPaintWatchProperties = true;

Is this flag just to avoid a HashMap lookup?

> Source/WebCore/rendering/style/RenderStyle.cpp:180
> +    if (other.m_hasCustomPaintWatchProperties) {

What if this->m_hasCustomPaintWatchProperties is true, shouldn't use remove this from customPaintWatchedPropertiesMap()?

You would have potentially found this bug if you had an assertion whenever you call customPaintWatchedPropertiesMap().add() to make sure we added new entry.

> Source/WebCore/rendering/style/RenderStyle.cpp:1052
> +        customPaintWatchedPropertiesMap().add(this, std::make_unique<HashSet<String>>());

We use HashMap::ensure() to make sure we avoid expensive operations such as std::make_unique<HashSet<String>>() when the key already exists.

> Source/WebCore/worklets/PaintWorkletGlobalScope.h:49
> +    struct PaintDefinition : public CanMakeWeakPtr<PaintDefinition> {

struct PaintDefinition : public CanMakeWeakPtr<PaintDefinition> {
WTF_MAKE_FAST_ALLOCATED;
public:

> Source/WebCore/worklets/PaintWorkletGlobalScope.h:57
> +        PaintDefinition(const AtomicString& name, JSC::JSObject* paintConstructor, Ref<CSSPaintCallback>&&, Vector<String>&& inputProperties, Vector<String> inputArguments);

This usually comes *before* the members.

Vector<String> should not be passed by value.

> Source/WebCore/worklets/PaintWorkletGlobalScope.h:77
> +        auto locker = holdLock(paintDefinitionLock());

#if !ASSERT_DISABLED
before this.

> Source/WebCore/worklets/PaintWorkletGlobalScope.h:78
> +        ASSERT(paintDefinitionMap().isEmpty());

#endif
after this. to avoid locking when assertions are disabled.

> Source/WebCore/worklets/WorkletGlobalScope.cpp:55
> +    , m_identifier(generateObjectIdentifier<WorkletGlobalScopeIdentifierType>())

What is this identifier used for? If it is only a key in the HashMap, we could presumably store |this| as key?

[Chris Dumez]

+Antti, I think he should definitely take a look at the RenderStyle changes as I am worried about the performance implications (memory and runtime).

[Chris Dumez]

Comment on attachment 354829 [details]
Patch

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

> Source/WebCore/worklets/PaintWorkletGlobalScope.h:52
> +        JSC::JSObject* paintConstructor;

JSC::JSObject* paintConstructor { nullptr };

[Chris Dumez]

Comment on attachment 354829 [details]
Patch

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

> Source/WebCore/rendering/style/RenderStyle.cpp:165
> +RenderStyle& RenderStyle::operator=(RenderStyle&& other)

This should probably check for self assignment and return early.

[Justin Michaud]

Created attachment 355015 [details]
Patch

[Justin Michaud]

Created attachment 355016 [details]
Patch

[Justin Michaud]

Created attachment 355019 [details]
Patch

[Justin Michaud]

(In reply to Chris Dumez from comment #29)
> > Source/WebCore/worklets/WorkletGlobalScope.cpp:55
> > +    , m_identifier(generateObjectIdentifier<WorkletGlobalScopeIdentifierType>())
> 
> What is this identifier used for? If it is only a key in the HashMap, we
> could presumably store |this| as key?

I didn't do this yet, I will handle it tomorrow.

About the size of RenderStyle: There are currently 3 bytes of padding, but they are between the two bitfield structs, so I can't just move my bool there. I could add my bool as a flag to inheritedFlags, although I would need to override the operators to make sure it doesn't act like any of the other flags.

In the current patch, I added #pragma pack to RenderStyle. Assuming everything compiles correctly, this brings RenderStyle from 88 to 86 bytes. I don't think the unaligned accesses will be a perf problem, since it is all around the bitfields anyway, but I am not sure. If this works though, I think it is a cleaner long-term solution. 

Any thoughts?

[Justin Michaud]

Created attachment 355111 [details]
Patch

[Justin Michaud]

Created attachment 355114 [details]
Patch

[Dean Jackson]

Comment on attachment 355114 [details]
Patch

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

Reviewing this while everyone else is on a break. They should double check when they come back.

> Source/WebCore/ChangeLog:13
> +        This class adds a new member to RenderStyle. To stop this from increasing the size of RenderStyle, this patch uses #pragma pack
> +        to pack RenderStyle, shrinking it from 88 bytes to 86.

I'm r+ing this, but Antti should confirm this is ok.

> Source/WebCore/rendering/style/RenderStyle.cpp:87
> +static_assert(sizeof(RenderStyle) <= sizeof(SameSizeAsRenderStyle), "RenderStyle should stay small");

Don't we normally change SameSizeAsRenderStyle? I guess this would allow it to grow in the future, as long as we never exceed that value.

> Source/WebCore/rendering/style/RenderStyle.cpp:155
> +        auto isNew = customPaintWatchedPropertiesMap().add(this, std::make_unique<HashSet<String>>(WTFMove(*customPaintWatchedPropertiesMap().get(&other))));

Even though you are always expecting this to create a new entry, I think you should use .ensure() anyway.

> Source/WebCore/rendering/style/RenderStyle.cpp:186
> +        auto isNew = customPaintWatchedPropertiesMap().add(this, std::make_unique<HashSet<String>>(WTFMove(*customPaintWatchedPropertiesMap().get(&other))));

Same here.

> Source/WebCore/rendering/style/RenderStyle.cpp:275
> +        auto isNew = customPaintWatchedPropertiesMap().add(this, std::make_unique<HashSet<String>>(*customPaintWatchedPropertiesMap().get(&other)));
> +        ASSERT_UNUSED(isNew, isNew);

And here.

> Source/WebCore/rendering/style/RenderStyle.cpp:297
> +        auto isNew = customPaintWatchedPropertiesMap().add(this, std::make_unique<HashSet<String>>(WTFMove(*customPaintWatchedPropertiesMap().get(&a))));
> +        ASSERT_UNUSED(isNew, isNew);

Ditto.

> Source/WebCore/rendering/style/RenderStyle.cpp:1064
> +        auto isNew = customPaintWatchedPropertiesMap().add(this, std::make_unique<HashSet<String>>());

And one more.

[Antti Koivisto]

Comment on attachment 355114 [details]
Patch

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

Please put RenderStyle related changes to a separate patch.

> Source/WebCore/rendering/style/RenderStyle.h:1890
> +#if ENABLE(CSS_PAINTING_API)
> +    bool m_hasCustomPaintWatchProperties { false };
> +#endif

I don't see any reason why this bit is super special and needs to be in the RenderStyle, necessitating custom copy constructors, operator== etc.

It should go to one of the substructures, probably RareNonInheritedData. There is already StyleCustomPropertyData instance there, so maybe that is the place? Also you don't need to use bit-and-side-hash approach. You can just put the hash there.

[Justin Michaud]

Created attachment 355920 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 355920 [details]
Patch

Clearing flags on attachment: 355920

Committed r238686: <https://trac.webkit.org/changeset/238686>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/46351466>

[Darin Adler]

Comment on attachment 355920 [details]
Patch

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

I looked this over and had a few thoughts.

Might be worth considering using AtomicString for property names instead of just String.

> Source/WebCore/bindings/js/JSDOMWrapper.cpp:38
> +using namespace JSC;

Could we add explicit "JSC::" prefixes in this file instead adding the "using namespace"? Or remove the explicit prefixes that are already present below? It seems inelegant to do a seemingly random combination of both.

> Source/WebCore/bindings/js/JSWorkletGlobalScopeBase.cpp:131
> +    ASSERT(workletGlobalScope.script());

Why is it safe to assert this? What guarantees it won’t be null?

> Source/WebCore/bindings/js/JSWorkletGlobalScopeBase.cpp:133
>      ASSERT(contextWrapper);

Same question.

> Source/WebCore/css/CSSPaintCallback.h:34
> +#include <JavaScriptCore/JSCJSValue.h>

We typically don’t need to include a header just to mention a typename in a function declaration. A forward declaration is typically sufficient:

    namespace JSC {
    class JSValue;
    }

> Source/WebCore/dom/ScriptExecutionContext.cpp:485
> +        return downcast<WorkletGlobalScope>(*this).script()->vm();

What guarantees this won’t be null?

> Source/WebCore/platform/graphics/CustomPaintImage.cpp:63
> +    JSC::JSValue paintConstructor(m_paintDefinition->paintConstructor);

It’s a little strange to use the () syntax here instead of just "=".

> Source/WebCore/platform/graphics/CustomPaintImage.cpp:68
> +    auto& paintCallback = m_paintDefinition->paintCallback.get();

I don’t understand why we need this local variable here. Can’t we just write this expression below when setting up "callback"?

> Source/WebCore/platform/graphics/CustomPaintImage.cpp:121
> +    JSC::ArgList noArgs;
> +    JSC::JSValue thisObject = { JSC::construct(&state, WTFMove(paintConstructor), noArgs, "Failed to construct paint class") };

The use of WTFMove here seems excessive. JSValue has a trivial copy constructor so there should be no value in moving paintConstructor.

Can this be done by passing { } instead of noArgs?

> Source/WebCore/platform/graphics/CustomPaintImage.cpp:128
> +    auto result = paintCallback.handleEvent(WTFMove(thisObject), *context, size, propertyMap, m_arguments);

The use of WTFMove here seems excessive. JSValue has a trivial copy constructor so there should be no value in moving thisObject.

> Source/WebCore/platform/graphics/CustomPaintImage.h:32
> +#include <JavaScriptCore/JSObject.h>

Why does JSObject.h need to be included here?

> Source/WebCore/rendering/style/RenderStyle.cpp:1035
> +#if ENABLE(CSS_PAINTING_API)
> +    auto* propertiesA = m_rareNonInheritedData.ptr()->customPaintWatchedProperties.get();
> +    auto* propertiesB = other.m_rareNonInheritedData.ptr()->customPaintWatchedProperties.get();
> +
> +    if (UNLIKELY(propertiesA || propertiesB)) {
> +        // FIXME: We should not need to use ComputedStyleExtractor here.
> +        ComputedStyleExtractor extractor((Element*) nullptr);
> +
> +        for (auto* watchPropertiesMap : { propertiesA, propertiesB }) {
> +            if (!watchPropertiesMap)
> +                continue;
> +
> +            for (auto& name : *watchPropertiesMap) {
> +                RefPtr<CSSValue> valueA;
> +                RefPtr<CSSValue> valueB;
> +                if (isCustomPropertyName(name) && getCustomProperty(name) && other.getCustomProperty(name)) {
> +                    valueA = CSSCustomPropertyValue::create(*getCustomProperty(name));
> +                    valueB = CSSCustomPropertyValue::create(*other.getCustomProperty(name));
> +                } else {
> +                    CSSPropertyID propertyID = cssPropertyID(name);
> +                    if (!propertyID)
> +                        continue;
> +                    valueA = extractor.valueForPropertyinStyle(*this, propertyID);
> +                    valueB = extractor.valueForPropertyinStyle(other, propertyID);
> +                }
> +
> +                if ((valueA && !valueB) || (!valueA && valueB))
> +                    return true;
> +
> +                if (!valueA)
> +                    continue;
> +
> +                if (!(*valueA == *valueB))
> +                    return true;
> +            }
> +        }
> +    }
> +#endif

This seems like it should be factored out into a function. It’s bigger than the rest of RenderStyle::changeRequiresRepaint and the rest follows a pattern where there is not so much code here. I’m also wondering why this is not handled inside the rareNonInheritedDataChangeRequiresRepaint function.

Is there a way we can do these checks with less memory allocation and deallocation?

> Source/WebCore/worklets/PaintWorkletGlobalScope.cpp:145
> +    if (responsibleDocument() && responsibleDocument()->renderView())
> +        responsibleDocument()->renderView()->repaintRootContents();

I think this needs a "why" comment. It’s not so obvious why this would be needed that we can just do it without explaining the reason.

> Source/WebCore/worklets/PaintWorkletGlobalScope.h:54
> +        // This map must be cleared before the vm is destroyed!
> +        JSC::JSObject* paintConstructor { nullptr };

Slightly confusing wording. Since paintConstructor is not a map, nor is PaintDefinition.

Of course I assume this comment is about paintDefinitionMap, and I think it should be worded more precisely. Having that comment here isn’t all that helpful even if it’s the presence of this pointer that motivates the need to clear the map. Because I wouldn't expect the person making the avoidable mistake to be reading this line of code.

Why is the paintConstructor the only non-const member of PaintDefinition? I think the pattern of making all data members const is a little strange, but it’s even stranger to have all but one even when we have no plans to change that one. I would either write JSC::JSObject* const, or consider abandoning the "intrinsically const" property of PaintDefinition.

[Justin Michaud]

Comment on attachment 355920 [details]
Patch

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

>> Source/WebCore/bindings/js/JSDOMWrapper.cpp:38
>> +using namespace JSC;
> 
> Could we add explicit "JSC::" prefixes in this file instead adding the "using namespace"? Or remove the explicit prefixes that are already present below? It seems inelegant to do a seemingly random combination of both.

I added that to fix compilation errors from unified sources. It sounds like a nice thing to clean up in a separate patch. I guess the file was originally supposed to use the prefix, but some code got checked in that didn't because the unified sources allowed it to compile.

>> Source/WebCore/css/CSSPaintCallback.h:34
>> +#include <JavaScriptCore/JSCJSValue.h>
> 
> We typically don’t need to include a header just to mention a typename in a function declaration. A forward declaration is typically sufficient:
> 
>     namespace JSC {
>     class JSValue;
>     }

I will try removing it to see if it works now. Originally this was because the derived sources need to include it, but there is no easy way to change them.

>> Source/WebCore/dom/ScriptExecutionContext.cpp:485
>> +        return downcast<WorkletGlobalScope>(*this).script()->vm();
> 
> What guarantees this won’t be null?

The only time when script() is null is after document tries to destroy it. Assuming the code is correct, no JS that could reference the WorkletGlobalScope should execute at that point, and there should not be any strong references to the WorkletGlobalScope remaining in the c++ code. As far as I can tell, if this is null then it is definitely a bug and should crash. Either way, it is not clear what the correct return value would be if script was null, since that means the vm has been destroyed.

>> Source/WebCore/platform/graphics/CustomPaintImage.cpp:121
>> +    JSC::JSValue thisObject = { JSC::construct(&state, WTFMove(paintConstructor), noArgs, "Failed to construct paint class") };
> 
> The use of WTFMove here seems excessive. JSValue has a trivial copy constructor so there should be no value in moving paintConstructor.
> 
> Can this be done by passing { } instead of noArgs?

JSC::construct takes an lvalue for some reason.

>> Source/WebCore/platform/graphics/CustomPaintImage.h:32
>> +#include <JavaScriptCore/JSObject.h>
> 
> Why does JSObject.h need to be included here?

Same as above. I will see if it works now with a forward declaration, but I originally did it because of bindings.

>> Source/WebCore/rendering/style/RenderStyle.cpp:1035
>> +#endif
> 
> This seems like it should be factored out into a function. It’s bigger than the rest of RenderStyle::changeRequiresRepaint and the rest follows a pattern where there is not so much code here. I’m also wondering why this is not handled inside the rareNonInheritedDataChangeRequiresRepaint function.
> 
> Is there a way we can do these checks with less memory allocation and deallocation?

We shouldn't need to do any, if we can ever get rid of ComputedStyleExtractor. I used ComputedStyleExtractor here because I did not want to maintain another huge switch statement, but in the future we could probably refactor most of the code from ComputedStyleExtractor to be shared.

>> Source/WebCore/worklets/PaintWorkletGlobalScope.cpp:145
>> +        responsibleDocument()->renderView()->repaintRootContents();
> 
> I think this needs a "why" comment. It’s not so obvious why this would be needed that we can just do it without explaining the reason.

Good idea. If we don't do it, then custom paint elements remain on the page as invalid images, instead of using the new paint definition. I will add a comment.

[Simon Fraser (smfr)]

Comment on attachment 355920 [details]
Patch

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

>>> Source/WebCore/worklets/PaintWorkletGlobalScope.cpp:145
>>> +        responsibleDocument()->renderView()->repaintRootContents();
>> 
>> I think this needs a "why" comment. It’s not so obvious why this would be needed that we can just do it without explaining the reason.
> 
> Good idea. If we don't do it, then custom paint elements remain on the page as invalid images, instead of using the new paint definition. I will add a comment.

This doesn't seem like the right long-term solution.