Bug 202476 - Allow pages using FontFaceSet to enter the back/forward cache
Summary: Allow pages using FontFaceSet to enter the back/forward cache
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Page Loading (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Chris Dumez
URL:
Keywords: InRadar
Depends on:
Blocks: 202293
  Show dependency treegraph
 
Reported: 2019-10-02 09:58 PDT by Chris Dumez
Modified: 2019-10-04 11:17 PDT (History)
18 users (show)

See Also:


Attachments
Patch (25.73 KB, patch)
2019-10-03 11:52 PDT, Chris Dumez
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Chris Dumez 2019-10-02 09:58:03 PDT
Allow pages using FontFaceSet to enter the back/forward cache. This causes failures on sites with Youtube embeds for example:
Comment 1 Chris Dumez 2019-10-03 11:52:44 PDT
Created attachment 380148 [details]
Patch
Comment 2 Myles C. Maxfield 2019-10-03 13:42:46 PDT
I'm also doing a little bit of additional investigation around this bug.
Comment 3 Chris Dumez 2019-10-03 14:27:28 PDT
(In reply to Myles C. Maxfield from comment #2)
> I'm also doing a little bit of additional investigation around this bug.

There are 2 issues:
1. Sometimes, the FontFace think it is loading when it actually is not (which I did not address and Myles is investigating).
2. We fail to enter page cache because the FontFace think it is still loading (which I address).

I agree that it'd be good to investigate 1 but fixing 1 would not be sufficient, because we want to enter PageCache no matter what, even if the font really is loading.
This is why I think implementing queueing in FontFaceSet like my patch does is the right thing to do.
Comment 4 Myles C. Maxfield 2019-10-03 15:42:40 PDT
Comment on attachment 380148 [details]
Patch

I've gotten a bug about this kind of behavior: https://github.com/doanythingfordethklok/safari-cache-bug but if we are really willing to use the policy of cancelling all in-flight requests during navigation, and never reloading them upon 'back', then this patch is a good one.
Comment 5 WebKit Commit Bot 2019-10-03 17:38:02 PDT
Comment on attachment 380148 [details]
Patch

Clearing flags on attachment: 380148

Committed r250693: <https://trac.webkit.org/changeset/250693>
Comment 6 WebKit Commit Bot 2019-10-03 17:38:04 PDT
All reviewed patches have been landed.  Closing bug.
Comment 7 Radar WebKit Bug Importer 2019-10-03 17:39:17 PDT
<rdar://problem/55968375>
Comment 8 Truitt Savell 2019-10-04 10:56:49 PDT
It looks like the changes in https://trac.webkit.org/changeset/250693/webkit

caused 4 new crashes on Debug:
fast/text/document-fonts-while-loading-crash.html 
fast/text/font-face-set-destroy-document.html 
fast/text/font-face-set-javascript.html 
http/tests/navigation/page-cache-fragment-referrer.html

History:
https://webkit-test-results.webkit.org/dashboards/flakiness_dashboard.html#showAllRuns=true&tests=fast%2Ftext%2Fdocument-fonts-while-loading-crash.html%20fast%2Ftext%2Ffont-face-set-destroy-document.html%20fast%2Ftext%2Ffont-face-set-javascript.html%20http%2Ftests%2Fnavigation%2Fpage-cache-fragment-referrer.html

Assertion crash:
ASSERTION FAILED: !m_valueOrException
/Volumes/Data/slave/mojave-debug/build/Source/WebCore/bindings/js/DOMPromiseProxy.h(294) : void WebCore::DOMPromiseProxyWithResolveCallback<WebCore::IDLInterface<WebCore::FontFaceSet> >::resolve(typename IDLType::ParameterType) [IDLType = WebCore::IDLInterface<WebCore::FontFaceSet>]

ASSERTION FAILED: !is<Document>(m_scriptExecutionContext) || &downcast<Document>(m_scriptExecutionContext)->contextDocument() == downcast<Document>(m_scriptExecutionContext)
Comment 9 Chris Dumez 2019-10-04 10:57:22 PDT
(In reply to Truitt Savell from comment #8)
> It looks like the changes in https://trac.webkit.org/changeset/250693/webkit
> 
> caused 4 new crashes on Debug:
> fast/text/document-fonts-while-loading-crash.html 
> fast/text/font-face-set-destroy-document.html 
> fast/text/font-face-set-javascript.html 
> http/tests/navigation/page-cache-fragment-referrer.html
> 
> History:
> https://webkit-test-results.webkit.org/dashboards/flakiness_dashboard.
> html#showAllRuns=true&tests=fast%2Ftext%2Fdocument-fonts-while-loading-crash.
> html%20fast%2Ftext%2Ffont-face-set-destroy-document.
> html%20fast%2Ftext%2Ffont-face-set-javascript.
> html%20http%2Ftests%2Fnavigation%2Fpage-cache-fragment-referrer.html
> 
> Assertion crash:
> ASSERTION FAILED: !m_valueOrException
> /Volumes/Data/slave/mojave-debug/build/Source/WebCore/bindings/js/
> DOMPromiseProxy.h(294) : void
> WebCore::DOMPromiseProxyWithResolveCallback<WebCore::IDLInterface<WebCore::
> FontFaceSet> >::resolve(typename IDLType::ParameterType) [IDLType =
> WebCore::IDLInterface<WebCore::FontFaceSet>]
> 
> ASSERTION FAILED: !is<Document>(m_scriptExecutionContext) ||
> &downcast<Document>(m_scriptExecutionContext)->contextDocument() ==
> downcast<Document>(m_scriptExecutionContext)

Looking now.
Comment 10 Chris Dumez 2019-10-04 11:17:01 PDT
(In reply to Chris Dumez from comment #9)
> (In reply to Truitt Savell from comment #8)
> > It looks like the changes in https://trac.webkit.org/changeset/250693/webkit
> > 
> > caused 4 new crashes on Debug:
> > fast/text/document-fonts-while-loading-crash.html 
> > fast/text/font-face-set-destroy-document.html 
> > fast/text/font-face-set-javascript.html 
> > http/tests/navigation/page-cache-fragment-referrer.html
> > 
> > History:
> > https://webkit-test-results.webkit.org/dashboards/flakiness_dashboard.
> > html#showAllRuns=true&tests=fast%2Ftext%2Fdocument-fonts-while-loading-crash.
> > html%20fast%2Ftext%2Ffont-face-set-destroy-document.
> > html%20fast%2Ftext%2Ffont-face-set-javascript.
> > html%20http%2Ftests%2Fnavigation%2Fpage-cache-fragment-referrer.html
> > 
> > Assertion crash:
> > ASSERTION FAILED: !m_valueOrException
> > /Volumes/Data/slave/mojave-debug/build/Source/WebCore/bindings/js/
> > DOMPromiseProxy.h(294) : void
> > WebCore::DOMPromiseProxyWithResolveCallback<WebCore::IDLInterface<WebCore::
> > FontFaceSet> >::resolve(typename IDLType::ParameterType) [IDLType =
> > WebCore::IDLInterface<WebCore::FontFaceSet>]
> > 
> > ASSERTION FAILED: !is<Document>(m_scriptExecutionContext) ||
> > &downcast<Document>(m_scriptExecutionContext)->contextDocument() ==
> > downcast<Document>(m_scriptExecutionContext)
> 
> Looking now.

Fixed in <https://trac.webkit.org/changeset/250731>.