Bug 15602 - Quirksmode: CSS1: WebKit fails dynamic :first-letter test
: Quirksmode: CSS1: WebKit fails dynamic :first-letter test
Status: REOPENED
: WebKit
CSS
: 523.x (Safari 3)
: Macintosh Mac OS X 10.4
: P2 Normal
Assigned To:
: http://www.quirksmode.org/css/firstli...
: HasReduction
:
: 9610
  Show dependency treegraph
 
Reported: 2007-10-21 18:18 PST by
Modified: 2014-04-19 20:01 PST (History)


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 PST, Joone Hur
no flags Review Patch | Details | Formatted Diff | Diff


Note

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


Description From 2007-10-21 18:18:24 PST
WebKit fails QuirksMode's dynamic :first-letter test

firefox and IE both pass.
------- Comment #1 From 2007-10-22 07:38:05 PST -------
Duplicate of bug 14550?
------- Comment #2 From 2007-10-22 10:30:01 PST -------
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 From 2008-02-05 05:17:24 PST -------
Click the link 'insert extra text' to see bug.
------- Comment #4 From 2009-01-01 02:33:50 PST -------
Created an attachment (id=26347) [details]
test case
------- Comment #5 From 2013-05-16 06:59:26 PST -------
*** Bug 98423 has been marked as a duplicate of this bug. ***
------- Comment #6 From 2013-05-16 07:47:02 PST -------
*** Bug 94850 has been marked as a duplicate of this bug. ***
------- Comment #7 From 2013-05-16 09:25:08 PST -------
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 From 2013-05-16 09:55:33 PST -------
(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 From 2013-05-16 09:59:57 PST -------
(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 From 2013-05-16 10:03:29 PST -------
(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 From 2013-05-17 05:43:35 PST -------
+joone, who, I think, is tackling the same problem in https://codereview.chromium.org/14113040/
------- Comment #12 From 2013-05-23 15:02:19 PST -------
(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 From 2013-05-23 15:54:19 PST -------
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 From 2013-09-30 03:10:47 PST -------
Created an attachment (id=212972) [details]
Patch
------- Comment #15 From 2013-09-30 03:14:23 PST -------
(In reply to comment #14)
> Created an attachment (id=212972) [details] [details]
> Patch

This bug was fixed in Blink:
https://chromiumcodereview.appspot.com/14113040
------- Comment #16 From 2013-10-01 15:20:18 PST -------
(From update of attachment 212972 [details])
r=me
------- Comment #17 From 2013-10-01 15:46:33 PST -------
(From update of attachment 212972 [details])
Clearing flags on attachment: 212972

Committed r156742: <http://trac.webkit.org/changeset/156742>
------- Comment #18 From 2013-10-01 15:46:37 PST -------
All reviewed patches have been landed.  Closing bug.
------- Comment #19 From 2013-10-01 21:58:27 PST -------
(From update of attachment 212972 [details])
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 From 2013-10-02 11:02:22 PST -------
Made some tweaks to expectations and landed Mac results in r156773 and r156774.
------- Comment #21 From 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 From 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 From 2014-04-19 20:01:05 PST -------
(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>