[Font Loading] Crash when multiple fonts fail
Created attachment 272826 [details] Patch
<rdar://problem/24865887>
Comment on attachment 272826 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=272826&action=review > Source/WebCore/css/CSSFontFace.cpp:385 > + if (m_clients.contains(client)) What is this m_clients.contains check supposed to do? It can’t correctly handle the case where a client is deleted and removed from the set of clients, because it’s possible that the client was removed, deleted, a new object allocated at the same address, and the new object added as a client, all before the call to contains happens, falsely returning true even though it’s a different object. Making a copy and doing this contains check is a common pattern people try to use to fix this category of problem, but it’s an incorrect one. There are correct solutions to this kind of iteration, but they typically involve storing a pointer to the local temporary set being iterated in the object and having remove do removal from both sets, and other techniques like that.
Comment on attachment 272826 [details] Patch Attachment 272826 [details] did not pass mac-debug-ews (mac): Output: http://webkit-queues.webkit.org/results/920410 New failing tests: fast/text/font-face-set-document-multiple-failure.html
Created attachment 272832 [details] Archive of layout-test-results from ews116 for mac-yosemite The attached test failures were seen while running run-webkit-tests on the mac-debug-ews. Bot: ews116 Port: mac-yosemite Platform: Mac OS X 10.10.5
The failure is WK1-specific.
Created attachment 272841 [details] WIP
Created attachment 273010 [details] Patch
Created attachment 273015 [details] Patch
fail to what?
Created attachment 273022 [details] Patch
Comment on attachment 273022 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=273022&action=review > Source/WebCore/ChangeLog:18 > + * css/CSSFontFace.cpp: > + (WebCore::CSSFontFace::setStatus): > + * css/FontFaceSet.cpp: > + (WebCore::FontFaceSet::faceFinished): > + * css/FontFaceSet.h: This doesn't mention the Document::fonts() change. > Source/WebCore/css/CSSFontFace.cpp:57 > + clientsCopy.append(client); I think this can be an uncheckedAppend() > Source/WebCore/dom/Document.cpp:6686 > + updateStyleIfNeeded(); This should be explained in the changelog.
Created attachment 273038 [details] Patch for committing
Comment on attachment 273038 [details] Patch for committing View in context: https://bugs.webkit.org/attachment.cgi?id=273038&action=review > Source/WebCore/ChangeLog:9 > + In Javascript, the first promise fulfilment/failure wins. However, in C++, any > + subsequent fulfulments/failures cause a crash. JavaScript has a capital S in it. Typo: it should be "fulfillment", not "fulfilment" or "fulfulment". > Source/WebCore/css/CSSFontFace.cpp:51 > +template <typename T> No space after the word template before the "<" in our coding style. I also like putting this all one one line rather than one template line and a separate function signature line. > Source/WebCore/css/CSSFontFace.cpp:58 > + client->guardAgainstClientRegistrationChanges(); If we arranged things so that the client class had functions named ref/deref that call through to the virtual functions, then we could use a Vector<Ref<CSSFontFace::Client>> (or maybe RefPtr instead of Ref) and not have to write as much code. This is the pattern we use in EventTarget, for example. This template could be even more generic then. > Source/WebCore/css/CSSFontFace.h:115 > + virtual void guardAgainstClientRegistrationChanges() { }; > + virtual void stopGuardingAgainstClientRegistrationChanges() { }; The semicolons are unneeded/incorrect. I see the same problem on previous lines of code. > Source/WebCore/css/FontFaceSet.h:104 > + bool terminalState { false }; Not too fond of this name. It’s called terminal state, but the variable doesn’t contain a terminal state or contain a state at all. Our normal style is to name booleans with clauses that fit the form "promise <xxx>", so it could be a name like hasReachedTerminalState.
Comment on attachment 273038 [details] Patch for committing View in context: https://bugs.webkit.org/attachment.cgi?id=273038&action=review > Source/WebCore/css/FontFace.cpp:393 > + ref(); A good comment here would be something like: // This class's destructor is what removes it as a client; a ref is what it takes to prevent that from happening.
Committed r197804: <http://trac.webkit.org/changeset/197804>