Closed Bug 331883 Opened 14 years ago Closed 13 years ago

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

Categories

(Core :: Layout, defect, critical)

PowerPC
macOS
defect
Not set
critical

Tracking

()

RESOLVED FIXED

People

(Reporter: jruderman, Assigned: roc)

References

(Blocks 1 open bug)

Details

(4 keywords, Whiteboard: [sg:critical])

Crash Data

Attachments

(5 files, 3 obsolete files)

[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
...
Whiteboard: [sg:critical]
Attached file testcase
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'.
Still crashes 2006-05-25 trunk nightly.
Flags: blocking1.9a1?
Flags: blocking1.8.0.5?
Attached patch Patch (obsolete) — Splinter Review
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.
Attachment #223898 - Flags: review?(roc)
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?
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.
> 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.
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.
Attached patch fix (obsolete) — Splinter Review
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+
> +  // 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.
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).
Re: comment 17
They are intended to be internal (but they might indeed be used by curious users).
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))
Attached patch fix (obsolete) — Splinter Review
updated as suggested
Attachment #225250 - Flags: superreview?(bzbarsky)
Attachment #225250 - Flags: review?(bzbarsky)
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?

> 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.
checked in.
Status: ASSIGNED → RESOLVED
Closed: 13 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.
IIRC usechromesheets only overrides the path to scrollbars.css in the theme.
Attached patch branch patchSplinter Review
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.
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?
Flags: blocking1.7.14?
Flags: blocking-aviary1.0.9?
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


Duplicate of this bug: 201286
Group: security
Flags: blocking1.9a1?
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.