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

Status: RESOLVED FIXED

(Core :: Layout, defect)

Description

[Jesse Ruderman]

[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
...

Comment 1

[Jesse Ruderman]

Attachment: testcase

Comment 2

[Daniel Veditz [:dveditz]]

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'.

Comment 3

[Jesse Ruderman]

Still crashes 2006-05-25 trunk nightly.

Comment 4

[Leon Sha]

Attachment: Patch

Comment 5

[Leon Sha]

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.

Comment 6

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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

Comment 7

[Leon Sha]

Attachment: The view tree and the frame tree.

Comment 8

[Leon Sha]

Attachment: The test case, crash on reload.

Comment 9

[Leon Sha]

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.

Comment 10

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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?

Comment 11

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

(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.

Comment 12

[Jesse Ruderman]

> 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 { }".

Comment 13

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

(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

[Jay Patel [:jay]]

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

Comment 15

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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.

Comment 16

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: fix

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

Comment 17

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

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 18

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

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.

Comment 19

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Boris, see David's comment #18.

Comment 20

[Jesse Ruderman]

> +  // 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.

Comment 21

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

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

Comment 22

[Boris Zbarsky [:bzbarsky]]

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.  :(

Comment 23

[Boris Zbarsky [:bzbarsky]]

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

[rbs]

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

Comment 25

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: fix

updated as suggested

Comment 26

[Boris Zbarsky [: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!

Comment 27

[Robert O'Callahan (:roc) (email my personal email if necessary)]

(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?

Comment 28

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: patch for checkin

Comment 29

[Boris Zbarsky [:bzbarsky]]

> 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.

Comment 30

[Robert O'Callahan (:roc) (email my personal email if necessary)]

oops, ok. Thanks

Comment 31

[Robert O'Callahan (:roc) (email my personal email if necessary)]

checked in.

Comment 32

[Daniel Veditz [:dveditz]]

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

Comment 33

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

Do we need to do anything to preserve interface compatibility?

Note that whatever goes on 1.8.0 should also go on 1.8.

Comment 34

[Robert O'Callahan (:roc) (email my personal email if necessary)]

We can create nsICSSLoader_MOZILLA_1_8_BRANCH and nsICSSParser_MOZILLA_1_8_BRANCH.

Comment 35

[[e-mail]]

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

Comment 36

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: branch patch

Comment 37

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

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.)

Comment 38

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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.

Comment 39

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Checked in on branches.

Comment 40

[Jay Patel [:jay]]

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.

Comment 41

[Daniel Veditz [:dveditz]]

Does this affect the 1.7/aviary branches?

Comment 42

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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

Comment 43

[Bob Clary [:bc] (inactive)]

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

Comment 45

[Bob Clary [:bc] (inactive)]

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