Last Comment Bug 331883 - Disable anonymous box selectors outside of UA stylesheets (was: Crash [@ nsContainerFrame::Destroy] [@ nsFrame::Destroy] involving ::-moz-line-frame (malformed view tree))
: Disable anonymous box selectors outside of UA stylesheets (was: Crash [@ nsCo...
Status: RESOLVED FIXED
[sg:critical]
: crash, testcase, verified1.8.0.5, verified1.8.1
Product: Core
Classification: Components
Component: Layout (show other bugs)
: Trunk
: PowerPC Mac OS X
: -- critical (vote)
: ---
Assigned To: Robert O'Callahan (:roc) (Exited; email my personal email if necessary)
:
Mentors:
: 201286 (view as bug list)
Depends on: 342167 342961 346222 347303
Blocks: randomclasses
  Show dependency treegraph
 
Reported: 2006-03-27 11:57 PST by Jesse Ruderman
Modified: 2011-06-13 10:01 PDT (History)
10 users (show)
dveditz: blocking1.7.14?
dveditz: blocking‑aviary1.0.9?
jaymoz: blocking1.8.0.5+
bob: in‑testsuite+
See Also:
Crash Signature:
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
testcase (1.28 KB, text/html)
2006-03-27 11:58 PST, Jesse Ruderman
no flags Details
Patch (787 bytes, patch)
2006-05-31 03:20 PDT, Leon Sha
no flags Details | Diff | Review
The view tree and the frame tree. (20.04 KB, text/plain)
2006-05-31 23:50 PDT, Leon Sha
no flags Details
The test case, crash on reload. (1.29 KB, text/html)
2006-05-31 23:51 PDT, Leon Sha
no flags Details
fix (22.73 KB, patch)
2006-06-08 17:18 PDT, Robert O'Callahan (:roc) (Exited; email my personal email if necessary)
dbaron: review+
dbaron: superreview+
Details | Diff | Review
fix (33.50 KB, patch)
2006-06-12 01:21 PDT, Robert O'Callahan (:roc) (Exited; email my personal email if necessary)
bzbarsky: review+
bzbarsky: superreview+
Details | Diff | Review
patch for checkin (34.59 KB, patch)
2006-06-12 15:59 PDT, Robert O'Callahan (:roc) (Exited; email my personal email if necessary)
dveditz: approval1.8.0.5+
Details | Diff | Review
branch patch (42.01 KB, patch)
2006-06-15 21:44 PDT, Robert O'Callahan (:roc) (Exited; email my personal email if necessary)
dbaron: approval‑branch‑1.8.1+
Details | Diff | Review

Description Jesse Ruderman 2006-03-27 11:57:59 PST
[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 2006-03-27 11:58:57 PST
Created attachment 216430 [details]
testcase
Comment 2 Daniel Veditz [:dveditz] 2006-03-27 12:57:20 PST
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 2006-05-29 17:45:27 PDT
Still crashes 2006-05-25 trunk nightly.
Comment 4 Leon Sha 2006-05-31 03:20:08 PDT
Created attachment 223898 [details] [diff] [review]
Patch
Comment 5 Leon Sha 2006-05-31 03:31:54 PDT
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) (Exited; email my personal email if necessary) 2006-05-31 14:36:48 PDT
Can you explain in more detail what's happening here and how the child is eventually destroyed?
Comment 7 Leon Sha 2006-05-31 23:50:03 PDT
Created attachment 224023 [details]
The view tree and the frame tree.
Comment 8 Leon Sha 2006-05-31 23:51:03 PDT
Created attachment 224024 [details]
The test case, crash on reload.
Comment 9 Leon Sha 2006-06-01 00:11:58 PDT
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) (Exited; email my personal email if necessary) 2006-06-01 15:20:14 PDT
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-7 (review requests must explain patch) 2006-06-05 19:05:09 PDT
(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 2006-06-05 19:16:25 PDT
> 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-7 (review requests must explain patch) 2006-06-05 19:54:23 PDT
(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] 2006-06-08 15:04:21 PDT
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) (Exited; email my personal email if necessary) 2006-06-08 15:17:36 PDT
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) (Exited; email my personal email if necessary) 2006-06-08 17:18:15 PDT
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.
Comment 17 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2006-06-08 17:31:33 PDT
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-7 (review requests must explain patch) 2006-06-08 17:56:24 PDT
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) (Exited; email my personal email if necessary) 2006-06-08 18:15:43 PDT
Boris, see David's comment #18.
Comment 20 Jesse Ruderman 2006-06-08 21:33:50 PDT
> +  // 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-7 (review requests must explain patch) 2006-06-08 21:39:59 PDT
Yeah, that might be good.  Boris probably has an opinion there too.
Comment 22 Boris Zbarsky [:bz] 2006-06-09 18:38:26 PDT
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 [:bz] 2006-06-09 18:42:45 PDT
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 2006-06-10 02:10:59 PDT
Re: comment 17
They are intended to be internal (but they might indeed be used by curious users).
Comment 25 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2006-06-12 01:21:36 PDT
Created attachment 225250 [details] [diff] [review]
fix

updated as suggested
Comment 26 Boris Zbarsky [:bz] 2006-06-12 09:51:23 PDT
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) (Exited; email my personal email if necessary) 2006-06-12 15:18:45 PDT
(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) (Exited; email my personal email if necessary) 2006-06-12 15:59:41 PDT
Created attachment 225339 [details] [diff] [review]
patch for checkin
Comment 29 Boris Zbarsky [:bz] 2006-06-12 17:45:40 PDT
> 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) (Exited; email my personal email if necessary) 2006-06-12 19:20:21 PDT
oops, ok. Thanks
Comment 31 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2006-06-14 20:20:42 PDT
checked in.
Comment 32 Daniel Veditz [:dveditz] 2006-06-15 15:00:38 PDT
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-7 (review requests must explain patch) 2006-06-15 15:14:28 PDT
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) (Exited; email my personal email if necessary) 2006-06-15 16:24:22 PDT
We can create nsICSSLoader_MOZILLA_1_8_BRANCH and nsICSSParser_MOZILLA_1_8_BRANCH.
Comment 35 neil@parkwaycc.co.uk 2006-06-15 16:35:00 PDT
IIRC usechromesheets only overrides the path to scrollbars.css in the theme.
Comment 36 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2006-06-15 21:44:43 PDT
Created attachment 225833 [details] [diff] [review]
branch patch
Comment 37 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2006-06-16 01:33:08 PDT
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) (Exited; email my personal email if necessary) 2006-06-16 02:16:35 PDT
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) (Exited; email my personal email if necessary) 2006-06-18 15:47:10 PDT
Checked in on branches.
Comment 40 Jay Patel [:jay] 2006-06-26 14:57:13 PDT
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] 2006-06-26 17:51:33 PDT
Does this affect the 1.7/aviary branches?
Comment 42 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2006-06-26 18:32:16 PDT
Yes. I think the patch could be backported there without too much trouble.
Comment 43 Bob Clary [:bc:] 2006-08-22 00:07:11 PDT
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 44 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2007-05-21 22:34:09 PDT
*** Bug 201286 has been marked as a duplicate of this bug. ***
Comment 45 Bob Clary [:bc:] 2009-04-24 11:15:37 PDT
crash tests landed
http://hg.mozilla.org/mozilla-central/rev/d8a03d62bd80

Note You need to log in before you can comment on or make changes to this bug.