Bug 208382

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.

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

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

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

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

Created attachment 392005 [details]
Patch

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

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.

Created attachment 392014 [details]
Patch

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

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?

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?

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

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

Created attachment 392015 [details]
Patch

Created attachment 392031 [details]
Patch

Comment on attachment 392031 [details]
Patch

Clearing flags on attachment: 392031

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

All reviewed patches have been landed.  Closing bug.

<rdar://problem/59910670>

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.