Last Comment Bug 379799 - [FIX]Crash [@ PresShell::GetPrimaryFrameFor] with :first-letter and :after and counter
: [FIX]Crash [@ PresShell::GetPrimaryFrameFor] with :first-letter and :after an...
Status: RESOLVED FIXED
[sg:critical] (needed if 362901 taken...
: crash, regression, testcase, verified1.8.1.8
Product: Core
Classification: Components
Component: Layout (show other bugs)
: Trunk
: All Mac OS X
: P2 critical (vote)
: mozilla1.9alpha5
Assigned To: Boris Zbarsky [:bz] (Out June 25-July 6)
:
Mentors:
Depends on:
Blocks: randomclasses 362901 372550
  Show dependency treegraph
 
Reported: 2007-05-04 20:08 PDT by Jesse Ruderman
Modified: 2011-06-13 10:01 PDT (History)
8 users (show)
dveditz: blocking1.8.1.8+
dveditz: wanted1.8.1.x+
caillon: blocking1.8.0.next+
dveditz: wanted1.8.0.x+
jruderman: in‑testsuite+
See Also:
Crash Signature:
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
testcase (493 bytes, text/html)
2007-05-04 20:08 PDT, Jesse Ruderman
no flags Details
Testcase that shows rendering bug here (431 bytes, text/html)
2007-05-06 14:46 PDT, Boris Zbarsky [:bz] (Out June 25-July 6)
no flags Details
Reference rendering for that testcase (138 bytes, text/html)
2007-05-06 14:47 PDT, Boris Zbarsky [:bz] (Out June 25-July 6)
no flags Details
Proposed fix (2.88 KB, patch)
2007-05-06 16:30 PDT, Boris Zbarsky [:bz] (Out June 25-July 6)
dbaron: review+
dbaron: superreview+
Details | Diff | Review
Renames as suggested (17.46 KB, patch)
2007-05-07 12:02 PDT, Boris Zbarsky [:bz] (Out June 25-July 6)
no flags Details | Diff | Review

Description Jesse Ruderman 2007-05-04 20:08:52 PDT
Created attachment 263822 [details]
testcase

Loading the testcase in a Mac trunk debug build causes:

Thread 0 Crashed:
0   <<00000000>> 	0x10711001 0 + 275845121
1   libgklayout.dylib        	0x19724602 PresShell::GetPrimaryFrameFor(nsIContent*) const + 42 (nsPresShell.cpp:4705)
2   libgklayout.dylib        	0x196e39af nsCSSFrameConstructor::CharacterDataChanged(nsIContent*, int) + 301 (nsCSSFrameConstructor.cpp:9852)
3   libgklayout.dylib        	0x19729455 PresShell::CharacterDataChanged(nsIDocument*, nsIContent*, CharacterDataChangeInfo*) + 233 (nsPresShell.cpp:4478)
4   libgklayout.dylib        	0x19a88fca nsBindingManager::CharacterDataChanged(nsIDocument*, nsIContent*, CharacterDataChangeInfo*) + 88 (nsBindingManager.cpp:1160)
5   libgklayout.dylib        	0x19945fa0 nsNodeUtils::CharacterDataChanged(nsIContent*, CharacterDataChangeInfo*) + 166 (nsNodeUtils.cpp:88)
6   libgklayout.dylib        	0x1992bb95 nsGenericDOMDataNode::SetTextInternal(unsigned, unsigned, unsigned short const*, unsigned, int) + 1003 (nsGenericDOMDataNode.cpp:485)
7   libgklayout.dylib        	0x1992c002 nsGenericDOMDataNode::SetData(nsAString_internal const&) + 84 (nsGenericDOMDataNode.cpp:323)
8   libgklayout.dylib        	0x19dc8bd0 nsTextNode::SetData(nsAString_internal const&) + 24 (nsTextNode.cpp:69)
9   libgklayout.dylib        	0x19700ad6 nsCounterList::RecalcAll() + 200 (nsCounterManager.cpp:186)
10  libgklayout.dylib        	0x19700b33 RecalcDirtyLists(nsAString_internal const&, nsCounterList*, void*) + 37 (nsCounterManager.cpp:274)
11  libgklayout.dylib        	0x19d54ea5 nsBaseHashtable<nsStringHashKey, nsAutoPtr<nsCounterList>, nsCounterList*>::s_EnumReadStub(PLDHashTable*, PLDHashEntryHdr*, unsigned, void*) + 81 (nsBaseHashtable.h:327)
12  libxpcom_core.dylib      	0x012f5764 PL_DHashTableEnumerate + 191 (pldhash.c:724)
13  libgklayout.dylib        	0x19d54b60 nsBaseHashtable<nsStringHashKey, nsAutoPtr<nsCounterList>, nsCounterList*>::EnumerateRead(PLDHashOperator (*)(nsAString_internal const&, nsCounterList*, void*), void*) const + 128 (nsBaseHashtable.h:188)
14  libgklayout.dylib        	0x19700bf3 nsCounterManager::RecalcAll() + 41 (nsCounterManager.cpp:281)
15  libgklayout.dylib        	0x196d66dc nsCSSFrameConstructor::RecalcQuotesAndCounters() + 80 (nsCSSFrameConstructor.cpp:10200)
16  libgklayout.dylib        	0x196d6784 nsCSSFrameConstructor::EndUpdate() + 36 (nsCSSFrameConstructor.cpp:10181)
17  libgklayout.dylib        	0x196ecfd2 nsCSSFrameConstructor::ProcessPendingRestyles() + 536 (nsCSSFrameConstructor.cpp:12973)
18  libgklayout.dylib        	0x196ed0bc nsCSSFrameConstructor::RestyleEvent::Run() + 200 (nsCSSFrameConstructor.cpp:13032)
19  libxpcom_core.dylib      	0x013557ea nsThread::ProcessNextEvent(int, int*) + 508 (nsThread.cpp:483)
Comment 1 Boris Zbarsky [:bz] (Out June 25-July 6) 2007-05-06 14:41:19 PDT
So the frame for the <span> ends up as a child of the first-letter frame in this case...
Comment 2 Boris Zbarsky [:bz] (Out June 25-July 6) 2007-05-06 14:46:12 PDT
Created attachment 263946 [details]
Testcase that shows rendering bug here
Comment 3 Boris Zbarsky [:bz] (Out June 25-July 6) 2007-05-06 14:47:01 PDT
Created attachment 263947 [details]
Reference rendering for that testcase
Comment 4 Boris Zbarsky [:bz] (Out June 25-July 6) 2007-05-06 16:30:37 PDT
Created attachment 263952 [details] [diff] [review]
Proposed fix

The real issue is that we don't remove first-letter on restyles, really... but this fixes the problem by making sure to remove it on ContentInserted if it's around (though we readd it too, unfortunately).
Comment 5 Boris Zbarsky [:bz] (Out June 25-July 6) 2007-05-06 16:35:21 PDT
I wonder whether this is a problem on branches...  I bet it is.
Comment 6 Jesse Ruderman 2007-05-06 16:49:54 PDT
Is there a bug filed on the "real issue" that comment 4 alludes to?
Comment 7 Boris Zbarsky [:bz] (Out June 25-July 6) 2007-05-06 17:04:38 PDT
Bug 8253.
Comment 8 Boris Zbarsky [:bz] (Out June 25-July 6) 2007-05-06 17:05:19 PDT
Though I suppose removing it in a performant way is easier than adding it... maybe.
Comment 9 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2007-05-07 10:00:02 PDT
Comment on attachment 263952 [details] [diff] [review]
Proposed fix

r+sr=dbaron

I don't like the function names here, though.  The two versions of HaveFirstLetterStyle are quite different.  Perhaps you should rename:

HaveFirstLetterStyle(,) -> ShouldHaveFirstLetterStyle
HaveFirstLineStyle -> ShouldHaveFirstLineStyle
HaveSpecialBlockStyle -> ShouldHaveSpecialBlockStyle

HaveFirstLetterStyle() -> HasFirstLetterStyle

as a followup patch, if you agree.  (No need to ask for further review.)

It would be nice to fix the underlying problem, though.
Comment 10 Boris Zbarsky [:bz] (Out June 25-July 6) 2007-05-07 10:58:24 PDT
Fixed on trunk.  Doesn't seem to be a problem on branches, so probably an effect of bug 372550.

Not adding tests yet, because those would give away the bug, I guess.  :(
Comment 11 Boris Zbarsky [:bz] (Out June 25-July 6) 2007-05-07 12:02:51 PDT
Created attachment 264029 [details] [diff] [review]
Renames as suggested

Good idea on this.  Checked this in too.
Comment 12 Daniel Veditz [:dveditz] 2007-05-28 22:14:11 PDT
bz: having trouble reconciling your branch-blocking requests with your comment 10 saying this isn't a problem on branches, which seem to be made at the same time. Do we want/need this on branches or not?
Comment 13 Boris Zbarsky [:bz] (Out June 25-July 6) 2007-05-28 22:54:44 PDT
This bug is a regression from the fix for bug 362901 (see comments in bug 372550).  The blocking flags were set to mirror those on bug 362901.  This business of not adding deps to hidden bugs really sucks.  :(  Do we have any better way to track things?
Comment 14 Daniel Veditz [:dveditz] 2007-06-14 10:48:59 PDT
Why are we not adding dependencies on hidden bugs? I can think of no better way to track things.
Comment 15 Boris Zbarsky [:bz] (Out June 25-July 6) 2007-06-14 10:53:30 PDT
I was told at some point to not have dependencies between hidden and not-hidden bugs...  Presumably because if a non-hidden bug blocks a hidden one people realize that it's a security-related fix?

In any case, if you're ok with me doing it I'd love to add said deps.
Comment 16 Daniel Veditz [:dveditz] 2007-06-14 14:46:34 PDT
If something is a regression from a security fix then the security fix has already been checked in--that's got to be more revealing than a dependent bug. Please definitely do mark regressions as blocking "fixed" security bugs so we don't miss them when we're planning branch landings.

In other cases if merely hinting that there might be a security bug that will fix or be fixed by the public bug is enough to point someone at the problem then the public bug should possibly not be public. If this is the case and you really don't want to privatize the public bug then maybe don't mark dependencies, but in most cases it should be fine.

In cases where you withhold explicit dependencies please mention them in the status whiteboard of the hidden bug(s) so we can easily find them during triage and remember to mark them when the security bugs are eventually publicized.
Comment 17 Boris Zbarsky [:bz] (Out June 25-July 6) 2007-06-14 22:13:56 PDT
Sounds like a plan.  I updated the dependencies...
Comment 18 Boris Zbarsky [:bz] (Out June 25-July 6) 2007-08-22 10:33:48 PDT
Fixed for 1.8.1.7 by checkin for bug 362901.
Comment 19 Carsten Book [:Tomcat] 2007-08-30 10:30:19 PDT
verified fixed 1.8.1.7 using Mozilla/5.0 (Macintosh; U; Intel Mac OS X; en-US; rv:1.8.1.7pre) Gecko/2007083005 BonEcho/2.0.0.7pre - no crash on the 2 testcases from this bug - adding verified keyword
Comment 20 Jesse Ruderman 2007-12-15 17:04:34 PST
I checked in my testcase as a crashtest and bz's reftest pair as a reftest pair.

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