Bug 158450

Created attachment 280858 [details] WIP

Created attachment 280863 [details] WIP

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

Created attachment 280865 [details] WIP

Created attachment 280884 [details] Patch

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

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 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 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 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().

(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()!!!!

Created attachment 280941 [details] Patch for committing

Created attachment 280942 [details] Patch for committing

Comment on attachment 280942 [details] Patch for committing Clearing flags on attachment: 280942 Committed r201887: <http://trac.webkit.org/changeset/201887>

All reviewed patches have been landed. Closing bug.

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

Re-opened since this is blocked by bug 158610

(In reply to comment #16) > Seems to have caused crashes: See rdar://problem/26735116

Committed r201971: <http://trac.webkit.org/changeset/201971>

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());

Reopening to attach new patch.

Created attachment 281321 [details] Addressing Darin's Comments

Committed r202085: <http://trac.webkit.org/changeset/202085>

*** Bug 154623 has been marked as a duplicate of this bug. ***

*** Bug 154622 has been marked as a duplicate of this bug. ***