The default bug view has changed. See this FAQ.

Disable anonymous box selectors outside of UA stylesheets (was: Crash [@ nsContainerFrame::Destroy] [@ nsFrame::Destroy] involving ::-moz-line-frame (malformed view tree))

RESOLVED FIXED

Status

()

Core
Layout
--
critical
RESOLVED FIXED
11 years ago
6 years ago

People

(Reporter: Jesse Ruderman, Assigned: roc)

Tracking

(Blocks: 1 bug, 4 keywords)

Trunk
PowerPC
Mac OS X
crash, testcase, verified1.8.0.5, verified1.8.1
Points:
---
Dependency tree / graph
Bug Flags:
blocking1.7.14 ?
blocking-aviary1.0.9 ?
blocking1.8.0.5 +
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [sg:critical], crash signature)

Attachments

(5 attachments, 3 obsolete attachments)

(Reporter)

Description

11 years ago
[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
...
(Reporter)

Updated

11 years ago
Whiteboard: [sg:critical]
(Reporter)

Comment 1

11 years ago
Created attachment 216430 [details]
testcase
(Reporter)

Updated

11 years ago
Summary: Crash [@ nsContainerFrame::Destroy] involving CSS positioning, :first-line, ::-moz-line-frame → Crash [@ nsContainerFrame::Destroy] [@ nsFrame::Destroy] involving CSS positioning, :first-line, ::-moz-line-frame
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'.
(Reporter)

Updated

11 years ago
Blocks: 331889
(Reporter)

Comment 3

11 years ago
Still crashes 2006-05-25 trunk nightly.
Flags: blocking1.9a1?
Flags: blocking1.8.0.5?

Comment 4

11 years ago
Created attachment 223898 [details] [diff] [review]
Patch

Comment 5

11 years ago
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.

Updated

11 years ago
Attachment #223898 - Flags: review?(roc)
Can you explain in more detail what's happening here and how the child is eventually destroyed?

Comment 7

11 years ago
Created attachment 224023 [details]
The view tree and the frame tree.

Comment 8

11 years ago
Created attachment 224024 [details]
The test case, crash on reload.

Comment 9

11 years ago
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?
(Reporter)

Updated

11 years ago
Summary: Crash [@ nsContainerFrame::Destroy] [@ nsFrame::Destroy] involving CSS positioning, :first-line, ::-moz-line-frame → Crash [@ nsContainerFrame::Destroy] [@ nsFrame::Destroy] involving CSS positioning, :first-line, ::-moz-line-frame (malformed view tree)
(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.
(Reporter)

Comment 12

11 years ago
> 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.

Comment 14

11 years ago
roc: Can you take a look at the patch?  We want a fix for this for 1.8.0.5.
Flags: blocking1.8.0.5? → blocking1.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.
Created attachment 224941 [details] [diff] [review]
fix

Boris, this is (I think) a fairly simple approach to disabling anonymous box selectors outside of UA (and editor override) style sheets.
Assignee: nobody → roc
Status: NEW → ASSIGNED
Attachment #224941 - Flags: superreview?(bzbarsky)
Attachment #224941 - Flags: review?(bzbarsky)
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.
Attachment #224941 - Flags: superreview?(dbaron)
Attachment #224941 - Flags: superreview?(bzbarsky)
Attachment #224941 - Flags: review?(dbaron)
Attachment #224941 - Flags: review?(bzbarsky)
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.
Attachment #224941 - Flags: superreview?(dbaron)
Attachment #224941 - Flags: superreview+
Attachment #224941 - Flags: review?(dbaron)
Attachment #224941 - Flags: review+
Boris, see David's comment #18.
(Reporter)

Comment 20

11 years ago
> +  // 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.

Updated

11 years ago
Attachment #223898 - Attachment is obsolete: true
Attachment #223898 - Flags: review?(roc)
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).

Comment 24

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

Updated

11 years ago
Summary: Crash [@ nsContainerFrame::Destroy] [@ nsFrame::Destroy] involving CSS positioning, :first-line, ::-moz-line-frame (malformed view tree) → Disable anonymous box selectors outside of UA stylesheets (was: Crash [@ nsContainerFrame::Destroy] [@ nsFrame::Destroy] involving ::-moz-line-frame (malformed view tree))
Created attachment 225250 [details] [diff] [review]
fix

updated as suggested
Attachment #225250 - Flags: superreview?(bzbarsky)
Attachment #225250 - Flags: review?(bzbarsky)
Attachment #224941 - Attachment is obsolete: true
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!
Attachment #225250 - Flags: superreview?(bzbarsky)
Attachment #225250 - Flags: superreview+
Attachment #225250 - Flags: review?(bzbarsky)
Attachment #225250 - Flags: review+
(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?

Created attachment 225339 [details] [diff] [review]
patch for checkin
Attachment #225250 - Attachment is obsolete: true
> 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.
Status: ASSIGNED → RESOLVED
Last Resolved: 11 years ago
Resolution: --- → FIXED
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
Attachment #225339 - Flags: approval1.8.0.5+
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.

Comment 35

11 years ago
IIRC usechromesheets only overrides the path to scrollbars.css in the theme.
Created attachment 225833 [details] [diff] [review]
branch patch
Attachment #225833 - Flags: approval-branch-1.8.1?(dbaron)
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.)
Attachment #225833 - Flags: approval-branch-1.8.1?(dbaron) → approval-branch-1.8.1+
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.
Keywords: fixed1.8.0.5, fixed1.8.1
Depends on: 342167

Comment 40

11 years ago
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.
Keywords: fixed1.8.0.5 → verified1.8.0.5
Does this affect the 1.7/aviary branches?
Flags: blocking1.7.14?
Flags: blocking-aviary1.0.9?
Yes. I think the patch could be backported there without too much trouble.
Depends on: 342961
Depends on: 346222
Depends on: 347303

Comment 43

11 years ago
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


Keywords: fixed1.8.1 → verified1.8.1
Duplicate of this bug: 201286
Group: security

Updated

9 years ago
Flags: blocking1.9a1?

Comment 45

8 years ago
crash tests landed
http://hg.mozilla.org/mozilla-central/rev/d8a03d62bd80
Flags: in-testsuite+
Crash Signature: [@ nsContainerFrame::Destroy] [@ nsFrame::Destroy]
You need to log in before you can comment on or make changes to this bug.