Bug 154101

Created attachment 271048 [details] WIP

Created attachment 272086 [details] WIP

Created attachment 272092 [details] WIP

Created attachment 272098 [details] Patch

Comment on attachment 272098 [details] Patch Attachment 272098 [details] did not pass mac-debug-ews (mac): Output: http://webkit-queues.webkit.org/results/875175 New failing tests: editing/execCommand/insert-line-break-onload.html editing/editability/empty-document-justify-right.html editing/selection/selection-modify-crash.html editing/execCommand/insert-image-with-selecting-document.html editing/selection/extend-by-line-in-empty-document.html editing/editability/empty-document-stylewithcss.html editing/execCommand/delete-empty-container.html editing/execCommand/insert-html-to-document-element-crash.html imported/blink/editing/selection/contains-node-cleared-document.html editing/selection/deleteFromDocument-after-document-open-crash.html imported/w3c/web-platform-tests/html/semantics/embedded-content/the-img-element/sizes/parse-a-sizes-attribute.html editing/editability/empty-document-delete.html

Created attachment 272103 [details] Archive of layout-test-results from ews117 for mac-yosemite The attached test failures were seen while running run-webkit-tests on the mac-debug-ews. Bot: ews117 Port: mac-yosemite Platform: Mac OS X 10.10.5

Comment on attachment 272098 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=272098&action=review The design here is a little bit oblique and peculiar. I understand that there’s this state where we are changing the fonts quickly and so want to optimize by not updating m_cssFontFaceSet. But isn’t there some cleaner way to make that laziness rather than these explicit calls. The particular function which calls buildCompleted doesn’t seem clear. > Source/WebCore/css/CSSFontSelector.cpp:258 > + ASSERT(m_status == Status::Built); For maximum safety, I think this should call buildCompleted. > Source/WebCore/css/CSSFontSelector.h:91 > + enum class Status { > + Building, > + Built > + }; Something like this normally reads better on a single line. Might also be clearer as a boolean rather than am enum. > Source/WebCore/css/CSSFontSelector.h:106 > + PendingFontFaceRule(StyleRuleFontFace& styleRuleFontFace, bool isInitiatingElementInUserAgentShadowTree) > + : styleRuleFontFace(styleRuleFontFace) > + , isInitiatingElementInUserAgentShadowTree(isInitiatingElementInUserAgentShadowTree) > + { > + } Constructor seems unnecessary. Can just use struct initialization syntax. > Source/WebCore/css/CSSFontSelector.h:124 > + Status m_status { Status::Building }; Seems the status should start out as Built, not Building. I would have this be a boolean and call it something m_buildIsUnderway and have it default to false. > Source/WebCore/css/StyleResolver.cpp:289 > + // FIXME: We need a way to make sure that the first call to appendAuthorStyleSheets() is the one which > + // is rebuilding the CSSFontSelector's constituent fonts. > + document().fontSelector().buildCompleted(); Wow, this seems indirect. Is there any cleaner way to do this? Maybe we can just wait until someone calls fontRangesForFamily and do the work on demand then? Or is the earlier timing here important?

I was talking with Ryosuke about this, and I'm no longer sure that this is the best way to go. Another alternative is to make CSSFontFace and FontFace have different lifetimes.

The case Ryosuke brought up is when a page enters the page cache, we want to destroy as much as possible, including all these CSSFontFace objects (but the FontFace objects must stick around).

If they have different lifetimes, there needs to be a discovery mechanism where a FontFace can find a CSSFontFace to route its calls to.

It also means that newly recreated CSSFontFace objects need to discover what state they were in last. This seems difficult.

This state rediscovery may actually cause an illegal state transition (if, for example, the backing bytes of the font are purged from the cache, it may make a font go from Success -> Pending)

Maybe I should partition the CSSFontFaces into "purgeable" and "nonpurgeable" and put a CSSFontFace into the "nonpurgeable" bucket when it is accessed from script.

Adding document.ensureStyleResolver() to resolveForDocument() may be necessary in this patch.

Comment on attachment 272098 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=272098&action=review >> Source/WebCore/css/StyleResolver.cpp:289 >> + document().fontSelector().buildCompleted(); > > Wow, this seems indirect. Is there any cleaner way to do this? Maybe we can just wait until someone calls fontRangesForFamily and do the work on demand then? Or is the earlier timing here important? Actually, now that I look harder, it is now provable that the first call to appendAuthorStyleSheets is the one which is rebuilding the CSSFontSelector's constituent fonts.

Comment on attachment 272098 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=272098&action=review >>> Source/WebCore/css/StyleResolver.cpp:289 >>> + document().fontSelector().buildCompleted(); >> >> Wow, this seems indirect. Is there any cleaner way to do this? Maybe we can just wait until someone calls fontRangesForFamily and do the work on demand then? Or is the earlier timing here important? > > Actually, now that I look harder, it is now provable that the first call to appendAuthorStyleSheets is the one which is rebuilding the CSSFontSelector's constituent fonts. The earlier timing is important to preserve the state of the FontFace JavaScript objects.

Created attachment 272436 [details] Patch for Committing

Comment on attachment 272436 [details] Patch for Committing Attachment 272436 [details] did not pass mac-ews (mac): Output: http://webkit-queues.webkit.org/results/893855 New failing tests: editing/selection/drag-to-contenteditable-iframe.html

Created attachment 272438 [details] Archive of layout-test-results from ews103 for mac-yosemite The attached test failures were seen while running run-webkit-tests on the mac-ews. Bot: ews103 Port: mac-yosemite Platform: Mac OS X 10.10.5

Comment on attachment 272436 [details] Patch for Committing Attachment 272436 [details] did not pass mac-debug-ews (mac): Output: http://webkit-queues.webkit.org/results/893867 New failing tests: editing/selection/drag-to-contenteditable-iframe.html

Created attachment 272440 [details] Archive of layout-test-results from ews114 for mac-yosemite The attached test failures were seen while running run-webkit-tests on the mac-debug-ews. Bot: ews114 Port: mac-yosemite Platform: Mac OS X 10.10.5

Created attachment 272550 [details] Patch for committing

Committed r197434: <http://trac.webkit.org/changeset/197434>

Re-opened since this is blocked by bug 154921

This caused imported/w3c/web-platform-tests/html/semantics/embedded-content/the-img-element/sizes/parse-a-sizes-attribute.html to crash <https://build.webkit.org/results/Apple%20El%20Capitan%20Debug%20WK2%20(Tests)/r197450%20(3369)/imported/w3c/web-platform-tests/html/semantics/embedded-content/the-img-element/sizes/parse-a-sizes-attribute-crash-log.txt>

Created attachment 280682 [details] Rebased patch

The crash can be reproduced by simply opening the following page: <style></style> <img sizes='1ex'>

(In reply to comment #27) > The crash can be reproduced by simply opening the following page: > > <style></style> > <img sizes='1ex'> The lengths in the "sizes" attribute are resolved against the document's style, which is hardcoded in resolveForDocument(). The size is hardcoded to "medium" (which equates to 16) and the family is hardcoded to "-webkit-standard". Therefore, using em's and ex's as units here is useless.

Created attachment 280772 [details] srcset sizes fix

Created attachment 280773 [details] Updated patch

Comment on attachment 280773 [details] Updated patch Clearing flags on attachment: 280773 Committed r201799: <http://trac.webkit.org/changeset/201799>

All reviewed patches have been landed. Closing bug.

<rdar://problem/32858252>