Bug 155009 – [Font Loading] Crash when a single load request causes multiple fonts to fail loading

• Component: New Bugs
• Status: RESOLVED FIXED

[Myles C. Maxfield]

[Font Loading] Crash when multiple fonts fail

[Myles C. Maxfield]

Created attachment 272826 [details]
Patch

[Myles C. Maxfield]

<rdar://problem/24865887>

[Darin Adler]

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.

[Build Bot]

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

[Build Bot]

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

[Myles C. Maxfield]

The failure is WK1-specific.

[Myles C. Maxfield]

Created attachment 272841 [details]
WIP

[Myles C. Maxfield]

Created attachment 273010 [details]
Patch

[Myles C. Maxfield]

Created attachment 273015 [details]
Patch

[Simon Fraser (smfr)]

fail to what?

[Myles C. Maxfield]

Created attachment 273022 [details]
Patch

[Simon Fraser (smfr)]

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.

[Myles C. Maxfield]

Created attachment 273038 [details]
Patch for committing

[Darin Adler]

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.

[Darin Adler]

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.

[Myles C. Maxfield]

Committed r197804: <http://trac.webkit.org/changeset/197804>