Bug 154101 - Extend CSSFontSelector's lifetime to be longer than the Document's lifetime
Summary: Extend CSSFontSelector's lifetime to be longer than the Document's lifetime
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Myles C. Maxfield
URL:
Keywords: InRadar
Depends on: 154921
Blocks: 153346 158450
  Show dependency treegraph
 
Reported: 2016-02-10 23:45 PST by Myles C. Maxfield
Modified: 2017-06-19 14:17 PDT (History)
8 users (show)

See Also:


Attachments
WIP (8.81 KB, patch)
2016-02-10 23:45 PST, Myles C. Maxfield
no flags Details | Formatted Diff | Diff
WIP (9.85 KB, patch)
2016-02-23 21:18 PST, Myles C. Maxfield
no flags Details | Formatted Diff | Diff
WIP (11.51 KB, patch)
2016-02-23 22:17 PST, Myles C. Maxfield
no flags Details | Formatted Diff | Diff
Patch (11.80 KB, patch)
2016-02-23 22:51 PST, Myles C. Maxfield
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews117 for mac-yosemite (1.59 MB, application/zip)
2016-02-24 00:08 PST, Build Bot
no flags Details
Patch for Committing (12.61 KB, patch)
2016-02-27 21:29 PST, Myles C. Maxfield
buildbot: commit-queue-
Details | Formatted Diff | Diff
Archive of layout-test-results from ews103 for mac-yosemite (824.12 KB, application/zip)
2016-02-27 22:20 PST, Build Bot
no flags Details
Archive of layout-test-results from ews114 for mac-yosemite (890.75 KB, application/zip)
2016-02-27 22:42 PST, Build Bot
no flags Details
Patch for committing (12.65 KB, patch)
2016-02-29 22:14 PST, Myles C. Maxfield
no flags Details | Formatted Diff | Diff
Rebased patch (31.53 KB, patch)
2016-06-07 00:26 PDT, Myles C. Maxfield
no flags Details | Formatted Diff | Diff
srcset sizes fix (2.40 KB, patch)
2016-06-07 22:29 PDT, Myles C. Maxfield
no flags Details | Formatted Diff | Diff
Updated patch (13.00 KB, patch)
2016-06-07 23:04 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-02-10 23:45:38 PST
Extend CSSFontSelector's lifetime to be longer than the Document's lifetime
Comment 1 Myles C. Maxfield 2016-02-10 23:45:59 PST
Created attachment 271048 [details]
WIP
Comment 2 Myles C. Maxfield 2016-02-23 21:18:56 PST
Created attachment 272086 [details]
WIP
Comment 3 Myles C. Maxfield 2016-02-23 22:17:34 PST
Created attachment 272092 [details]
WIP
Comment 4 Myles C. Maxfield 2016-02-23 22:51:13 PST
Created attachment 272098 [details]
Patch
Comment 5 Build Bot 2016-02-24 00:08:06 PST
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
Comment 6 Build Bot 2016-02-24 00:08:08 PST
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 7 Darin Adler 2016-02-24 09:12:58 PST
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?
Comment 8 Myles C. Maxfield 2016-02-24 15:37:42 PST
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.
Comment 9 Myles C. Maxfield 2016-02-24 15:38:54 PST
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).
Comment 10 Myles C. Maxfield 2016-02-24 15:40:20 PST
If they have different lifetimes, there needs to be a discovery mechanism where a FontFace can find a CSSFontFace to route its calls to.
Comment 11 Myles C. Maxfield 2016-02-24 15:48:34 PST
It also means that newly recreated CSSFontFace objects need to discover what state they were in last. This seems difficult.
Comment 12 Myles C. Maxfield 2016-02-24 15:50:44 PST
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)
Comment 13 Myles C. Maxfield 2016-02-24 16:37:53 PST
Maybe I should partition the CSSFontFaces into "purgeable" and "nonpurgeable" and put a CSSFontFace into the "nonpurgeable" bucket when it is accessed from script.
Comment 14 Myles C. Maxfield 2016-02-25 12:50:07 PST
Adding document.ensureStyleResolver() to resolveForDocument() may be necessary in this patch.
Comment 15 Myles C. Maxfield 2016-02-27 17:54:05 PST
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 16 Myles C. Maxfield 2016-02-27 21:13:04 PST
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.
Comment 17 Myles C. Maxfield 2016-02-27 21:29:06 PST
Created attachment 272436 [details]
Patch for Committing
Comment 18 Build Bot 2016-02-27 22:20:46 PST
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
Comment 19 Build Bot 2016-02-27 22:20:49 PST
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 20 Build Bot 2016-02-27 22:42:32 PST
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
Comment 21 Build Bot 2016-02-27 22:42:36 PST
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
Comment 22 Myles C. Maxfield 2016-02-29 22:14:49 PST
Created attachment 272550 [details]
Patch for committing
Comment 23 Myles C. Maxfield 2016-03-01 18:34:43 PST
Committed r197434: <http://trac.webkit.org/changeset/197434>
Comment 24 WebKit Commit Bot 2016-03-02 10:49:16 PST
Re-opened since this is blocked by bug 154921
Comment 25 Ryan Haddad 2016-03-02 10:54:29 PST
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>
Comment 26 Myles C. Maxfield 2016-06-07 00:26:37 PDT
Created attachment 280682 [details]
Rebased patch
Comment 27 Myles C. Maxfield 2016-06-07 00:26:55 PDT
The crash can be reproduced by simply opening the following page:

<style></style>
<img sizes='1ex'>
Comment 28 Myles C. Maxfield 2016-06-07 21:55:43 PDT
(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.
Comment 29 Myles C. Maxfield 2016-06-07 22:29:33 PDT
Created attachment 280772 [details]
srcset sizes fix
Comment 30 Myles C. Maxfield 2016-06-07 23:04:49 PDT
Created attachment 280773 [details]
Updated patch
Comment 31 WebKit Commit Bot 2016-06-08 00:58:54 PDT
Comment on attachment 280773 [details]
Updated patch

Clearing flags on attachment: 280773

Committed r201799: <http://trac.webkit.org/changeset/201799>
Comment 32 WebKit Commit Bot 2016-06-08 00:58:59 PDT
All reviewed patches have been landed.  Closing bug.
Comment 33 Radar WebKit Bug Importer 2017-06-19 14:17:49 PDT
<rdar://problem/32858252>