Bug 15602 - Quirksmode: CSS1: WebKit fails dynamic :first-letter test
: Quirksmode: CSS1: WebKit fails dynamic :first-letter test
Status: REOPENED
Product: WebKit
Classification: Unclassified
Component: CSS
: WebKit Nightly Build
: All All
: P2 Normal
Assigned To: Joone Hur
http://www.quirksmode.org/css/firstli...
: HasReduction
Depends on: 163848
Blocks: 9610
  Show dependency treegraph
 
Reported: 2007-10-21 18:18 PDT by Eric Seidel
Modified: 2016-10-22 07:56 PDT (History)
23 users (show)

See Also:


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
joone: review?
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Eric Seidel 2007-10-21 18:18:24 PDT
WebKit fails QuirksMode's dynamic :first-letter test

firefox and IE both pass.
Comment 1 mitz@webkit.org 2007-10-22 07:38:05 PDT
Duplicate of bug 14550?
Comment 2 Eric Seidel 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.
Comment 3 Nicholas Shanks 2008-02-05 05:17:24 PST
Click the link 'insert extra text' to see bug.
Comment 4 Robert Blaut 2009-01-01 02:33:50 PST
Created attachment 26347 [details]
test case
Comment 5 Raphael Kubo da Costa (:rakuco) 2013-05-16 06:59:26 PDT
*** Bug 98423 has been marked as a duplicate of this bug. ***
Comment 6 Raphael Kubo da Costa (:rakuco) 2013-05-16 07:47:02 PDT
*** Bug 94850 has been marked as a duplicate of this bug. ***
Comment 7 Igor Trindade Oliveira 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.
Comment 8 Elliott Sprehn 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.
Comment 9 Elliott Sprehn 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. :/
Comment 10 Igor Trindade Oliveira 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.
Comment 11 Raphael Kubo da Costa (:rakuco) 2013-05-17 05:43:35 PDT
+joone, who, I think, is tackling the same problem in https://codereview.chromium.org/14113040/
Comment 12 Elliott Sprehn 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?
Comment 13 Igor Trindade Oliveira 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?
Comment 14 Joone Hur 2013-09-30 03:10:47 PDT
Created attachment 212972 [details]
Patch
Comment 15 Joone Hur 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
Comment 16 Dave Hyatt 2013-10-01 15:20:18 PDT
Comment on attachment 212972 [details]
Patch

r=me
Comment 17 WebKit Commit Bot 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>
Comment 18 WebKit Commit Bot 2013-10-01 15:46:37 PDT
All reviewed patches have been landed.  Closing bug.
Comment 19 Alexey Proskuryakov 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.
Comment 20 Alexey Proskuryakov 2013-10-02 11:02:22 PDT
Made some tweaks to expectations and landed Mac results in r156773 and r156774.
Comment 21 Joone Hur 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
Comment 22 Joone Hur 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
Comment 23 David Kilzer (:ddkilzer) 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>
Comment 24 Joone Hur 2014-06-23 01:55:49 PDT
Created attachment 233588 [details]
Patch
Comment 25 Michael Catanzaro 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.
Comment 26 Joone Hur 2016-10-20 15:49:19 PDT
Created attachment 292274 [details]
Patch
Comment 27 Build Bot 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
Comment 28 Build Bot 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
Comment 29 Build Bot 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
Comment 30 Build Bot 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
Comment 31 Build Bot 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
Comment 32 Build Bot 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
Comment 33 Build Bot 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
Comment 34 Build Bot 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
Comment 35 Darin Adler 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.
Comment 36 Simon Fraser (smfr) 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.
Comment 37 Joone Hur 2016-10-21 23:27:01 PDT
Created attachment 292475 [details]
Patch
Comment 38 Build Bot 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
Comment 39 Build Bot 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
Comment 40 Build Bot 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
Comment 41 Build Bot 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
Comment 42 Build Bot 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
Comment 43 Build Bot 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
Comment 44 Build Bot 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
Comment 45 Build Bot 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
Comment 46 Joone Hur 2016-10-22 02:55:01 PDT
Created attachment 292488 [details]
Patch
Comment 47 zalan 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).