Last Comment Bug 367650 - [FIX]"ASSERTION: should only be called for text frames" with counters, :first-letter
: [FIX]"ASSERTION: should only be called for text frames" with counters, :first...
: assertion, fixed1.8.1.8, testcase
Product: Core
Classification: Components
Component: CSS Parsing and Computation (show other bugs)
: Trunk
: All All
: P1 normal (vote)
: mozilla1.9alpha1
Assigned To: Boris Zbarsky [:bz] (TPAC)
: Hixie (not reading bugmail)
Depends on: 372550
Blocks: randomclasses 229764 230611 362901 368451 1198607
  Show dependency treegraph
Reported: 2007-01-21 06:22 PST by Jesse Ruderman
Modified: 2015-08-26 00:09 PDT (History)
6 users (show)
dveditz: blocking1.8.1.5-
dveditz: blocking1.8.0.13-
bzbarsky: in‑testsuite+
See Also:
Crash Signature:
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---

testcase (679 bytes, text/html)
2007-01-21 06:22 PST, Jesse Ruderman
no flags Details
stack trace for the assertion (6.92 KB, text/plain)
2007-01-21 14:55 PST, Jesse Ruderman
no flags Details
Testcase that's guaranteed to assert (344 bytes, text/html)
2007-01-21 15:35 PST, Boris Zbarsky [:bz] (TPAC)
no flags Details
Proposed fix (10.60 KB, patch)
2007-02-03 00:37 PST, Boris Zbarsky [:bz] (TPAC)
no flags Details | Diff | Splinter Review
With that change (28.66 KB, patch)
2007-02-27 21:06 PST, Boris Zbarsky [:bz] (TPAC)
roc: review+
roc: superreview+
Details | Diff | Splinter Review

Description Jesse Ruderman 2007-01-21 06:22:19 PST
Created attachment 252209 [details]

Loading this testcase triggers the following assertion about 50% of the time:

###!!! ASSERTION: should only be called for text frames: 'Not Reached', file /Users/admin/trunk/mozilla/layout/generic/nsFrame.cpp, line 3273
Comment 1 Jesse Ruderman 2007-01-21 06:29:26 PST
nsFrame::CharacterDataChanged is what doesn't want to be called.
Comment 2 Boris Zbarsky [:bz] (TPAC) 2007-01-21 11:55:01 PST
So... I can't seem to reproduce this.  I assume you're using a build in which bug 367243 is fixed?

If so, what's the concrete frame class when the unwanted CharacterDataChanged is called?
Comment 3 Jesse Ruderman 2007-01-21 14:54:43 PST
Yes, I'm using a build in which bug 367243 is fixed.

(gdb) f 3
#3  0x084c686c in nsFrame::CharacterDataChanged (this=0x2f2518c, aPresContext=0x268147d0, aChild=0x2681d240, aAppend=0) at /Users/admin/trunk/mozilla/layout/generic/nsFrame.cpp:3273
3273      NS_NOTREACHED("should only be called for text frames");

(gdb) whats this
0x8ec6ee0 <_ZTV18nsFirstLetterFrame+8>: 0x84c3530 <_ZN7nsFrame14QueryInterfaceERK4nsIDPPv>
Comment 4 Jesse Ruderman 2007-01-21 14:55:30 PST
Created attachment 252260 [details]
stack trace for the assertion
Comment 5 Boris Zbarsky [:bz] (TPAC) 2007-01-21 15:35:29 PST
Created attachment 252262 [details]
Testcase that's guaranteed to assert

This is sort of a combination of bug 230611 and bug 362901...
Comment 6 Boris Zbarsky [:bz] (TPAC) 2007-01-21 15:37:44 PST
So I was wondering....  Should we be setting the _block_ as mContent on the nsFirstLetterFrame?  That way it wouldn't end up as the primary frame for the text content.
Comment 7 Robert O'Callahan (:roc) (email my personal email if necessary) 2007-01-21 17:28:03 PST
That's probably a good idea. But maybe we need a better story for how we assign and find frames when we create a stack of frames for an element ... like maybe GetPrimaryFrameFor should take a parameter that controls which frame in a set we should return. E.g. possible values
-- "outermost" (the frame whose parent is associated with a different element)
-- "innermost" (the frame whose children are associated with different elements)
-- "styleholder" (the frame whose style context has the style for the element)
-- "replaced" (the special frame that implements the functionality of a replaced element)
hmm, any others?
Comment 8 Boris Zbarsky [:bz] (TPAC) 2007-02-03 00:37:12 PST
Created attachment 253847 [details] [diff] [review]
Proposed fix

This fixes this bug and bug 368451.  The frame constructor changes are really enough to fix this bug.  The nsContainerFrame changes fix bug 368451.  The rest are assertions that I added as I was working on this to make sure that I didn't end up with textnodes in unexpected places, etc.
Comment 9 Boris Zbarsky [:bz] (TPAC) 2007-02-04 17:26:47 PST
That fix doesn't change our GetPrimaryFor logic, by the way.

I do think it would be nice to have a better way to get the various frames mapping to a content node...  Unfortunately, I'm not sure that there is always a unique "innermost" frame (e.g. comboboxes have a display frame and the dropdown), and so forth.  We should spin that off into a separate bug.
Comment 10 Boris Zbarsky [:bz] (TPAC) 2007-02-26 20:15:27 PST
Comment on attachment 253847 [details] [diff] [review]
Proposed fix

roc, do you think you can review this instead?
Comment 11 Robert O'Callahan (:roc) (email my personal email if necessary) 2007-02-26 20:59:41 PST
We should document this decision somewhere.

We have an invariant right now that if frame F is an ancestor of frame G, then F->mContent is an ancestor of (or equal to) G->mContent. (Well, I can't think of any counterexamples.) This breaks that. I don't know how scared we should be about that.
Comment 12 Boris Zbarsky [:bz] (TPAC) 2007-02-26 21:10:23 PST
The other option, I guess, is to change the code in nsCSSFrameConstructor to check the frame type and get the first child as needed. Then we'll need to fix bug 368451 in some other way (probably by doing that thing with anonymous content I've suggested before where we reframe its closest non-anonymous ancestor).
Comment 13 Boris Zbarsky [:bz] (TPAC) 2007-02-26 21:12:32 PST
Yet another option, I suppose, is to give the first-letter the content which is the parent of the textnode.  That should preserve the invariant in question while fixing this bug and bug 368451, I think.
Comment 14 Robert O'Callahan (:roc) (email my personal email if necessary) 2007-02-27 00:47:43 PST
The last suggestion sounds the best to me.
Comment 15 Boris Zbarsky [:bz] (TPAC) 2007-02-27 21:06:54 PST
Created attachment 256756 [details] [diff] [review]
With that change

This patch uses the parent of the text for the first-letter content.

The change to CharacterDataChange deserves some mention.  I made it because I was getting assertions about nodes not being unbound properly on the bug 368451 testcase.  The reason was that the quote text ended up getting reinserted, so ended up outside the :after wrapper.  The dynamic-1 test tests that.  The dynamic-2 test tests bug 368451 comment 2.  The dynamic-3 tests test the testcase in this bug's comment 5, basically.

With this patch all the attached tests pass, and I get no asserts on the testcases in this bug or bug 368451.
Comment 16 Boris Zbarsky [:bz] (TPAC) 2007-02-28 14:32:03 PST
Checked in.
Comment 17 Boris Zbarsky [:bz] (TPAC) 2007-04-15 17:24:24 PDT
Jesse, do you think we want this on branches?  If so, I'll work on a patch for that...
Comment 18 Jesse Ruderman 2007-04-15 17:27:07 PDT
Only if this assertion bothers you, or if you think bug 368451 is likely to be a security hole rather than just a sorta-leak.
Comment 19 Boris Zbarsky [:bz] (TPAC) 2007-04-15 17:47:18 PDT
Hmm... probably better to not do it for now, since neither of those seems _that_ bad...
Comment 20 Boris Zbarsky [:bz] (TPAC) 2007-06-14 22:13:19 PDT
Thought this does seem to block bug 362901, I think...
Comment 21 Daniel Veditz [:dveditz] 2007-06-21 10:59:34 PDT
This one doesn't look like a blocker, but we may approve a branch patch if other bugs down the chain turn out to be blockers.
Comment 22 Boris Zbarsky [:bz] (TPAC) 2007-08-22 10:33:51 PDT
Fixed for by checkin for bug 362901.
Comment 23 Karl Tomlinson (:karlt) 2010-11-25 20:16:46 PST
quote-1d/b.html and dynamic-3b.html now pass on all platforms:

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