Bug 208382 – Garbage collection prevents FontFace.loaded promise from getting resolved

• Status: RESOLVED
• Resolution: FIXED
• Component: Text

[Alexey Proskuryakov]

Created attachment 391991 [details]
layout test

We have some tests that use FontFace.loaded, and they fail (time out) when frequent garbage collection is forced with JSC_slowPathAllocsBetweenGCs=10. They also fail flakily on other bots.

Steps to reproduce:

Put attached test into LayoutTests, and run as:

run-webkit-tests --no-retry --dump-render-tree --additional-env-var=JSC_slowPathAllocsBetweenGCs=10 font-promises-gc.html

It should not time out, and should say "SUCCESS".

[Alexey Proskuryakov]

Presumably the promise needs to keep FontFace wrapper alive. I am not familiar with modern GC code enough to fix this in a reasonable amount of time.

[Ryosuke Niwa]

FontFace probably needs to be an active DOM object and make it have a pending activity while there is font loading happening

[Ryosuke Niwa]

We also need to make sure the promise object itself is kept alive in each DOM wrapper world too

[Alexey Proskuryakov]

> FontFace probably needs to be an active DOM object and make it have a pending activity while there is font loading happening

I'm not sure if that would be sufficient. Doesn't promise resolving need to work even when loading isn't happening (either not started, or already finished)?

As an aside, I found that custom properties on FontFace wrappers don't survive GC either.

[Ryosuke Niwa]

(In reply to Alexey Proskuryakov from comment #4)
> > FontFace probably needs to be an active DOM object and make it have a pending activity while there is font loading happening
> 
> I'm not sure if that would be sufficient. Doesn't promise resolving need to
> work even when loading isn't happening (either not started, or already
> finished)?

Sure.

> As an aside, I found that custom properties on FontFace wrappers don't
> survive GC either.

Not surprised. Almost all DOM objects have this bug because people don't know how GC works. FontFace isn't an ActiveDOMObject nor is it reachable from anything so yeah, it'd get GC'ed immediately.

[Chris Dumez]

Created attachment 392005 [details]
Patch

[Keith Miller]

(In reply to Ryosuke Niwa from comment #5)
> (In reply to Alexey Proskuryakov from comment #4)
> > As an aside, I found that custom properties on FontFace wrappers don't
> > survive GC either.
> 
> Not surprised. Almost all DOM objects have this bug because people don't
> know how GC works. FontFace isn't an ActiveDOMObject nor is it reachable
> from anything so yeah, it'd get GC'ed immediately.

I thought wrappers were handled by the wrapper map, which is careful to not GC wrappers when it would be observable?

[Alexey Proskuryakov]

Comment on attachment 392005 [details]
Patch

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

> LayoutTests/fast/text/font-promises-gc.html:46
> +setInterval(function() {
> +    document.fonts.forEach(function(f) {
> +        console.log("Font status: " + f.status);
> +    });
> +}, 5000);

Delete this? Five seconds is hard to hit, but not impossible on a very busy bot.

[Chris Dumez]

Created attachment 392014 [details]
Patch

[Chris Dumez]

(In reply to Alexey Proskuryakov from comment #8)
> Comment on attachment 392005 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=392005&action=review
> 
> > LayoutTests/fast/text/font-promises-gc.html:46
> > +setInterval(function() {
> > +    document.fonts.forEach(function(f) {
> > +        console.log("Font status: " + f.status);
> > +    });
> > +}, 5000);
> 
> Delete this? Five seconds is hard to hit, but not impossible on a very busy
> bot.

My bad :)

[Ryosuke Niwa]

Comment on attachment 392014 [details]
Patch

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

> Source/WebCore/css/FontFace.cpp:490
> +    return !isContextStopped() && m_isScriptPotentiallyInterestedInLoad && !m_loadedPromise->isFulfilled();

Should we clear m_isScriptPotentiallyInterestedInLoad when we're fulfilled the promise?

[Chris Dumez]

Comment on attachment 392014 [details]
Patch

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

>> Source/WebCore/css/FontFace.cpp:490
>> +    return !isContextStopped() && m_isScriptPotentiallyInterestedInLoad && !m_loadedPromise->isFulfilled();
> 
> Should we clear m_isScriptPotentiallyInterestedInLoad when we're fulfilled the promise?

It would not be observable since we call m_loadedPromise->isFulfilled() here. Are you concerned about someone using this boolean for something else later?

[Ryosuke Niwa]

(In reply to Chris Dumez from comment #12)
> Comment on attachment 392014 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=392014&action=review
> 
> >> Source/WebCore/css/FontFace.cpp:490
> >> +    return !isContextStopped() && m_isScriptPotentiallyInterestedInLoad && !m_loadedPromise->isFulfilled();
> > 
> > Should we clear m_isScriptPotentiallyInterestedInLoad when we're fulfilled the promise?
> 
> It would not be observable since we call m_loadedPromise->isFulfilled()
> here. Are you concerned about someone using this boolean for something else
> later?

Yeah. Also, I also thought it's a misleading.

Maybe we can rename the variable to something like m_isPromisePotentiallyScriptObservable?

[Ryosuke Niwa]

I think the confusing part of m_isScriptPotentiallyInterestedInLoad is that it sort of implies the load hasn't happened yet.

[Chris Dumez]

Created attachment 392015 [details]
Patch

[Chris Dumez]

Created attachment 392031 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 392031 [details]
Patch

Clearing flags on attachment: 392031

Committed r257676: <https://trac.webkit.org/changeset/257676>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/59910670>

[Alexey Proskuryakov]

This also fixed fast/text/font-weight-fallback.html when running with JSC_slowPathAllocsBetweenGCs. It's using the same pattern, didn't notice it for he workaround in bug 208383.