Bug 15602

Summary:

Quirksmode: CSS1: WebKit fails dynamic :first-letter test

Product:

WebKit

Reporter:

Eric Seidel (no email) <eric>

Component:

CSS

Assignee:

Joone Hur <joone>

Status:

REOPENED

Severity:

Normal

CC:

ahmad.saleem792, beidson, buildbot, bunhere, cdumez, changseok, commit-queue, darin, dbates, ddkilzer, esprehn+autocc, esprehn, ews-watchlist, glenn, gyuyoung.kim, hyatt, igor.oliveira, jonlee, joone, joshua.lock, kondapallykalyan, mitz, pdr, rakuco, rniwa, sergio, simon.fraser, webkit, zalan

Priority:

Keywords:

HasReduction

Version:

WebKit Nightly Build

Hardware:

All

OS:

All

URL:

http://www.quirksmode.org/css/firstline.html

Bug Depends on:

163848

Bug Blocks:

9610

Attachments:

Description	Flags
test case	none
Patch	none
Patch	none
Patch	none
Archive of layout-test-results from ews103 for mac-yosemite	none
Archive of layout-test-results from ews117 for mac-yosemite	none
Archive of layout-test-results from ews124 for ios-simulator-wk2	none
Archive of layout-test-results from ews107 for mac-yosemite-wk2	none
Patch	none
Archive of layout-test-results from ews105 for mac-yosemite-wk2	none
Archive of layout-test-results from ews103 for mac-yosemite	none
Archive of layout-test-results from ews112 for mac-yosemite	none
Archive of layout-test-results from ews124 for ios-simulator-wk2	none
Patch	none
Rebase the patch	none
Update first-letter-block-change-expected files	none
“Updated”	ews-feeder: commit-queue-

Eric Seidel (no email)

Reported 2007-10-21 18:18:24 PDT

WebKit fails QuirksMode's dynamic :first-letter test firefox and IE both pass.

Attachments
test case (1.81 KB, text/html) 2009-01-01 02:33 PST, Robert Blaut	no flags	Details
Patch (24.44 KB, patch) 2013-09-30 03:10 PDT, Joone Hur	no flags	Details Formatted Diff Diff
Patch (19.32 KB, patch) 2014-06-23 01:55 PDT, Joone Hur	no flags	Details Formatted Diff Diff
Patch (20.24 KB, patch) 2016-10-20 15:49 PDT, Joone Hur	no flags	Details Formatted Diff Diff
Archive of layout-test-results from ews103 for mac-yosemite (1.47 MB, application/zip) 2016-10-20 16:38 PDT, Build Bot	no flags	Details
Archive of layout-test-results from ews117 for mac-yosemite (1.91 MB, application/zip) 2016-10-20 16:45 PDT, Build Bot	no flags	Details
Archive of layout-test-results from ews124 for ios-simulator-wk2 (18.17 MB, application/zip) 2016-10-20 16:46 PDT, Build Bot	no flags	Details
Archive of layout-test-results from ews107 for mac-yosemite-wk2 (1.33 MB, application/zip) 2016-10-20 16:52 PDT, Build Bot	no flags	Details
Patch (20.24 KB, patch) 2016-10-21 23:27 PDT, Joone Hur	no flags	Details Formatted Diff Diff
Archive of layout-test-results from ews105 for mac-yosemite-wk2 (1.50 MB, application/zip) 2016-10-22 00:22 PDT, Build Bot	no flags	Details
Archive of layout-test-results from ews103 for mac-yosemite (1.21 MB, application/zip) 2016-10-22 00:26 PDT, Build Bot	no flags	Details
Archive of layout-test-results from ews112 for mac-yosemite (1.80 MB, application/zip) 2016-10-22 00:37 PDT, Build Bot	no flags	Details
Archive of layout-test-results from ews124 for ios-simulator-wk2 (9.66 MB, application/zip) 2016-10-22 00:42 PDT, Build Bot	no flags	Details
Patch (20.48 KB, patch) 2016-10-22 02:55 PDT, Joone Hur	no flags	Details Formatted Diff Diff
Rebase the patch (38.62 KB, patch) 2022-04-10 21:06 PDT, Joone Hur	no flags	Details Formatted Diff Diff
Update first-letter-block-change-expected files (23.49 KB, patch) 2022-04-11 10:07 PDT, Joone Hur	no flags	Details Formatted Diff Diff
“Updated” (23.34 KB, patch) 2022-04-25 14:59 PDT, Joone Hur	ews-feeder: commit-queue-	Details Formatted Diff Diff
Show Obsolete (15) View All Add attachment proposed patch, testcase, etc.

mitz

Comment 1 2007-10-22 07:38:05 PDT

Duplicate of bug 14550?

Eric Seidel (no email)

Comment 2 2007-10-22 10:30:01 PDT

Not sure if it's a dup of 14550, but it might be the same root cause. In this bug, it looks like the (likely anonymous) renderer for :firstletter is just forgotten about.

Nicholas Shanks

Comment 3 2008-02-05 05:17:24 PST

Click the link 'insert extra text' to see bug.

Robert Blaut

Comment 4 2009-01-01 02:33:50 PST

Created attachment 26347 [details] test case

Raphael Kubo da Costa (:rakuco)

Comment 5 2013-05-16 06:59:26 PDT

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

Raphael Kubo da Costa (:rakuco)

Comment 6 2013-05-16 07:47:02 PDT

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

Igor Trindade Oliveira

Comment 7 2013-05-16 09:25:08 PDT

I am taking this one, I already have an initial patch for https://bugs.webkit.org/show_bug.cgi?id=94850, and it also fixes this bug. Basically I am creating the first-letter when RenderBlock::addChild, RenderInline::addChild(RenderInline::addChild is trick because PseudoElement::attach has a reference to the first-letter and createFirstLetter destroys the ptr, so PseudoElement can have an invalid pointer) and RenderBlock::styleDidChange. I am not using PseudoElement yet because i am concerned about performance.

Elliott Sprehn

Comment 8 2013-05-16 09:55:33 PDT

(In reply to comment #7) > I am taking this one, I already have an initial patch for https://bugs.webkit.org/show_bug.cgi?id=94850, and it also fixes this bug. > > Basically I am creating the first-letter when RenderBlock::addChild, RenderInline::addChild(RenderInline::addChild is trick because PseudoElement::attach has a reference to the first-letter and createFirstLetter destroys the ptr, so PseudoElement can have an invalid pointer) and RenderBlock::styleDidChange. That seems weird. How did we end up with an invalid ptr in PseudoElement::attach? If the renderer was destroyed we should have done setRenderer(0) on the PseudoElement. > > I am not using PseudoElement yet because i am concerned about performance. I wouldn't be concerned about the performance of PseudoElement. It's plenty fast, and pages that use :first-letter are very rare. Unless you have a profile that shows this is a bottle neck on real page loading it shouldn't be an issue.

Elliott Sprehn

Comment 9 2013-05-16 09:59:57 PDT

(In reply to comment #8) > (In reply to comment #7) > > I am taking this one, I already have an initial patch for https://bugs.webkit.org/show_bug.cgi?id=94850, and it also fixes this bug. > > > > Basically I am creating the first-letter when RenderBlock::addChild, RenderInline::addChild(RenderInline::addChild is trick because PseudoElement::attach has a reference to the first-letter and createFirstLetter destroys the ptr, so PseudoElement can have an invalid pointer) and RenderBlock::styleDidChange. > > That seems weird. How did we end up with an invalid ptr in PseudoElement::attach? If the renderer was destroyed we should have done setRenderer(0) on the PseudoElement. > I see where this happens, PseudoElement::attach has: renderer->addChild(child); if (child->isQuote()) ... And the addChild call destroys the child. :/

Igor Trindade Oliveira

Comment 10 2013-05-16 10:03:29 PDT

(In reply to comment #9) > (In reply to comment #8) > > (In reply to comment #7) > > > I am taking this one, I already have an initial patch for https://bugs.webkit.org/show_bug.cgi?id=94850, and it also fixes this bug. > > > > > > Basically I am creating the first-letter when RenderBlock::addChild, RenderInline::addChild(RenderInline::addChild is trick because PseudoElement::attach has a reference to the first-letter and createFirstLetter destroys the ptr, so PseudoElement can have an invalid pointer) and RenderBlock::styleDidChange. > > > > That seems weird. How did we end up with an invalid ptr in PseudoElement::attach? If the renderer was destroyed we should have done setRenderer(0) on the PseudoElement. > > > > I see where this happens, PseudoElement::attach has: > > renderer->addChild(child); > if (child->isQuote()) > ... > > And the addChild call destroys the child. :/ Exactly. About the performance issue, i can give a try in PseudoElement. In the end, it is the best approach.

Raphael Kubo da Costa (:rakuco)

Comment 11 2013-05-17 05:43:35 PDT

+joone, who, I think, is tackling the same problem in https://codereview.chromium.org/14113040/

Elliott Sprehn

Comment 12 2013-05-23 15:02:19 PDT

(In reply to comment #10) > ... > > Exactly. > > About the performance issue, i can give a try in PseudoElement. In the end, it is the best approach. I was looking at this more today, and we never call updateFirstLetter() inside addChild(). Where do you see the renderer get destroyed so the loop in PseudoElement::attach is confused?

Igor Trindade Oliveira

Comment 13 2013-05-23 15:54:19 PDT

It is not right now. I was working on a patch to move out first-letter creation from RenderBlock::layout to RenderBlock/Inline ::addChild. (In reply to comment #12) > (In reply to comment #10) > > ... > > > > Exactly. > > > > About the performance issue, i can give a try in PseudoElement. In the end, it is the best approach. > > I was looking at this more today, and we never call updateFirstLetter() inside addChild(). Where do you see the renderer get destroyed so the loop in PseudoElement::attach is confused?

Joone Hur

Comment 14 2013-09-30 03:10:47 PDT

Created attachment 212972 [details] Patch

Joone Hur

Comment 15 2013-09-30 03:14:23 PDT

(In reply to comment #14) > Created an attachment (id=212972) [details] > Patch This bug was fixed in Blink: https://chromiumcodereview.appspot.com/14113040

Dave Hyatt

Comment 16 2013-10-01 15:20:18 PDT

Comment on attachment 212972 [details] Patch r=me

WebKit Commit Bot

Comment 17 2013-10-01 15:46:33 PDT

Comment on attachment 212972 [details] Patch Clearing flags on attachment: 212972 Committed r156742: <http://trac.webkit.org/changeset/156742>

WebKit Commit Bot

Comment 18 2013-10-01 15:46:37 PDT

All reviewed patches have been landed. Closing bug.

Alexey Proskuryakov

Comment 19 2013-10-01 21:58:27 PDT

Comment on attachment 212972 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=212972&action=review > LayoutTests/platform/mac/TestExpectations:1379 > +# This test case needs to be rebaselined. Please do.

Alexey Proskuryakov

Comment 20 2013-10-02 11:02:22 PDT

Made some tweaks to expectations and landed Mac results in r156773 and r156774.

Joone Hur

Comment 21 2013-12-29 07:45:04 PST

The same fix was reverted from Blink due to Heap-use-after-free on ClusterFuzz, so we need to revert this patch, just in case. https://codereview.chromium.org/102993011

Joone Hur

Comment 22 2013-12-29 08:24:25 PST

(In reply to comment #21) > The same fix was reverted from Blink due to Heap-use-after-free on ClusterFuzz, so we need to revert this patch, just in case. > > https://codereview.chromium.org/102993011 Here is a patch: https://bugs.webkit.org/show_bug.cgi?id=126275

David Kilzer (:ddkilzer)

Comment 23 2014-04-19 20:01:05 PDT

(In reply to comment #22) > (In reply to comment #21) > > The same fix was reverted from Blink due to Heap-use-after-free on ClusterFuzz, so we need to revert this patch, just in case. > > > > https://codereview.chromium.org/102993011 > > Here is a patch: https://bugs.webkit.org/show_bug.cgi?id=126275 Reverted in r161137: <http://trac.webkit.org/changeset/161137>

Joone Hur

Comment 24 2014-06-23 01:55:49 PDT

Created attachment 233588 [details] Patch

Michael Catanzaro

Comment 25 2016-09-17 07:12:02 PDT

Comment on attachment 233588 [details] Patch Hi, Apologies that your patch was not reviewed in a timely manner. Since it's now quite old, I am removing it from the review request queue. Please consider rebasing it on trunk and resubmitting. To increase the chances of getting a review, consider using 'Tools/Scripts/webkit-patch upload --suggest-reviewers' to CC reviewers who might be interested in this bug.

Joone Hur

Comment 26 2016-10-20 15:49:19 PDT

Created attachment 292274 [details] Patch

Build Bot

Comment 27 2016-10-20 16:38:35 PDT

Comment on attachment 292274 [details] Patch Attachment 292274 [details] did not pass mac-ews (mac): Output: http://webkit-queues.webkit.org/results/2334044 New failing tests: imported/blink/fast/pagination/first-letter-inherit-all-crash.html

Build Bot

Comment 28 2016-10-20 16:38:41 PDT

Created attachment 292279 [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

Build Bot

Comment 29 2016-10-20 16:45:18 PDT

Comment on attachment 292274 [details] Patch Attachment 292274 [details] did not pass mac-debug-ews (mac): Output: http://webkit-queues.webkit.org/results/2334043 New failing tests: imported/blink/fast/pagination/first-letter-inherit-all-crash.html

Build Bot

Comment 30 2016-10-20 16:45:23 PDT

Created attachment 292281 [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

Build Bot

Comment 31 2016-10-20 16:46:18 PDT

Comment on attachment 292274 [details] Patch Attachment 292274 [details] did not pass ios-sim-ews (ios-simulator-wk2): Output: http://webkit-queues.webkit.org/results/2334032 New failing tests: fast/css/first-letter-block-change.html imported/blink/fast/pagination/first-letter-inherit-all-crash.html

Build Bot

Comment 32 2016-10-20 16:46:25 PDT

Created attachment 292282 [details] Archive of layout-test-results from ews124 for ios-simulator-wk2 The attached test failures were seen while running run-webkit-tests on the ios-sim-ews. Bot: ews124 Port: ios-simulator-wk2 Platform: Mac OS X 10.11.6

Build Bot

Comment 33 2016-10-20 16:52:27 PDT

Comment on attachment 292274 [details] Patch Attachment 292274 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.webkit.org/results/2334111 New failing tests: imported/blink/fast/pagination/first-letter-inherit-all-crash.html

Build Bot

Comment 34 2016-10-20 16:52:33 PDT

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

Darin Adler

Comment 35 2016-10-21 11:43:54 PDT

Comment on attachment 292274 [details] Patch This test is crashing: imported/blink/fast/pagination/first-letter-inherit-all-crash.html [ Crash ] Please test it locally to find out why and fix it before uploading a new patch.

Simon Fraser (smfr)

Comment 36 2016-10-21 11:54:15 PDT

Also Zalan is trying very hard to fix layout code that mutates the render tree during layout, and this patch just adds more.

Joone Hur

Comment 37 2016-10-21 23:27:01 PDT

Created attachment 292475 [details] Patch

Build Bot

Comment 38 2016-10-22 00:22:08 PDT

Comment on attachment 292475 [details] Patch Attachment 292475 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.webkit.org/results/2343204 New failing tests: fast/css/first-letter-block-change.html imported/blink/fast/pagination/first-letter-inherit-all-crash.html

Build Bot

Comment 39 2016-10-22 00:22:15 PDT

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

Build Bot

Comment 40 2016-10-22 00:26:30 PDT

Comment on attachment 292475 [details] Patch Attachment 292475 [details] did not pass mac-ews (mac): Output: http://webkit-queues.webkit.org/results/2343221 New failing tests: fast/css/first-letter-block-change.html imported/blink/fast/pagination/first-letter-inherit-all-crash.html

Build Bot

Comment 41 2016-10-22 00:26:36 PDT

Created attachment 292482 [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

Build Bot

Comment 42 2016-10-22 00:37:39 PDT

Comment on attachment 292475 [details] Patch Attachment 292475 [details] did not pass mac-debug-ews (mac): Output: http://webkit-queues.webkit.org/results/2343236 New failing tests: fast/css/first-letter-block-change.html imported/blink/fast/pagination/first-letter-inherit-all-crash.html

Build Bot

Comment 43 2016-10-22 00:37:45 PDT

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

Build Bot

Comment 44 2016-10-22 00:41:56 PDT

Comment on attachment 292475 [details] Patch Attachment 292475 [details] did not pass ios-sim-ews (ios-simulator-wk2): Output: http://webkit-queues.webkit.org/results/2343231 New failing tests: fast/css/first-letter-block-change.html imported/blink/fast/pagination/first-letter-inherit-all-crash.html

Build Bot

Comment 45 2016-10-22 00:42:02 PDT

Created attachment 292485 [details] Archive of layout-test-results from ews124 for ios-simulator-wk2 The attached test failures were seen while running run-webkit-tests on the ios-sim-ews. Bot: ews124 Port: ios-simulator-wk2 Platform: Mac OS X 10.11.6

Joone Hur

Comment 46 2016-10-22 02:55:01 PDT

Created attachment 292488 [details] Patch

zalan

Comment 47 2016-10-22 07:56:01 PDT

We've learned in the past that mutating the render tree during layout could lead to use-after-free type of security issues. Even if the logic here is correct, I'd hold off landing this patch until after bug 163848 is fixed (this change ought to be done soon after the text renderer insertion).

Brady Eidson

Comment 48 2018-02-14 10:35:37 PST

Comment on attachment 292488 [details] Patch Patches that have been up for review since 2016 are almost certainly too stale to be relevant to trunk in their current form. If this patch is still important please rebase it and post it for review again.

Joone Hur

Comment 49 2022-04-10 21:06:02 PDT

Created attachment 457229 [details] Rebase the patch

Joone Hur

Comment 50 2022-04-11 10:07:47 PDT

Created attachment 457268 [details] Update first-letter-block-change-expected files

Darin Adler

Comment 51 2022-04-11 14:22:26 PDT

Comment on attachment 457268 [details] Update first-letter-block-change-expected files View in context: https://bugs.webkit.org/attachment.cgi?id=457268&action=review > Source/WebCore/rendering/updating/RenderTreeBuilderFirstLetter.cpp:171 > + RenderElement* firstLetter = firstLetterText.parent(); auto* > Source/WebCore/rendering/updating/RenderTreeBuilderFirstLetter.cpp:174 > + RenderElement* firstLetterContainer = firstLetter->parent(); auto* But also, this local variable doesn’t need to exist. We only use it one place below and we could write firstLetter->parent() there. Unless it’s here because we want to write assertions? > Source/WebCore/rendering/updating/RenderTreeBuilderFirstLetter.cpp:175 > + ASSERT(firstLetter->isFloating() || firstLetter->isInline()); Can we move this ASSERT up? It’s asserting things about firstLetter, not firstLettterContainer. > Source/WebCore/rendering/updating/RenderTreeBuilderFirstLetter.cpp:177 > + // Check if the first letter was part of the remaning text. If not, Typo here: "remaning". > Source/WebCore/rendering/updating/RenderTreeBuilderFirstLetter.cpp:179 > + RenderObject* remainingText = firstLetter->nextSibling(); auto* > Source/WebCore/rendering/updating/RenderTreeBuilderFirstLetter.cpp:180 > + if (remainingText && firstLetterText.node() != remainingText->node()) { Maybe we could do early return instead of nesting for this? We use early return for what’s next. > Source/WebCore/rendering/updating/RenderTreeBuilderFirstLetter.cpp:184 > + if (RenderTextFragment* oldRemainingText = downcast<RenderBoxModelObject>(*firstLetter).firstLetterRemainingText()) { auto* And consider early return. > Source/WebCore/rendering/updating/RenderTreeBuilderFirstLetter.cpp:186 > + // Destroy the RenderTextFragment object that has the remaining text, > + // and create a RenderText with the original text recovered from the corresponding DOM node. This comment seems to be a line early. > Source/WebCore/rendering/updating/RenderTreeBuilderFirstLetter.cpp:187 > + if (Text* text = oldRemainingText->textNode()) { auto* And consider early return. > Source/WebCore/rendering/updating/RenderTreeBuilderFirstLetter.cpp:188 > + RenderPtr<RenderText> originalText = createRenderer<RenderText>(*text, text->data()); auto

Joone Hur

Comment 52 2022-04-25 14:54:53 PDT

Comment on attachment 457268 [details] Update first-letter-block-change-expected files View in context: https://bugs.webkit.org/attachment.cgi?id=457268&action=review >> Source/WebCore/rendering/updating/RenderTreeBuilderFirstLetter.cpp:171 >> + RenderElement* firstLetter = firstLetterText.parent(); > > auto* Done. >> Source/WebCore/rendering/updating/RenderTreeBuilderFirstLetter.cpp:174 >> + RenderElement* firstLetterContainer = firstLetter->parent(); > > auto* > > But also, this local variable doesn’t need to exist. We only use it one place below and we could write firstLetter->parent() there. Unless it’s here because we want to write assertions? Removed this line. >> Source/WebCore/rendering/updating/RenderTreeBuilderFirstLetter.cpp:175 >> + ASSERT(firstLetter->isFloating() || firstLetter->isInline()); > > Can we move this ASSERT up? It’s asserting things about firstLetter, not firstLettterContainer. Done. >> Source/WebCore/rendering/updating/RenderTreeBuilderFirstLetter.cpp:177 >> + // Check if the first letter was part of the remaning text. If not, > > Typo here: "remaning". Done. >> Source/WebCore/rendering/updating/RenderTreeBuilderFirstLetter.cpp:179 >> + RenderObject* remainingText = firstLetter->nextSibling(); > > auto* Done. >> Source/WebCore/rendering/updating/RenderTreeBuilderFirstLetter.cpp:180 >> + if (remainingText && firstLetterText.node() != remainingText->node()) { > > Maybe we could do early return instead of nesting for this? We use early return for what’s next. Done. >> Source/WebCore/rendering/updating/RenderTreeBuilderFirstLetter.cpp:184 >> + if (RenderTextFragment* oldRemainingText = downcast<RenderBoxModelObject>(*firstLetter).firstLetterRemainingText()) { > > auto* > > And consider early return. Done. >> Source/WebCore/rendering/updating/RenderTreeBuilderFirstLetter.cpp:186 >> + // and create a RenderText with the original text recovered from the corresponding DOM node. > > This comment seems to be a line early. Done. >> Source/WebCore/rendering/updating/RenderTreeBuilderFirstLetter.cpp:187 >> + if (Text* text = oldRemainingText->textNode()) { > > auto* > > And consider early return. Done. >> Source/WebCore/rendering/updating/RenderTreeBuilderFirstLetter.cpp:188 >> + RenderPtr<RenderText> originalText = createRenderer<RenderText>(*text, text->data()); > > auto Done.

Joone Hur

Comment 53 2022-04-25 14:59:41 PDT

Created attachment 458301 [details] “Updated”

Joone Hur

Comment 54 2022-04-25 15:19:40 PDT

Hi, Darin Thank you for the review. I'm trying to fix the test failures so I will ask you review my patch again after fixing them.

Note You need to log in before you can comment on or make changes to this bug.