[sg:critical] because the stack trace has a random address on top. Thread 0 Crashed: 0 0 + 1056979456 1 nsContainerFrame::Destroy(nsPresContext*) + 128 2 nsFrameList::DestroyFrames(nsPresContext*) + 64 ...

In a ff1.5.0.2 windows debug build I crash a little deeper (possibly before yours?), at nsIView::Destroy() on an already-deleted 'this'.

Still crashes 2006-05-25 trunk nightly.

The frame tree and the view tree doesn't match after restyle. So if the view still hold a client data, we destroy it latter.

Can you explain in more detail what's happening here and how the child is eventually destroyed?

In the view tree you can see this. ............................ 88df760 {0,570,6300,285} z=0 vis=1 opc=1.000 tran=1 clientData=8d2fba4 < ^^"Parent View" 8a89180 {0,0,6300,285} z=0 vis=1 opc=1.000 tran=1 clientData=8bb4498 < ^^"Child View" > > ............................. In the frame tree you can see this. ........................................................ line 8bb3864: Line(div)(1)@8d2fba4 [view=88df760!!("Parent View")] Inline(span)(1)@8bb4458 [view=89b4358] Text(0)@8bb39ec[36,43,F] "ZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZ " > > > > line 8bb37b8: Line(div)(1)@8d2faf8 [view=8d06bd0] Inline(span)(1)@8bb4498 [view=8a89180!!("Child View")] Text(0)@8bb3940[79,43,F] "ZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZ " > > > > ........................... The parent view and the child view belong to the different frame tree. If the frame with the parent view destroies, then it will destroy the parent view and the child view. So when the frame with the child view destroies, the child view has been already destroied. That cause the crash.

That is a malformed view tree. The structure of the view tree needs to match the structure of the frame tree, so that view V1 is an ancestor of view V2 if and only if V1's frame is an ancestor of V2's frame. We need to fix this bug by not creating that bad view/frame tree. One thing we should do here is we should not allow -moz-line-frame to be styled with crazy stuff like position:absolute. See http://www.w3.org/TR/REC-CSS2/selector.html#first-line-pseudo under "certain restrictions". We should implement those restrictions in the style system. In fact we should not allow most of our -moz psuedos to be styled outside of our UA style sheets. Do you agree, David?

(In reply to comment #10) > In > fact we should not allow most of our -moz psuedos to be styled outside of our > UA style sheets. Do you agree, David? Yes. We have similar issues with blocking some properties on :first-line and :first-letter that we currently don't quite completely solve, although those are probably harder to solve.

> we should not allow most of our -moz psuedos to be styled outside of our > UA style sheets When fixing this, I think it would be better to do so by ignoring the selector rather than by ignoring the properties. I imagine it's possible for there to be crashes where just having a ::-moz-* selector match can cause issues, since the testcase for this bug contains the empty rule ".lizard:first-line { }".

(In reply to comment #12) > When fixing this, I think it would be better to do so by ignoring the selector > rather than by ignoring the properties. I imagine it's possible for there to > be crashes where just having a ::-moz-* selector match can cause issues, since > the testcase for this bug contains the empty rule ".lizard:first-line { }". That's only the case for true pseudo-elements (:first-line, :first-letter, :before, :after, and :-moz-selection), which are hard to implement and of which none are Mozilla-proprietary.

roc: Can you take a look at the patch? We want a fix for this for 1.8.0.5.

I don't think it's fixing the right place. We can probably still crash somewhere with the malformed view tree. I'll go with comment #11 and try to get a fix for this bug that way.

Boris, this is (I think) a fairly simple approach to disabling anonymous box selectors outside of UA (and editor override) style sheets.

It seems possible that these three: CSS_ANON_BOX(mozMathStretchy, ":-moz-math-stretchy") CSS_ANON_BOX(mozMathAnonymous, ":-moz-math-anonymous") CSS_ANON_BOX(mozMathInline, ":-moz-math-inline") *might* be intended to be used by pages. rbs, are these just for Mozilla, or for Web authors? Other than that this seems fine to me.

Comment on attachment 224941 [details] [diff] [review] fix I'm also interested in Boris's opinion on the very first change in the patch -- i.e., what we should do for nsIStyleSheetService. That service could be used to manage hand or computer-written user sheets, and also UA sheets, so I think what you have is the right thing, but Boris may have a better idea of what nsIStyleSheetService is and could be used for.

Boris, see David's comment #18.

> + // Editor override style sheets may want to style Gecko anonymous boxes > + cssLoader->SetUnsafeRulesEnabled(PR_TRUE); > ... > rv = cssLoader->LoadSheetSync(uaURI, getter_AddRefs(sheet)); > ... > + // Disable unsafe rules because this loader can be used again > + cssLoader->SetUnsafeRulesEnabled(PR_FALSE); Perhaps "unsafe rules enabled" should be a bool argument to LoadSheetSync rather than a member variable, to prevent bugs where someone forgets to reset it on a css loader that can be reused.

Yeah, that might be good. Boris probably has an opinion there too.

Yeah, the nsIStyleSheetService change looks good. It's just designed for adding UA and user sheets for extensions, basically. I do think it would be safer to make the boolean an arg to LoadSheetSync. I'd kinda like to avoid the member in the CSSLoader, because unfortunately LoadSheetSync is sync but does not prevent the CSSLoader being reentered (e.g. if someone loads an http: URI sync). The right thing to do is probably to add the boolean to the SheetLoadData; that way it can be inherited for @imported sheets in LoadChildSheet, and the member on the parser can be set and unset in ParseSheet() around the Parse() call. roc, let me know if you'd be happier with me doing the CSSLoader part? Also, for the branch we'll want to add a branch-only interface, I guess. :(

I should add that this is an awesome idea; I wish we'd done it long ago. We might be able to eliminate some !importants from rules in ua.css now (and possibly even some of the rules).

Re: comment 17 They are intended to be internal (but they might indeed be used by curious users).

updated as suggested

Comment on attachment 225250 [details] [diff] [review] fix >Index: layout/style/nsCSSLoader.cpp >@@ -154,24 +154,25 @@ SheetLoadData::SheetLoadData(CSSLoaderIm >+ mAllowUnsafeRules(PR_FALSE), > mWasAlternate(aIsAlternate), Switch these two lines. >Index: layout/style/nsICSSLoader.h >+ * @param aEnableUnsafeRules wet whether unsafe rules are enabled. s/wet // And maybe "enabled for this sheet load". >+ * In particular, most anonymous box psuedoelements must be very carefully "pseudoelements" >Index: layout/style/nsICSSParser.h > NS_IMETHOD SetCaseSensitive(PRBool aCaseSensitive) = 0; >- >+ Nix this change? > NS_IMETHOD Parse(nsIUnicharInputStream* aInput, ... >+ PRBool aAllowUnsafeRules, Maybe add a comment here pointing to the nsICSSLoader comment? Don't you need to also change the chrome registry code that reloads UA sheets? It lives in nsChromeRegistry::RefreshWindow (both chrome registries). Note that that code reloads both the UA and document sheets, so you'd probably want to add a boolean arg to LoadStyleSheetWithURL in the rdf/chrome chrome registry. There's t he usechromesheets thing in nsDocumentViewer... do we want to allow that to use unsafe rules? I'd say "no", even if it's adding UA sheets -- I don't have a good feel for when it's actually triggered. r+sr=bzbarsky with the nits picked and the chrome registries fixed. This looks great!

(In reply to comment #26) > Don't you need to also change the chrome registry code that reloads UA > sheets? It lives in nsChromeRegistry::RefreshWindow (both chrome registries). > Note that that code reloads both the UA and document sheets, Yes, but it reloads them in separate code paths, so I can just add PR_TRUE to the first LoadSheetSync and leave the other one as-is. Right?

> Yes, but it reloads them in separate code paths One of the chrome registries does... the other is a little (not much) more complicated because it has a helper function. The "patch for checkin" only fixes one chrome registry; fix the other one too, please.

oops, ok. Thanks

checked in.

Comment on attachment 225339 [details] [diff] [review] patch for checkin Please make sure to fix both chrome registries per bz's comment. approved for 1.8.0 branch, a=dveditz for drivers

Do we need to do anything to preserve interface compatibility? Note that whatever goes on 1.8.0 should also go on 1.8.

We can create nsICSSLoader_MOZILLA_1_8_BRANCH and nsICSSParser_MOZILLA_1_8_BRANCH.

IIRC usechromesheets only overrides the path to scrollbars.css in the theme.

Comment on attachment 225833 [details] [diff] [review] branch patch approval-branch-181=dbaron. Presumably this is what you're planning to check in to 1.8.0 as well. (If you still have it, it might be nice to see the interdiff between the result of merging to the branch and this patch.)

I merged some bits, but because function names changed, I just went ahead and created new interfaces without merging the rest, so that interdiff doesn't really exist.

Checked in on branches.

v.fixed on 1.8.0 branch with Mozilla/5.0 (Macintosh; U; Intel Mac OS X; en-US; rv:1.8.0.5) Gecko/20060626 Firefox/1.5.0.5, no crash with testcase.

Does this affect the 1.7/aviary branches?

Yes. I think the patch could be backported there without too much trouble.

https://bugzilla.mozilla.org/attachment.cgi?id=216430 ff2b2 windows, linux, macppc no crash https://bugzilla.mozilla.org/attachment.cgi?id=224024 ff2b2 windows, linux, macppc no crash

crash tests landed http://hg.mozilla.org/mozilla-central/rev/d8a03d62bd80