Bug 158450 - Deleting a CSSOM style rule invalidates any previously-added FontFaces
Summary: Deleting a CSSOM style rule invalidates any previously-added FontFaces
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Text (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Myles C. Maxfield
URL:
Keywords:
: 154622 154623 (view as bug list)
Depends on: 154101 158610
Blocks: 153346
  Show dependency treegraph
 
Reported: 2016-06-06 17:02 PDT by Myles C. Maxfield
Modified: 2016-06-20 14:39 PDT (History)
4 users (show)

See Also:


Attachments
Reproduction (491 bytes, text/html)
2016-06-06 17:02 PDT, Myles C. Maxfield
no flags Details
WIP (8.00 KB, patch)
2016-06-08 17:28 PDT, Myles C. Maxfield
no flags Details | Formatted Diff | Diff
WIP (11.25 KB, patch)
2016-06-08 17:59 PDT, Myles C. Maxfield
no flags Details | Formatted Diff | Diff
WIP (17.88 KB, patch)
2016-06-08 18:42 PDT, Myles C. Maxfield
no flags Details | Formatted Diff | Diff
Patch (28.63 KB, patch)
2016-06-08 23:12 PDT, Myles C. Maxfield
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews113 for mac-yosemite (1.60 MB, application/zip)
2016-06-09 00:23 PDT, Build Bot
no flags Details
Patch for committing (30.82 KB, patch)
2016-06-09 13:44 PDT, Myles C. Maxfield
no flags Details | Formatted Diff | Diff
Patch for committing (32.23 KB, patch)
2016-06-09 13:59 PDT, Myles C. Maxfield
no flags Details | Formatted Diff | Diff
Addressing Darin's Comments (1.87 KB, patch)
2016-06-14 20:40 PDT, Myles C. Maxfield
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Myles C. Maxfield 2016-06-06 17:02:48 PDT
Created attachment 280649 [details]
Reproduction

See the attached test. The text should be rendered with Ahem.
Comment 1 Myles C. Maxfield 2016-06-08 17:28:04 PDT
Created attachment 280858 [details]
WIP
Comment 2 Myles C. Maxfield 2016-06-08 17:59:19 PDT
Created attachment 280863 [details]
WIP
Comment 3 Myles C. Maxfield 2016-06-08 18:04:34 PDT
The test should: (checking stuff after every step)

1. Add a font using font loading API
2. Add a font to the end using CSSOM
3. Add a font to the beginning using CSSOM
4. Remove a font using CSSOM
5. Get a font's font loading API object
6. Modify an attribute of the font using CSSOM
6. Make sure the font loading API object changed
Comment 4 Myles C. Maxfield 2016-06-08 18:42:58 PDT
Created attachment 280865 [details]
WIP
Comment 5 Myles C. Maxfield 2016-06-08 23:12:36 PDT
Created attachment 280884 [details]
Patch
Comment 6 Build Bot 2016-06-09 00:23:01 PDT
Comment on attachment 280884 [details]
Patch

Attachment 280884 [details] did not pass mac-debug-ews (mac):
Output: http://webkit-queues.webkit.org/results/1470913

New failing tests:
svg/hittest/text-with-text-node-and-content-elements.svg
fast/text/font-face-set-document.html
svg/hittest/text-with-text-path.svg
svg/hittest/text-with-multiple-tspans.svg
svg/hittest/text-with-text-node-only.svg
svg/hittest/text-dominant-baseline-hanging.svg
svg/text/text-tselect-02-f.svg
svg/hittest/text-multiple-dx-values.svg
Comment 7 Build Bot 2016-06-09 00:23:03 PDT
Created attachment 280890 [details]
Archive of layout-test-results from ews113 for mac-yosemite

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews113  Port: mac-yosemite  Platform: Mac OS X 10.10.5
Comment 8 Darin Adler 2016-06-09 10:02:52 PDT
Comment on attachment 280884 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=280884&action=review

The failure on the mac-debug EWS tester seems like it could be a real assertion failure added by this patch.

> Source/WebCore/ChangeLog:12
> +        to their StyleRuleFontFace which represents their CSS-connection. When changing a

What is a CSS-connection?

> Source/WebCore/ChangeLog:28
> +               only CSS-connected fonts which have never been touched by script are
> +               purgeable.

Why? I don’t know what a “CSS-connected font” is. And I also don’t know what it means for a font to be “touched by script”.

> Source/WebCore/css/CSSFontFace.cpp:117
> +    if (m_cssConnection)

Where does the name "CSS connection" come from? This class is already named "CSS font face", so the name "CSS connection" doesn’t make much sense to me. Is there some clearer name we can come up with for the style properties? In particular, a class in the directory named "css" with the prefix "CSS" in its name, including “CSS” in the name of a data member doesn’t seem to say anything clearly.

> Source/WebCore/css/CSSFontFace.cpp:444
> +        return Ref<FontFace>(*m_wrapper.get());

Can you omit the type name here? I would expect this to compile:

    return *m_wrapper.get();

> Source/WebCore/css/CSSFontFace.cpp:446
> +    Ref<FontFace> wrapper = FontFace::create(*this);

Should be auto.

> Source/WebCore/css/CSSFontFace.h:177
> +    bool m_touchedByScript { false };

This is a confusing concept. There is no general notion of being "touched" by a script, so the wording is ambiguous. I think it would be better to have the name here be more precise and specific. Since this prevents the font face from being purgeable, maybe that’s what the data member should be named; name it based on what it does rather than based on when it’s set. But I’d like to understand more about this. I presume the idea is that script code can observe if this is purged, so once something happens purging must not occur. But the code does not make it clear what that something is. Carefully reading the quite long comment in the change log did not answer that question for me.

> Source/WebCore/css/CSSFontFaceSet.cpp:244
> +    for (auto& face : m_faces) {
> +        if (face->cssConnection() == &target)
> +            return face.ptr();
> +    }

This seems to indicate we are using the wrong data structure. Iterating the entire set to answer this question could lead to poor performance.

> Source/WebCore/css/CSSFontFaceSet.cpp:250
> +    Vector<Ref<CSSFontFace>> toRemove;

Why aren’t these CSSFontFace* or std::reference_wrapper<CSSFontFace> instead? Is there some reason they need to be reference counted between when the vector is built and the items are removed? Does removing an item run arbitrary JavaScript code?

> Source/WebCore/css/CSSFontFaceSet.h:67
> +    CSSFontFace* containsCSSConnection(StyleRuleFontFace&);

This is named as if it’s a boolean predicate, but it doesn’t return a boolean. It should have a different name.

> Source/WebCore/css/CSSFontSelector.cpp:208
> +    if (RefPtr<CSSFontFace> existingFace = m_cssFontFaceSet->containsCSSConnection(fontFaceRule)) {

This should be auto*, not RefPtr. There is no reason to use a RefPtr here.

> Source/WebCore/css/CSSFontSelector.cpp:210
> +        if (RefPtr<FontFace> existingWrapper = existingFace->existingWrapper())

This should be auto, not RefPtr. There is no reason to use a RefPtr here.

> Source/WebCore/css/FontFace.cpp:273
> +    const_cast<CSSFontFace&>(m_backing.get()).updateStyleIfNeeded();

How did you find all the places this needs to be called? Is there a way CSSFontFace can do this that doesn’t require callers to take care to call this at the right times?

> Source/WebCore/css/FontFace.cpp:371
> +    m_backing = Ref<CSSFontFace>(newFace);

We should not need to explicitly write Ref() here. Should just be:

    m_backing = newFace;
Comment 9 Myles C. Maxfield 2016-06-09 11:58:28 PDT
Comment on attachment 280884 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=280884&action=review

>> Source/WebCore/css/CSSFontFace.cpp:117
>> +    if (m_cssConnection)
> 
> Where does the name "CSS connection" come from? This class is already named "CSS font face", so the name "CSS connection" doesn’t make much sense to me. Is there some clearer name we can come up with for the style properties? In particular, a class in the directory named "css" with the prefix "CSS" in its name, including “CSS” in the name of a data member doesn’t seem to say anything clearly.

"CSS connection" is a term used in the spec.

A FontFace JavaScript object is "CSS connected" if it represents an existing @font-face rule inside a <style> tag.

The member variable name m_cssConnection is a RefPtr to the actual StyleRuleFontFace.

I think it's a good idea to use the proper name for this

>> Source/WebCore/css/CSSFontFace.h:177
>> +    bool m_touchedByScript { false };
> 
> This is a confusing concept. There is no general notion of being "touched" by a script, so the wording is ambiguous. I think it would be better to have the name here be more precise and specific. Since this prevents the font face from being purgeable, maybe that’s what the data member should be named; name it based on what it does rather than based on when it’s set. But I’d like to understand more about this. I presume the idea is that script code can observe if this is purged, so once something happens purging must not occur. But the code does not make it clear what that something is. Carefully reading the quite long comment in the change log did not answer that question for me.

That's correct - it avoids an illegal state transition. Consider the following case:

1. The site uses an @font-face rule, and styles an element with it, so the font gets downloaded and used (successfully).
2. JavaScript on the page checks the status of the FontFace, and sees "success."
3. The user clicks a link, so the page enters the page cache. We purge the font. The new page the user is now on loads a bunch of stuff, thereby evicting the font's downloaded bytes from our caches.
4. The user clicks the back button. We lay out the page again, see the web font being used, and start downloading it again (since it's not in any caches)
5. A network error occurs
6. JavaScript on the page checks the status of the FontFace, and now sees "failure."

From the perspective of JavaScript, the FontFace appears to transition from "success" to "failure."

>> Source/WebCore/css/CSSFontSelector.cpp:208
>> +    if (RefPtr<CSSFontFace> existingFace = m_cssFontFaceSet->containsCSSConnection(fontFaceRule)) {
> 
> This should be auto*, not RefPtr. There is no reason to use a RefPtr here.

I don't think this is true. m_cssFontFaceSet might hold the last reference to the CSSFontFace. We then immediately remove it from m_cssFontFaceSet on the next line. It needs to stay alive at least throughout this block.

>> Source/WebCore/css/FontFace.cpp:273
>> +    const_cast<CSSFontFace&>(m_backing.get()).updateStyleIfNeeded();
> 
> How did you find all the places this needs to be called? Is there a way CSSFontFace can do this that doesn’t require callers to take care to call this at the right times?

These are all the property accessors of FontFace. These functions are the only callers of the implementation functions inside CSSFontFace. Calling these from JavaScript requires the updateStyle, but I think it's valuable to keep the implementation functions inside CSSFontFace to not have side-effects. The side effects should be in this upper-level object.
Comment 10 Myles C. Maxfield 2016-06-09 12:08:23 PDT
Comment on attachment 280884 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=280884&action=review

>> Source/WebCore/css/CSSFontFaceSet.cpp:244
>> +    }
> 
> This seems to indicate we are using the wrong data structure. Iterating the entire set to answer this question could lead to poor performance.

Yeah, I can keep a HashMap inside the CSSFontFaceSet and update it in ::add() and ::remove().
Comment 11 Myles C. Maxfield 2016-06-09 12:09:57 PDT
(In reply to comment #10)
> Comment on attachment 280884 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=280884&action=review
> 
> >> Source/WebCore/css/CSSFontFaceSet.cpp:244
> >> +    }
> > 
> > This seems to indicate we are using the wrong data structure. Iterating the entire set to answer this question could lead to poor performance.
> 
> Yeah, I can keep a HashMap inside the CSSFontFaceSet and update it in
> ::add() and ::remove().

And ::clear()!!!!
Comment 12 Myles C. Maxfield 2016-06-09 13:44:41 PDT
Created attachment 280941 [details]
Patch for committing
Comment 13 Myles C. Maxfield 2016-06-09 13:59:49 PDT
Created attachment 280942 [details]
Patch for committing
Comment 14 WebKit Commit Bot 2016-06-09 15:02:23 PDT
Comment on attachment 280942 [details]
Patch for committing

Clearing flags on attachment: 280942

Committed r201887: <http://trac.webkit.org/changeset/201887>
Comment 15 WebKit Commit Bot 2016-06-09 15:02:27 PDT
All reviewed patches have been landed.  Closing bug.
Comment 16 Chris Dumez 2016-06-09 22:09:51 PDT
Seems to have caused crashes:
Exception Type:        EXC_BAD_ACCESS (SIGSEGV)
Exception Codes:       KERN_INVALID_ADDRESS at 0x00000001d4db3ff0
Exception Note:        EXC_CORPSE_NOTIFY
 
CRASHING TEST: css3/font-feature-settings-rendering-expected.html

Thread 0 Crashed:: Dispatch queue: com.apple.main-thread
0   com.apple.WebCore             	0x000000010a916f92 WebCore::CSSFontFaceSet::remove(WebCore::CSSFontFace const&) + 834
1   com.apple.WebCore             	0x000000010a9172db WebCore::CSSFontFaceSet::purge() + 379
2   com.apple.WebCore             	0x000000010a91e609 WebCore::CSSFontSelector::buildStarted() + 89
3   com.apple.WebCore             	0x000000010aa0cd8f WebCore::Document::~Document() + 783
4   com.apple.WebCore             	0x000000010ac8d1be WebCore::HTMLDocument::~HTMLDocument() + 14
5   com.apple.WebCore             	0x000000010b2ee1a2 WebCore::Node::~Node() + 210
6   com.apple.WebCore             	0x000000010ac8ce1e WebCore::HTMLDivElement::~HTMLDivElement() + 14
7   JavaScriptCore                	0x000000010a0c7ec8 JSC::MarkedBlock::FreeList JSC::MarkedBlock::specializedSweep<(JSC::MarkedBlock::BlockState)3, (JSC::MarkedBlock::SweepMode)1, true>() + 184
Comment 17 WebKit Commit Bot 2016-06-09 22:20:35 PDT
Re-opened since this is blocked by bug 158610
Comment 18 Ryan Haddad 2016-06-09 22:23:51 PDT
(In reply to comment #16)
> Seems to have caused crashes:

See rdar://problem/26735116
Comment 19 Myles C. Maxfield 2016-06-11 10:51:10 PDT
Committed r201971: <http://trac.webkit.org/changeset/201971>
Comment 20 Darin Adler 2016-06-11 12:00:21 PDT
Comment on attachment 280942 [details]
Patch for committing

View in context: https://bugs.webkit.org/attachment.cgi?id=280942&action=review

> Source/WebCore/css/CSSFontFaceSet.cpp:194
> +        auto addResult = m_constituentCSSConnections.add(face.cssConnection(), &face);
> +        ASSERT_UNUSED(addResult, addResult.isNewEntry);

There’s a more straightforward way to write this that we might prefer in the future, given that we don’t have a strong need to minimize the number of hash table lookups done in assertions:

    ASSERT(!m_constituentCSSConnections.contains(face.cssConnection()));
    m_constituentCSSConnections.add(face.cssConnection(), &face);

> Source/WebCore/css/CSSFontFaceSet.cpp:233
> +        bool removed = m_constituentCSSConnections.remove(face.cssConnection());
> +        ASSERT_UNUSED(removed, removed);

There’s a more straightforward way to write this that we might prefer in the future, given that we don’t have a strong need to minimize the number of hash table lookups done in assertions. Also, it’s a stronger assertion that can catch other kinds of errors:

    ASSERT(m_constituentCSSConnections.get(face.cssConnection()) == &face);
    m_constituentCSSConnections.remove(face.cssConnection());
Comment 21 Myles C. Maxfield 2016-06-14 20:40:37 PDT
Reopening to attach new patch.
Comment 22 Myles C. Maxfield 2016-06-14 20:40:39 PDT
Created attachment 281321 [details]
Addressing Darin's Comments
Comment 23 Myles C. Maxfield 2016-06-14 23:35:28 PDT
Committed r202085: <http://trac.webkit.org/changeset/202085>
Comment 24 Myles C. Maxfield 2016-06-20 14:39:11 PDT
*** Bug 154623 has been marked as a duplicate of this bug. ***
Comment 25 Myles C. Maxfield 2016-06-20 14:39:39 PDT
*** Bug 154622 has been marked as a duplicate of this bug. ***