468645 - Crash [@ nsFrame::Destroy] with binding, page-break-after and stylesheet when closing print preview

Status: VERIFIED FIXED

(Core :: Layout, defect, P2)

Description

[Martijn Wargers (dead)]

Attachment: testcase

See testcase, which crashes current trunk build on closing print preview.

This regressed between 2008-07-26 and 2008-07-27.

Hg seems to be down, so I can't really look now what patch might have caused this.
http://hg.mozilla.org/mozilla-central/pushloghtml

Breakpad stacks don't seem to give useful stacks:
http://crash-stats.mozilla.com/report/index/945f6c65-481a-4072-b221-bd6862081209?p=1
@0x0

With a debug build, I get this stacktrace:
>	gklayout.dll!nsFrame::Destroy()  Line 515 + 0x7 bytes	C++
 	gklayout.dll!nsLineBox::DeleteLineList(nsPresContext * aPresContext=0x0896ac38, nsLineList & aLines={...})  Line 341	C++
 	gklayout.dll!nsBlockFrame::Destroy()  Line 301 + 0x10 bytes	C++
 	gklayout.dll!nsLineBox::DeleteLineList(nsPresContext * aPresContext=0x0896ac38, nsLineList & aLines={...})  Line 341	C++
 	gklayout.dll!nsBlockFrame::Destroy()  Line 301 + 0x10 bytes	C++
 	gklayout.dll!nsFrameList::DestroyFrames()  Line 68	C++
 	gklayout.dll!nsContainerFrame::Destroy()  Line 266	C++
 	gklayout.dll!nsFrameList::DestroyFrames()  Line 68	C++
 	gklayout.dll!nsContainerFrame::Destroy()  Line 266	C++
 	gklayout.dll!nsFrameList::DestroyFrames()  Line 68	C++
 	gklayout.dll!nsContainerFrame::Destroy()  Line 266	C++
 	gklayout.dll!nsFrameList::DestroyFrames()  Line 68	C++
 	gklayout.dll!nsContainerFrame::Destroy()  Line 266	C++
 	gklayout.dll!nsTableFrame::Destroy()  Line 276	C++
 	gklayout.dll!nsFrameList::DestroyFrames()  Line 68	C++
 	gklayout.dll!nsContainerFrame::Destroy()  Line 266	C++
 	gklayout.dll!nsTableOuterFrame::Destroy()  Line 228	C++
 	gklayout.dll!nsLineBox::DeleteLineList(nsPresContext * aPresContext=0x0896ac38, nsLineList & aLines={...})  Line 341	C++
 	gklayout.dll!nsBlockFrame::Destroy()  Line 301 + 0x10 bytes	C++
 	gklayout.dll!nsFrameList::DestroyFrames()  Line 68	C++
 	gklayout.dll!nsContainerFrame::Destroy()  Line 266	C++
 	gklayout.dll!CanvasFrame::Destroy()  Line 240	C++
 	gklayout.dll!nsFrameList::DestroyFrames()  Line 68	C++
 	gklayout.dll!nsContainerFrame::Destroy()  Line 266	C++
 	gklayout.dll!ViewportFrame::Destroy()  Line 69	C++
 	gklayout.dll!nsFrameList::DestroyFrames()  Line 68	C++
 	gklayout.dll!nsContainerFrame::Destroy()  Line 266	C++
 	gklayout.dll!nsFrameList::DestroyFrames()  Line 68	C++
 	gklayout.dll!nsContainerFrame::Destroy()  Line 266	C++
 	gklayout.dll!nsFrameList::DestroyFrames()  Line 68	C++
 	gklayout.dll!nsContainerFrame::Destroy()  Line 266	C++
 	gklayout.dll!nsHTMLScrollFrame::Destroy()  Line 158	C++
 	gklayout.dll!nsFrameList::DestroyFrames()  Line 68	C++
 	gklayout.dll!nsContainerFrame::Destroy()  Line 266	C++
 	gklayout.dll!ViewportFrame::Destroy()  Line 69	C++
 	gklayout.dll!nsFrameManager::Destroy()  Line 293	C++
 	gklayout.dll!PresShell::Destroy()  Line 1710	C++
 	gklayout.dll!nsPrintObject::DestroyPresentation()  Line 98	C++
 	gklayout.dll!nsPrintObject::~nsPrintObject()  Line 62	C++
 	gklayout.dll!nsPrintObject::`scalar deleting destructor'()  + 0xf bytes	C++
 	gklayout.dll!nsPrintData::~nsPrintData()  Line 124 + 0x1f bytes	C++
 	gklayout.dll!nsPrintData::`scalar deleting destructor'()  + 0xf bytes	C++
 	gklayout.dll!nsPrintEngine::Destroy()  Line 260 + 0x1f bytes	C++
 	gklayout.dll!DocumentViewerImpl::ReturnToGalleyPresentation()  Line 4069	C++
 	gklayout.dll!DocumentViewerImpl::ExitPrintPreview()  Line 3841	C++
etc...

Comment 1

[Martijn Wargers (dead)]

Hg is back up:
http://hg.mozilla.org/mozilla-central/pushloghtml?startdate=2008-07-26+03%3A00%3A00&enddate=2008-07-27+04%3A00%3A00
I guess this is somehow a regression from bug 156716.

Comment 2

[Martijn Wargers (dead)]

Attachment: testcase2

This testcase is a little bit simpler and crashes directly on print preview.

Breakpad doesn't give a useful stack though:
http://crash-stats.mozilla.com/report/index/ed270aa0-049c-46b0-9966-f79262081209?p=1

Comment 3

[Martijn Wargers (dead)]

Attachment: testcase3

This one also crashes on print preview, but I noticed that this hasn't the same regression range as comment 0, so perhaps the regression range is not good or useful.

Comment 4

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Was the regression range for testcase3 before or after the other regression range?

Comment 5

[Martijn Wargers (dead)]

It was after the other regression range. It regressed between 2008-11-25 and 2008-11-26:
http://hg.mozilla.org/mozilla-central/pushloghtml?startdate=2008-11-25+04%3A00%3A00&enddate=2008-11-26+06%3A00%3A00
Breakpad stack:
http://crash-stats.mozilla.com/report/index/f018a543-29ab-4799-8e92-249302081215?p=1
0  	xul.dll  	nsFrameManager::UnregisterPlaceholderFrame  	 layout/base/nsFrameManager.cpp:528
1 	xul.dll 	UnregisterPlaceholderChain 	layout/base/nsCSSFrameConstructor.cpp:9341
2 	xul.dll 	nsCSSFrameConstructor::RemoveFixedItems 	layout/base/nsCSSFrameConstructor.cpp:13070
3 	xul.dll 	nsCSSFrameConstructor::ReconstructDocElementHierarchyInternal 	layout/base/nsCSSFrameConstructor.cpp:7651
4 	xul.dll 	xul.dll@0x313846 	
5 	xul.dll 	nsCSSFrameConstructor::ProcessRestyledFrames 	layout/base/nsCSSFrameConstructor.cpp:9912
6 	xul.dll 	nsCSSFrameConstructor::RebuildAllStyleData 	layout/base/nsCSSFrameConstructor.cpp:13403
7 		@0x12f637 	

So I guess testcase3 might actually be a different bug.

Comment 6

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Attachment: valgrind's explanation for crash on attachment 352178

Comment 7

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Attachment: valgrind's explanation for crash on attachment 352132

significantly less useful than the previous one (but hopefully it's the same bug)

Comment 8

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Attachment: valgrind's explanation for crash on attachment 352414

This does seem to be a different bug.

Comment 9

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

It doesn't help that the SetBounds we do during initialization causes a media feature values changed event:

#0  nsPresContext::PostMediaFeatureValuesChangedEvent (this=0x25d9d10)
    at /home/dbaron/builds/mozilla-central/mozilla/layout/base/nsPresContext.cpp:1459
#1  0x00007fb4bcd0a9e5 in DocumentViewerImpl::InitPresentationStuff (
    this=0x25d9bb0, aDoInitialReflow=0, aReenableRefresh=0)
    at /home/dbaron/builds/mozilla-central/mozilla/layout/base/nsDocumentViewer.cpp:740

we should probably fix that too.

Comment 10

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

... and also, in addition to the cases above where we haven't done rule matching yet, the SetVisibleArea calls from DocumentViewerImpl::SizeToContent

Comment 11

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Attachment: patch 1: fix style context invariants for page breaks

This fixes the other underlying problem:  when we try to do style reresolution for a page break, we give it a different parent and a different content node.  In this case, the different parent was causing problems because it changed whether the page break frame was inheriting -moz-user-input from the text input or not; this caused a framechange hint.  Somehow this causes the root element's block frame to have a style context with a dangling rule node.  (I'd have expected that for the page break frame, since an attempt to reconstruct would miss it, but not for the root.)

So maybe there's yet a third underlying problem...

Comment 12

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Attachment: give nsPageBreakFrame a frame name

This was helpful in the debugging that led to the previous patch.

Comment 13

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

(In reply to comment #11)
> or not; this caused a framechange hint.  Somehow this causes the root element's
> block frame to have a style context with a dangling rule node.  (I'd have

Er, that wasn't the case; I was just getting aFrame and aChild mixed up.

It was actually the old page break frame's style context that had the dangling rule node.

The third problem is that when we get a frame reconstruct for an element with page break properties, we don't remove its page break frame.  (But we do add a new one, so we end up with two, one with a good style context and one with a bad one.)


That said, we don't support incremental layout in print mode yet; I should probably spin this first issue off into a separate bug and fix the issues in comment 9 and comment 10.

Comment 14

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

(In reply to comment #13)
> The third problem is that when we get a frame reconstruct for an element with
> page break properties, we don't remove its page break frame.  (But we do add a
> new one, so we end up with two, one with a good style context and one with a
> bad one.)

But the dangling pointer problem here is actually specific to getting a frame reconstruct for the page break itself, and I think should be fixed by the patch in comment 11.

Comment 15

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

I filed bug 470929 on dynamic frame reconstruction for page break frames.

Comment 16

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Attachment: patch 1: fix style context invariants for page breaks

The first version of this patch caused a crash in reftests (and presumably some correctness bugs) because it moved up to the parent style context too early.

Comment 17

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Attachment: patch 3: prevent rebuilding all style data in paginated mode

This prevents us from getting to the problematic code, and also fixes the bug.

However, without any other patches, we hit the assertion.  I need to fix that as well.

Comment 18

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

So, for what it's worth, I have three different ways to fix this; patch 1 above, patch 3 above (which causes assertions without patch 2a), and patches 2a, 2b, and 2c, which cause the rebuild of style data not to happen.  I still need to figure out why (1) 2a regresses opening test_acid3_test46.html in a new tab and (2) why the test for bug 453896 doesn't detect that failure.  I think we should do all three.

The patches are in http://hg.mozilla.org/users/dbaron_mozilla.com/patches/ .

Comment 19

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

It turns out the test failure was intermittent to begin with, and was present without the patch too.  (Although maybe my previous patch made it reliable, and now my new one brings us back to intermittent.)  In any case, I have a simpler version of patch 2a now.

Comment 20

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Attachment: patch 2a: don't do media query change handling if we don't yet have anything needing to be invalidated

Comment 21

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Attachment: patch 2b: fix media query 'orientation' handling for print preview

This is a change that I missed in bug 466559.

It's needed to make the optimization in patch 2c fully valid.

I'm really not sure how to test it.

Comment 22

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Attachment: patch 2c: suppress media query change handling for SetVisibleArea when paginated

Since SetVisibleArea only changes something that affects media queries when not paginated, we should suppress the media query change handling.  This actually matters because there's some SetVisibleArea calls nested within a SizeToContent call that affect the print preview window... I need to file a separate bug on that as well.

Comment 23

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Attachment: patch 3: prevent rebuilding all style data in paginated mode

I'm not sure whether or not I want the PreferenceChanged change here; it's the only change relative to attachment 354300 [details] [diff] [review].

Comment 24

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

So when I said that each patch independently fixes this bug, I was testing on attachment 352178 [details].

All three patches together fix attachment 352132 [details] and attachment 352414 [details], but I haven't tested that each one independently does; in fact, I suspect that's likely not the case.

Comment 25

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

patch 1 (attachment 354299 [details] [diff] [review]) on its own fixes the crashes on testcase (attachment 352132 [details]) and testcase2 (attachment 352178 [details]), but NOT the crash on testcase3 (attachment 352414 [details]).

patches 2a (attachment 354399 [details] [diff] [review]), 2b (attachment 354400 [details] [diff] [review]), and 2c (attachment 354401 [details] [diff] [review]) on their own fix all three testcases. 

patch 3 (attachment 354402 [details] [diff] [review]) on its own fixes all three testcases (by replacing the crash with an assertion).

Comment 26

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Attachment: patch 4: fix style reresolution idempotence for :before { display: table }

This is the analog to patch 1.

This, by itself, fixes the crash on testcase3 (attachment 352414 [details]), but NOT on the other two.

I should probably also file a followup bug on ReconstructDocElementHierarchy crashing in this situation in paginated mode.

Comment 27

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Attachment: patch 2b: fix media query 'orientation' handling for print preview

It's actually easy to test, since the reftest printing harness uses the opposite orientation.  This fixes the reftest failure introduced by the previous patch by fixing the test.  I'm not sure why jmaher left this bug report hidden in a one-line comment in the test where I didn't see it...

Comment 28

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 354415 [details] [diff] [review]
patch 4: fix style reresolution idempotence for :before { display: table }

A slightly longer explanation for why I think patch 4 is both correct and pretty safe:

The aParentContent parameter to ReResolveStyleContext is used in two cases:
 (1) as fallback content for style resolution and change list queues in case aFrame->GetContent() is null (are there any cases where this should happen?  I don't know of any...)
 (2) (primarily) as the content node for style resolution for pseudo-elements.

The code I'm changing is the code where we resolve the provider first when the style context provider is a child, a case that happens only in the presence of table outer frames (i.e., for them, or for some other frame that defers to them, although I can't think of any examples of how the latter could happen).

So, in other words, I don't think this change would have made any difference until we started supporting display:table on :before and :after pseudo-elements (bug 238072).

And I think passing through aParentContent is the right thing to do in that case; it's the actual element involved rather than the fake element that we make for the pseudo-element, and thus what we want to do selector matching on.

Comment 29

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

(In reply to comment #22)
> paginated, we should suppress the media query change handling.  This actually
> matters because there's some SetVisibleArea calls nested within a SizeToContent
> call that affect the print preview window... I need to file a separate bug on
> that as well.

Sorry, it actually has nothing to do with SizeToContent (I think I got stuck in gdb looking at information about the progress dialog rather than the document being previewed), but we do seem to resize the document a bunch of times, back and forth between two somewhat different sizes.

Comment 30

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

...well, we resize the galley context once, and then resize the print preview context once.  I'm not sure why there isn't only one resize, though.

Comment 31

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Attachment: patch 5: don't post values changed when there was no resize

This seems like an obvious thing to do as well.  I wasn't debugging well enough to know whether it reduced the number of events posted or not, actually, and it's rather a pain to figure out due to the long rebuild times...

Comment 32

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Attachment: patch 6: fix nsLayoutUtils::IsReallyFixedPos for paginated mode

This fixes the underlying problem about ReconstructDocElementHierarchy crashing that I mentioned in comment 26.  Like patch 4, it fixes testcase 3 (attachment 352414 [details]) by itself.

This fixes a mistake introduced quite recently, in bug 455171: http://hg.mozilla.org/mozilla-central/rev/483aaa57f290

Comment 33

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Since there are a lot of patches here, I'll give a brief summary of their relationship to each other and to the testcases:

The crash in each of the three testcases required a sequence of three separate things to go wrong:
 (1) calling RebuildAllStyleData during the setup of print preview, which shouldn't happen
 (2) a bug in the ensuing style reresolution (which should have led to no change at all) that caused an erroneous reframe hint
 (3) a bug in the code to handle that reframe that caused a crash

Problem (1) is a regression from bug 156716, and is fixed by patches 2a and 2c (there were multiple causes).  2b (a separate bug fix) is a prerequisite to make the optimization in 2c completely valid.  Patch 3 is a belt-and-suspenders check to make sure that if there are other problems of the type of problem (1), we will skip the style rebuild.  (I'm not entirely sure it's a good idea; we might want to remove it at some point in the future.  That said, it does assert, which should point anybody trying to get that to work to the code blocking it from working.)

Problems (2) and (3) differ between (testcase and testcase 2) and testcase3.

In testcase and testcase2:
 * problem (2) is fixed by patch 1
 * problem (3) is described by bug 470929, and I'm not worrying about it here
and both problems (2) and (3) are related to nsPageBreakFrames.

In testcase3:
 * problem (2) is fixed by patch 4, is an old bug unreachable without
     bug 238072, and is related to :before { display: table }
 * problem (3) is fixed by patch 6, is a regression from bug 455171, and is
     related to position:fixed when printing

Comment 34

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Of course, we should really figure out a way to test for other problems like (2) and (3), especially now that (1) is fixed.

Comment 35

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Attachment: patch 5: don't post values changed when there was no resize

Attachment 354434 [details] [diff] was so simple and obvious that I didn't run mochitests, which was a mistake.  Here's a corrected version using IsExactEqual.

Comment 36

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 354299 [details] [diff] [review]
patch 1: fix style context invariants for page breaks

>+++ b/layout/base/nsCSSFrameConstructor.cpp
>+    // Use the same parent style context as |styleContext|, since that's
>+    // easier to re-resolve and it doesn't matter in practice.  (Getting
>+    // different parents can result in framechange hints, e.g., for
>+    // user-modify.)

It'd make more sense to have this comment closer to the GetParentContext() call (and modify it to make sense in that context), I think.

r+sr=bzbarsky with that change.  I hate the pseudo stuff in ReResolveStyleContext.  :(

Comment 37

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 354399 [details] [diff] [review]
patch 2a: don't do media query change handling if we don't yet have anything needing to be invalidated

>+++ b/layout/base/nsIPresShell.h
>+  PRBool DidInitialReflow() const { return mDidInitialReflow; }

This doesn't seem to be used in this patch.  Is it used later or something?  If we do want this change, can we also nix the nsresult version?

>+++ b/layout/style/nsStyleSet.h
>+  PRBool HasCachedStyleData() const {
>+    if (!mRuleTree)
>+      return PR_FALSE;
>+    return mRuleTree->TreeHasCachedData();

  return mRuleTree && mRuleTree->TreeHasCachedData();

That said, do we need to worry about data cached in style contexts (in a situation where nothing in the document matches any rules)?  Or would we always have cached data on the root rulnenode in that case?

r+sr=bzbarsky with the above addressed.

Comment 38

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 354415 [details] [diff] [review]
patch 4: fix style reresolution idempotence for :before { display: table }

So why does this change help?  Change the content used for pseudo-element stuff, basically?

This could use some comments, both on the function declaration (to explain what the aParentContent arg is supposed to be) and the declarations of localContent and content (to explain what they are).

Comment 39

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

(In reply to comment #37)
> (From update of attachment 354399 [details] [diff] [review])
> >+++ b/layout/base/nsIPresShell.h
> >+  PRBool DidInitialReflow() const { return mDidInitialReflow; }
> 
> This doesn't seem to be used in this patch.  Is it used later or something?

It was used in the original version, but then I dropped it for a simpler approach.  However, since the patch causes a rebuild-all-of-layout anyway, I figured I'd leave it in.

> If we do want this change, can we also nix the nsresult version?

The plan was to allow gradual conversion.  I didn't get to looking at how many callers there were.  If there's only a few I suppose I could just do it.

>   return mRuleTree && mRuleTree->TreeHasCachedData();
> 
> That said, do we need to worry about data cached in style contexts (in a
> situation where nothing in the document matches any rules)?  Or would we always
> have cached data on the root rulnenode in that case?

Some of the things that call RebuildAllStyleData (e.g., zooming) call it because they invalidate conversions that were used to build cached data.  Therefore having cached data on on the root means we do need to do the change handling.

(In reply to comment #38)
> (From update of attachment 354415 [details] [diff] [review])
> So why does this change help?  Change the content used for pseudo-element
> stuff, basically?

Yes, it changes the content used for pseudo-element stuff.  We only go through that codepath for tables; it's where we're resolving style for the inner table frame first so that we can use it as the parent context for the outer table frame.  In this case the inner table is a :before pseudo-element, and without this fix we try to re-match (::before dummy content node)::before instead of the element::before that would give us back what we already have.  See comment 28.

> This could use some comments, both on the function declaration (to explain what
> the aParentContent arg is supposed to be) and the declarations of localContent
> and content (to explain what they are).

I can give it a shot, although I don't completely understand what's going on there, actually; do you understand why we'd have frames with null content nodes?  Or was that just paranoia that should have been an assertion?  I suppose I could try making it an assertion and find out.

Comment 40

[Boris Zbarsky [:bzbarsky]]

> The plan was to allow gradual conversion.

Ah, ok.  That's fine.  Followup patch for this is ok by me.

I'm not sure my question about cached style data was clear enough.  What if we have an mRuleTree, but there is no cached data on the root rulenode and no kids of the root rulenode.  Could we not still have style contexts with cached data.  In that case, do we need to rebuild?  I'd think that we do, if that situation can arise...

> without this fix we try to re-match (::before dummy content node)::before

Ah, ok.  That makes sense.

> do you understand why we'd have frames with null content nodes?

Viewport frames are the only such frames I'm aware of, offhand.  The problem is that the viewport frame needs to survive removal of the documentElement.  Or something.

Comment 41

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Attachment: patch 7: deCOMtaminate GetDidInitialReflow

Here's the full deCOMtamination of GetDidInitialReflow, separated out of patch 2a.

Comment 42

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

(In reply to comment #36)
> It'd make more sense to have this comment closer to the GetParentContext() call
> (and modify it to make sense in that context), I think.

Ah, right.  I didn't fix the comment when I revised the patch in comment 16.

(In reply to comment #40)
> I'm not sure my question about cached style data was clear enough.  What if we
> have an mRuleTree, but there is no cached data on the root rulenode and no kids
> of the root rulenode.  Could we not still have style contexts with cached data.
>  In that case, do we need to rebuild?  I'd think that we do, if that situation
> can arise...

Ah, right.  I'll try changing nsStyleSet.h to have:

  PRBool HasCachedStyleData() const {
    return (mRuleTree && mRuleTree->TreeHasCachedData()) || !mRoots.IsEmpty();
  }

Comment 43

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Attachment: patch 4: fix style reresolution idempotence for :before { display: table }

Here's a version with comments, plus an assertion that I think ought to hold (although I wouldn't be surprised if Jesse files a bug with a testcase showing when it happens).  If the assertion actually ends up not firing, we'll know we can simplify the code a bit...

Comment 44

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

(In reply to comment #42)
> Ah, right.  I'll try changing nsStyleSet.h to have:
> 
>   PRBool HasCachedStyleData() const {
>     return (mRuleTree && mRuleTree->TreeHasCachedData()) || !mRoots.IsEmpty();
>   }

And I've tested that it still returns false a decent number of times.

Comment 45

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 354635 [details] [diff] [review]
patch 7: deCOMtaminate GetDidInitialReflow

>+++ b/content/xul/document/src/nsXULDocument.cpp
>@@ -3886,17 +3886,17 @@ nsXULDocument::OverlayForwardReference::
>     PRBool notify = PR_FALSE;
>     nsIPresShell *shell = mDocument->GetPrimaryShell();
>     if (shell)
>-        shell->GetDidInitialReflow(&notify);
>+        notify = shell->DidInitialReflow();

How about moving the declaration of |notify| to after we get shell and doing it like so:

  PRBool notify = shell && shell->DidInitialReflow();

?

r+sr=bzbarsky with that.

Comment 46

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 354637 [details] [diff] [review]
patch 4: fix style reresolution idempotence for :before { display: table }

>+  NS_ASSERTION(aFrame->GetContent(),
>+               "frame must have content (i.e., not be viewport)");

This will fail any time nsCSSFrameConstructor::RebuildAllStyleData is called, I think.

Of course in that case aParentContent will also be null, so I think we _could_ assert that aFrame->GetContent() || !aParentContent.

With that change, r+sr=bzbarsky.

The proposed style set change looks good to me.

Comment 47

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

(In reply to comment #46)
> Of course in that case aParentContent will also be null, so I think we _could_
> assert that aFrame->GetContent() || !aParentContent.

I think we could even assert aFrame->GetContent() || !aFrame->GetParent(), which is stronger.

Comment 48

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Landed on mozilla-central:
http://hg.mozilla.org/mozilla-central/rev/1b8061be9145
http://hg.mozilla.org/mozilla-central/rev/43391574f35f
http://hg.mozilla.org/mozilla-central/rev/b80da4a764fc
http://hg.mozilla.org/mozilla-central/rev/efbec418c3a1
http://hg.mozilla.org/mozilla-central/rev/ffd1fe854d95
http://hg.mozilla.org/mozilla-central/rev/e09da908f8b4
http://hg.mozilla.org/mozilla-central/rev/6849ce51dfef
http://hg.mozilla.org/mozilla-central/rev/dee577166dd8
http://hg.mozilla.org/mozilla-central/rev/62304457e1b8
http://hg.mozilla.org/mozilla-central/rev/b1d50ee5b9e7
http://hg.mozilla.org/mozilla-central/rev/068e61bef01e

Comment 49

[Boris Zbarsky [:bzbarsky]]

> I think we could even assert aFrame->GetContent() || !aFrame->GetParent(),

That seems fine, but doesn't get us anywhere in terms of simplifying the code here.  I think we could also assert:

  aFrame->GetContent() || (!aFrame->GetParent() && !aParentContent)

and if _that_ doesn't trip, then we'll still be able to simplify the code.

Comment 50

[Martijn Wargers (dead)]

Verified fixed, using:
Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.2a1pre) Gecko/20081230 Minefield/3.2a1pre

Comment 51

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

http://hg.mozilla.org/releases/mozilla-1.9.1/rev/44f32749e57d
http://hg.mozilla.org/releases/mozilla-1.9.1/rev/81f3559edb0e
http://hg.mozilla.org/releases/mozilla-1.9.1/rev/b7007b8618e8
http://hg.mozilla.org/releases/mozilla-1.9.1/rev/a5967b160a25
http://hg.mozilla.org/releases/mozilla-1.9.1/rev/9258cfc46b24
http://hg.mozilla.org/releases/mozilla-1.9.1/rev/982c8f0d834c
http://hg.mozilla.org/releases/mozilla-1.9.1/rev/223df5cc7355
http://hg.mozilla.org/releases/mozilla-1.9.1/rev/f40982a7e167
http://hg.mozilla.org/releases/mozilla-1.9.1/rev/0411b64f02cb
http://hg.mozilla.org/releases/mozilla-1.9.1/rev/2338ff4080d3

Note that I did not transplant
http://hg.mozilla.org/mozilla-central/rev/efbec418c3a1

Comment 52

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Landed 
http://hg.mozilla.org/mozilla-central/rev/b3087034bc80 since I forgot to bump the IID and http://hg.mozilla.org/mozilla-central/rev/2e16533cc07e to fix the assertion per comment 49.

Comment 53

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

http://hg.mozilla.org/mozilla-central/rev/2e16533cc07e still needs to be landed on 1.9.1, so I'm waiting to mark fixed1.9.1

Comment 54

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

http://hg.mozilla.org/releases/mozilla-1.9.1/rev/2238e4099ae7

Comment 55

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

I backed out patch 3 to fix bug 472353:
http://hg.mozilla.org/mozilla-central/rev/6f2d2ef53d75
http://hg.mozilla.org/mozilla-central/rev/2a1f13246939
http://hg.mozilla.org/releases/mozilla-1.9.1/rev/f3745d8868f0
http://hg.mozilla.org/releases/mozilla-1.9.1/rev/b5dad31a8a15

The bug should still be fixed, though.  (And that was the patch I wasn't sure about; see comment 33.)

Comment 56

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

I'm also going to add a patch to bug 472353 to weaken the assertions from patch 4 and comment 49.

Comment 57

[Henrik Skupin [:whimboo][⌚️UTC+1]]

None of the testcases crashes on 1.9.1. Verified with:

Mozilla/5.0 (Windows; U; Windows NT 6.0; en-US; rv:1.9.1b3pre) Gecko/20090201 Shiretoko/3.1b3pre

Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.1b3pre) Gecko/20090201 Shiretoko/3.1b3pre

Comment 58

[Henrik Skupin [:whimboo][⌚️UTC+1]]

David, I've set the TM to 1.9.2a1 because all of those patches went into mozilla-central and the 1.9.1 branch. We have the fixed/verified1.9.1 keyword for branch checkins while the TM should be set for fixed bugs which landed after the 12/01/09 on trunk. We had a in length discussion about that topic in m.d.planning a while ago and we really should be consistent.