| Summary: | Use references in font loading code instead of pointers which can never be null | ||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Myles C. Maxfield <mmaxfield> | ||||||||
| Component: | New Bugs | Assignee: | Myles C. Maxfield <mmaxfield> | ||||||||
| Status: | RESOLVED FIXED | ||||||||||
| Severity: | Normal | CC: | cdumez, clord, esprehn+autocc, ews-watchlist, glenn, gyuyoung.kim, macpherson, menard, rniwa, webkit-bug-importer | ||||||||
| Priority: | P2 | Keywords: | InRadar | ||||||||
| Version: | WebKit Nightly Build | ||||||||||
| Hardware: | Unspecified | ||||||||||
| OS: | Unspecified | ||||||||||
| Attachments: |
|
||||||||||
|
Description
Myles C. Maxfield
2021-06-03 21:01:11 PDT
Created attachment 430539 [details]
Patch
Comment on attachment 430539 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=430539&action=review > Source/WebCore/css/CSSFontFaceSet.cpp:121 > + Ref<CSSFontFace> face = CSSFontFace::create(*m_owningFontSelector.get(), nullptr, nullptr, true); .get() should not be needed. > Source/WebCore/css/FontFaceSet.cpp:169 > + ASSERT(scriptExecutionContext); I don't see how this is safe. FontFaceSet is an ActiveDOMObject and an ActiveDOMObject's scriptExecutionContext can definitely become null. I would imagine it is able to hit the assertion by: 1. Having the top frame get a FontFaceSet from a subframe 2. Remove that subframe from the document 3. Have the top frame call load() on that FontFaceSet (In reply to Chris Dumez from comment #2) > Comment on attachment 430539 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=430539&action=review > > > Source/WebCore/css/FontFaceSet.cpp:169 > > + ASSERT(scriptExecutionContext); > > I don't see how this is safe. FontFaceSet is an ActiveDOMObject and an > ActiveDOMObject's scriptExecutionContext can definitely become null. I would > imagine it is able to hit the assertion by: > 1. Having the top frame get a FontFaceSet from a subframe > 2. Remove that subframe from the document > 3. Have the top frame call load() on that FontFaceSet Aha! You're right. When I was reading ContextDestructionObserver I missed the implementation of the contextDestroyed() function. Created attachment 430547 [details]
Patch
Created attachment 430548 [details]
Patch
Comment on attachment 430548 [details]
Patch
r=me
Committed r278466 (238486@main): <https://commits.webkit.org/238486@main> All reviewed patches have been landed. Closing bug and clearing flags on attachment 430548 [details]. |