WebKit fails QuirksMode's dynamic :first-letter test firefox and IE both pass.
Duplicate of bug 14550?
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.
Click the link 'insert extra text' to see bug.
Created attachment 26347 [details] test case
*** Bug 98423 has been marked as a duplicate of this bug. ***
*** Bug 94850 has been marked as a duplicate of this bug. ***
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.
(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.
(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. :/
(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.
+joone, who, I think, is tackling the same problem in https://codereview.chromium.org/14113040/
(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?
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?
Created attachment 212972 [details] Patch
(In reply to comment #14) > Created an attachment (id=212972) [details] > Patch This bug was fixed in Blink: https://chromiumcodereview.appspot.com/14113040
Comment on attachment 212972 [details] Patch r=me
Comment on attachment 212972 [details] Patch Clearing flags on attachment: 212972 Committed r156742: <http://trac.webkit.org/changeset/156742>
All reviewed patches have been landed. Closing bug.
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.
Made some tweaks to expectations and landed Mac results in r156773 and r156774.
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
(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
(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>
Created attachment 233588 [details] Patch
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.
Created attachment 292274 [details] Patch
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
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
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
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
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
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
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
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
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.
Also Zalan is trying very hard to fix layout code that mutates the render tree during layout, and this patch just adds more.
Created attachment 292475 [details] Patch
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
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
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
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
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
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
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
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
Created attachment 292488 [details] Patch
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).
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.
Created attachment 457229 [details] Rebase the patch
Created attachment 457268 [details] Update first-letter-block-change-expected files
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
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.
Created attachment 458301 [details] “Updated”
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.