Bug 221071 – Remove another use of FontSelector from within CSSFontFace

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Myles C. Maxfield]

Remove another use of FontSelector from within CSSFontFace

[Myles C. Maxfield]

Created attachment 418604 [details]
Patch

[Chris Lord]

Testing this locally, after rebasing, this seems to cause an infinite loop somewhere where dispatchInvalidationCallbacks will never return, possibly because of an oscillating value or a missing state increment somewhere in the CSSFontFace::font chain... I can reproduce it consistently by running the fast/css/rtl-ordering.html layout test.

This doesn't look like the same issue on EWS, so possibly I rebased incorrectly(?)

[Chris Lord]

The infinite loop is caused by re-entrance in CSSFontFace::pump due to CSSFontSelector::fontStateChanged calling dispatchInvalidationCallbacks in response in response to CSSFontFace::setState (called from pump) iterating before actually setting m_status (so pump continually sets from Loading to Success).

Changing it so m_status is set before iterating over clients in ::setState removes that infinite loop, but then causes an assertion to get hit in Resolver::~Resolver because m_document.isResolvingTreeStyle is true. I'm not sure what's going on with this last part yet...

[Chris Lord]

Created attachment 419143 [details]
Patch

[Chris Lord]

Will have to see what EWS says, but this rebased and updated patch fixes the issue I was experiencing locally.

Re-entrancy was being caused by Document::fontsNeedUpdate() being called during style resolution and causing Document::userAgentShadowTreeStyleResolver() to be called inside itself. I fixed this by adding Document::userAgentShadowTreeStyleResolverIfExists() and using that in Style::Scope::resolverIfExists() - this seems like an issue that could occur in other circumstances, so felt like a reasonable fix to me.

[Radar WebKit Bug Importer]

<rdar://problem/73959528>

[Chris Lord]

These failures are caused, I assume due to the slight difference in behaviour that means that dispatchInvalidationCallbacks is called more often - load events currently are only triggered via CSSFontFaceSource::fontLoaded, but after this patch, font status changes as a side-effect of calling CSSFontFace::font will also cause CSSFontSelector to behave as if fontLoaded had been called.

This new behaviour feels correct to me, and I wonder if ::pump is really the correct thing to call in ::font, given it has a side-effect? I don't know enough about this code to reason why it has this behaviour right now. Intuitively, I think we want to do the equivalent of pump, but without setting the status - more of a 'peek', I suppose.

[Chris Lord]

Created attachment 419380 [details]
Patch

[Chris Lord]

I'm hoping this alternative fix restores parity with previous behaviour - I removed the added Document::userAgentShadowTreeStyleResolverIfExists() and instead guarded against re-entrancy in Document::invalidateMatchedPropertiesCacheAndForceStyleRecalc using m_inStyleRecalc.
I also added an ASSERT(!m_inStyleRecalc) for good measure when creating m_userAgentShadowTreeStyleResolver to make sure that if this problem re-occurs, the cause is obvious.

[Chris Lord]

Created attachment 419385 [details]
Patch

[Chris Lord]

Ok, even less over-thinking, this just guards against re-entrancy in Document::fontsNeedUpdate and nowhere else.

[Darin Adler]

Comment on attachment 419143 [details]
Patch

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

Looks pretty good to me; had planned to set review+ assuming EWS passes on all platforms, but forgot to Publish! Here are my (days old) comments on an older version of the patch.

> Source/WebCore/css/CSSFontFaceSet.cpp:515
> +    // FIXME: Clients should know about TimedOut events so they can update their style.

Why a FIXME and not fixed in this patch? Not pushing, just wondering.

> Source/WebCore/css/CSSFontSelector.cpp:260
> +    if (newState == CSSFontFace::Status::TimedOut || newState == CSSFontFace::Status::Success || newState == CSSFontFace::Status::Failure)
>          dispatchInvalidationCallbacks();

This seems like risky code. When we use an enum, it’s typically safer to use a switch statement so the code has to be visited again if someone adds a new enumeration value. And in terms of explanatory power, seeing which new states are the ones where we don’t want to clear the cache would be helpful.

I also don’t understand why it’s best to ignore the old state when making this decision; I’m not saying it’s wrong, but I’m saying it’s not obvious enough to go without comment.

> Source/WebCore/css/CSSFontSelector.h:95
> +    void ref() final { RefCounted<FontSelector>::ref(); }
> +    void deref() final { RefCounted<FontSelector>::deref(); }

Prefer to write these as:

    void ref() final { FontSelector::ref(); }

The use of RefCounted is an implementation detail that need not be referred to directly here.

> Source/WebCore/css/CSSSegmentedFontFace.cpp:57
> +    if (newState == CSSFontFace::Status::TimedOut || newState == CSSFontFace::Status::Success || newState == CSSFontFace::Status::Failure)
> +        m_cache.clear();

This seems like risky code. When we use an enum, it’s typically safer to use a switch statement so the code has to be visited again if someone adds a new enumeration value. And in terms of explanatory power, seeing which new states are the ones where we don’t want to clear the cache would be helpful.

I also don’t understand why it’s best to ignore the old state when making this decision; I’m not saying it’s wrong, but I’m saying it’s not obvious enough to go without comment.

> Source/WebCore/css/CSSSegmentedFontFace.h:61
> +    void fontStateChanged(CSSFontFace&, CSSFontFace::Status, CSSFontFace::Status newState) final;

No good reason here to include the argument name "newState" but omit the argument name "oldState". I suggest including both or neither here.

> Source/WebCore/dom/Document.h:540
> +    Style::Resolver* userAgentShadowTreeStyleResolverIfExists() { return m_userAgentShadowTreeStyleResolver.get(); };

Extra semicolon here after the "}".

(I’m not the biggest fan of the xxxIfExists naming scheme, I prefer existingXxx, but I think we’ve discussed this and the WebKit project overall has consensus we prefer xxxIfExists.)

[Chris Lord]

Investigating the failure of fast/css/font-face-multiple-remote-sources.html, I think the new output actually reflects what's finally rendered and I assume that this happens as sometimes, prior to this patch, the font-load callback doesn't get fired when a font has loaded and so was previously missing a required invalidation call.

The final visual output in minibrowser is unchanged post-patch and also still matches Chrome, but now the render tree text output actually reflects the visual output.

Still need to investigate and see what's going on with the other 2 failures - assuming fixing either of those doesn't change the results of this, I'll update the test results.

[Chris Lord]

The other two tests are unchanged for GTK, so this makes theorising what's happening a bit tricky - The output does not match Chrome either before or after the patch. One of the failures (imported/w3c/web-platform-tests/css/css-grid/alignment/grid-self-baseline-not-applied-if-sizing-cyclic-dependency-001.html) was already a failure and now just fails with slightly different numbers, making me inclined to think it's a case of a previously missing invalidation changing the result, but the other (imported/w3c/web-platform-tests/css/css-multicol/multicol-gap-percentage-001.html) changes from a pass to a partial failure on Mac, which has me more concerned.

I have one change I'd like to try, but I'm shooting in the dark somewhat without being able to reproduce anything like this locally...

[Chris Lord]

Created attachment 419590 [details]
Patch

[Chris Lord]

Created attachment 419693 [details]
Patch

[Chris Lord]

I don't think the original patch's assertion that fontStateChanged can replace fontLoaded is true - fontLoaded is only fired for fonts with external sources and there's no way to distinguish those fonts with just a CSSFontFace, at least that I can see. This change isn't required by this patch, it can be revisited in a different bug.

I've restored the fontLoaded callback in this version of the patch, which also addresses all the review comments in https://bugs.webkit.org/attachment.cgi?id=419143&action=review

[Chris Lord]

Created attachment 419721 [details]
Patch

[Chris Lord]

Looks like the debug crash was caused by a pre-existing bug in iterateClients in CSSFontFace.cpp - unless I'm reading it wrong, it makes a copy of the clients table, but then just iterates the original table instead of the copy.

This latest revision has that fixed, so hopefully should be all green now (and maybe fixed some tricky intermittent problems elsewhere...)

[Chris Lord]

Created attachment 419730 [details]
Patch

[Darin Adler]

Comment on attachment 419730 [details]
Patch

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

> Source/WebCore/ChangeLog:9
> +        Instead of CSSFontFace directly knowing about CSSFontSelector, we can just make CSSFontSelector
> +        inherit from CSSFontFace::Client.

This makes every CSSFontFace have a set of clients with one more client in it. Those are bigger hash tables. Should be fine, but it’s going to use more memory.

> Source/WebCore/css/CSSFontFace.cpp:61
> +    for (auto client : clientsCopy)

Could save reference count churn here by using auto& instead of auto; still safe.

> Source/WebCore/css/CSSFontFace.cpp:97
> +    if (fontSelector)
> +        addClient(*fontSelector);

If the goal is to remove uses of CSSFontSelector inside CSSFontFace, I think we’d want this addClient call at the call sites, not inside this constructor.

> Source/WebCore/css/CSSFontFace.cpp:682
>  void CSSFontFace::updateStyleIfNeeded()

Function name is kind of vague. Most of our calls tell the client about something that happened, rather than telling the client what to do. Would be nice to recast this in those terms for clarity and consistency. Not necessarily in this patch.

> Source/WebCore/css/CSSFontFace.h:133
> +        virtual void updateStyleIfNeeded() { }

It’s peculiar that this callback does not take a CSSFontFace&. More efficient, but not consistent design.

[Chris Lord]

(In reply to Darin Adler from comment #21)
> Comment on attachment 419730 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=419730&action=review
> 
> > Source/WebCore/ChangeLog:9
> > +        Instead of CSSFontFace directly knowing about CSSFontSelector, we can just make CSSFontSelector
> > +        inherit from CSSFontFace::Client.
> 
> This makes every CSSFontFace have a set of clients with one more client in
> it. Those are bigger hash tables. Should be fine, but it’s going to use more
> memory.

Noted, I suppose we're trading potential maintainability/understandability for a little memory here.

> > Source/WebCore/css/CSSFontFace.cpp:61
> > +    for (auto client : clientsCopy)
> 
> Could save reference count churn here by using auto& instead of auto; still
> safe.

Will do, silly oversight on my part.

> > Source/WebCore/css/CSSFontFace.cpp:97
> > +    if (fontSelector)
> > +        addClient(*fontSelector);
> 
> If the goal is to remove uses of CSSFontSelector inside CSSFontFace, I think
> we’d want this addClient call at the call sites, not inside this constructor.

There are currently 3 call-sites, and I do think that one of them doesn't actually need this call, so this could be a nice saving (as I think one use of it may cause unnecessary style updates on the associated Document).

I suppose I'd be a little concerned that having the addClient outside of this constructor is more fragile in that the cost of doing it unnecessarily is performance, but the cost of not doing it is possibly broken functionality...

I'd be inclined to leave it as it is, but I'll seek your opinion again before cq+

> > Source/WebCore/css/CSSFontFace.cpp:682
> >  void CSSFontFace::updateStyleIfNeeded()
> 
> Function name is kind of vague. Most of our calls tell the client about
> something that happened, rather than telling the client what to do. Would be
> nice to recast this in those terms for clarity and consistency. Not
> necessarily in this patch.
> 
> > Source/WebCore/css/CSSFontFace.h:133
> > +        virtual void updateStyleIfNeeded() { }
> 
> It’s peculiar that this callback does not take a CSSFontFace&. More
> efficient, but not consistent design.

I think fontStyleUpdateNeeded would be a consistent name for this, and I agree, it should have that parameter to remain consistent.

[Darin Adler]

(In reply to Chris Lord from comment #22)
> I'd be inclined to leave it as it is, but I'll seek your opinion again
> before cq+

Seems OK to leave as is for now. More thinking longer term I guess about how we will achieve the overall goal of removing FontSelector dependencies.

[Chris Lord]

Created attachment 419964 [details]
Patch

[EWS]

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

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