2004021810/trunk/W2k Mozilla displays http://www.messagelabs.com/ as blank page, probably problem with CSS. I wanna turn CSS off, but crash occur. Repro: 1. open http://www.messagelabs.com/home/default.asp 2. run CSS toggling bookmarklet javascript:var i=0;if(document.styleSheets.length>0){cs=!document.styleSheets[0].disabled;for(i=0;i<document.styleSheets.length;i++) document.styleSheets[i].disabled=cs;};void(cs=true)

> I wanna turn CSS off First off, that's not what that bookmarklet does. But apart from that.... ;) I can reproduce the crash. Details once I have a stack.

Boris, I know that this bookmarklet is not 100% way to turn them off, but AFAIK is this bookmarklet best solution in Mozilla.

Fun stuff.... ###!!! ASSERTION: Error scroll port has too many children: 'GetFirstChild() == nsnull || GetFirstChild()->GetNextSibling() == nsnull', file /home/bzbarsky/mozilla/xlib/mozilla/view/src/nsScrollPortView.cpp, line 414 ###!!! ASSERTION: This hack should not be needed now!!! See bug 126263.: 'Error', file /home/bzbarsky/mozilla/xlib/mozilla/layout/html/base/src/nsContainerFrame.cpp, line 430 ###!!! ASSERTION: Don't try to move the root widget to something non-zero: 'GetParent() || (aX == 0 && aY == 0)', file /home/bzbarsky/mozilla/xlib/mozilla/view/src/nsView.cpp, line 293 ###!!! ASSERTION: null view: 'nsnull != aView', file /home/bzbarsky/mozilla/xlib/mozilla/view/src/nsViewManager.cpp, line 1702 That last assert is fatal: (gdb) frame #0 0x41626235 in nsViewManager::UpdateView(nsIView*, nsRect const&, unsigned) ( this=0x869a248, aView=0x0, aRect=@0xbfffc570, aUpdateFlags=4) at /home/bzbarsky/mozilla/xlib/mozilla/view/src/nsViewManager.cpp:1710 1710 view->GetClippedRect(clippedRect, isClipped, isEmpty); (gdb) p view $1 = (class nsView *) 0x0 We have some existing bugs that mention this crash.....

Adam, if you can possibly attach a reduced testcase, that would help a good bit..... (minimal HTML and stylesheets needed to reproduce the bug).

Is bug 210261 related?

The key seems to be the scrollframe on the root....

Related like a duplicate...

What's happening here is that every time we reframe the HTML element, we build another level of nested scrollframes, which also seems to be corrupt in various ways. It's easy to see this in the layout debugger, just get a frame dump between clicking on the text in the testcase. But shouldn't we just be applying the overflow:scroll to the viewport and ignoring it on the HTML element?

*** This bug has been marked as a duplicate of 210261 ***

Oops. That's not what I meant to do.

This testcase, which is absolutely static, seems to have problems. 1) We're not honouring the overflow-on-HTML-applies-to-viewport quirk, which I thought we did honor, and CSS2.1 allows us to honor 2) The presentation we have built doesn't work. I can't click on the viewport scrollbar, for example. 3) I can use keys to scroll the viewport ... sort of ... but the viewport scrollbar doesn't change. 4) Various assertions fire.

ooh, 5) press page down to get to the bottom of the page. Then resize the browser window to make it larger vertically. Hello twilight zone

There are two places scrollframes seem to be built here: nsCSSFrameConstructor::ConstructRootFrame and nsCSSFrameConstructor::ConstructDocElementFrame The latter is used when the root is reframed. I think it's also used when the frame for the root element is first constructed... In any case, it's not quite clear to me what the interaction of those two methods should be. In any case, the problem is probably that the scrolled frame, not the scrollframe, is set as the primary frame for the root node in this case. That means we never tear down the scrollframe when we reframe, which is probably whence the bogus multiple scrollframes arise. See nsCSSFrameConstructor::ConstructDocElementFrame; we probably need to set a flag of some sort when we BeginBuildingScrollFrame so we'll know that aParenFrame is the primary frame, not contentFrame.

Ah, we're honoring overflow:hidden set on HTML or BODY, but nothing else. So one approach to this bug would be to apply all overflow settings on the root element to the viewport, and never apply them to the root element (it would always be overflow:visible). Another approach would be to fix frame (re)construction like you suggest. What do you think?

For starters, I think I can't recall what the spec says about this nowadays...

The spec says simply > HTML UAs may apply the overflow property from the BODY or HTML elements to the > viewport. http://www.w3.org/TR/2004/CR-CSS21-20040225/visufx.html#overflow I guess, in theory, we shouldn't apply root element overflow settings to the viewport for XML documents --- although there's no other way to control the scrollbars of the viewport, AFAIK. That would leave us with a problem for XML documents. Or we could just ignore overflow on the XML document root element.

Also, I don't know if the setting of the primary frame explains the bugs I'm seeing in the static testcase.

Yeah, the static testcase is due to some other problem in the whole setup. Note that problem #1 in comment 13 is due to the fact that we only propagate the "hidden" value of overflow to the viewport scrollbar, basically.

I think it would not be hard to disable scrolling on the root element itself and apply the overflow settings to the viewport. Maybe it's not quite the right thing to do for XML documents but I don't think it really matters --- even in that situation it's mostly going to do what people expect. Suppose we just deleted this chunk of code here: http://lxr.mozilla.org/mozilla/source/layout/html/style/src/nsCSSFrameConstructor.cpp#3326 and arranged for the viewport's nsGfxScrollFrame to pull its overflow style from the document root element. (Note that ConstructDocElementFrame is used even in the inital frame construction: the pres shell does ConstructRootFrame and calls nsCSSFrameConstructor::ContentInserted with the root element, which invokes ConstructDocElementFrame.) For dynamic changes we'd have to make any reframe on the root element recreate the entire frame tree for the document. I don't know how easy that is. I'll look more at this tomorrow.

The working group hasn't got that far yet (as in, we've discussed it, have tentative consensus, but haven't put it in any public draft yet), but the answer you're looking for is @-moz-viewport { overflow: auto; }.

This patch handles the static case for HTML elements. It does not handle BODY propagation yet.

This also needs to not hook up <HTML> (and <BODY>) if the overflow style is 'visible'. And there may be some issues with the interaction with a container's scroll preferences.

This is it. Handles dynamic style changes, BODY, the works.

This testcase lets you switch through all the combinations dynamically. It works now!!

So what behavior does that implement? Mapping the root's overflow style to the vieport?

Yes. If the root element has overflow != visible, then we apply that to the viewport. if the root element has overflow visible and it's an HTML document and the BODY element has overflow != visible, then we apply that to the viewport. If an element has its overflow applied to the viewport, then it is treated as overflow:visible. The logic in PropagateScrollToViewport should be fairly clear. Hmm, this patch has a bug, I think: it should not propagate the HTML or BODY overflow style to the viewport if we're in print preview or printing.

Don't mess with the viewport scroll state in print/print preview

I think we should consider putting this in 1.7. It fixes crashers, makes our behaviour far more consistent (both internally and with CSS2.1), and is more IE compatible.

Comment on attachment 145386 [details] [diff] [review] revised >+ const nsStyleDisplay* display = styleContext->GetStyleDisplay(); >+ if (display->mOverflow != NS_STYLE_OVERFLOW_VISIBLE) { >+ aPresContext->SetViewportOverflowOverride(display->mOverflow); This would probably be cleaner as: PRUint8 overflow = styleContext->GetStyleDisplay()->mOverflow; if (overflow != NS_STYLE_OVERFLOW_VISIBLE) { aPresContext->SetViewportOverflowOverride(overflow); (old habits from when getting style data required an out param?) >+ display = bodyContext->GetStyleDisplay(); >+ if (display->mOverflow != NS_STYLE_OVERFLOW_VISIBLE) { >+ aPresContext->SetViewportOverflowOverride(display->mOverflow); likewise here. >+ // This isn't quite right. The scrollframe will still be scrollable using keys. >+ // This can happen when HTML or BODY has propagated this style to the viewport. Might be worth commenting on the bug about making scrollframes scrollable using keys -- we want to scroll any with 'overflow: auto' or 'overflow: scroll', but not those with 'overflow: hidden', even if it's the root. (Or maybe not?) The can all be scrolled by selecting text, though.

Yeah, and we don't want mousewheel to scroll overflow:hidden either. I'll fix the code as you suggest.

Comment on attachment 145386 [details] [diff] [review] revised I want to consider this patch for 1.7. The bad news is that it's a non-trivial change to some pretty important code. The good news is that it fixes some crashers and makes our behaviour more internally consistent, more CSS 2.1 compliant, and more IE compatible.

Comment on attachment 145386 [details] [diff] [review] revised a=asa (on behalf of drivers) for checkin to 1.7

Checked in on trunk. I'll wait for the 1.7 branch Tinderbox to appear before checking in on the 1.7 branch.

Checked into branch.

This checkin may have broken overflow:hidden on fixed-position elements (bug 241304). Also, is there a good reason why you don't check the the "body" in ConstructFrameByDisplayType is at least an HTML node before doing all the work of PropagateScrollToViewport?

OK, I've verified that this checkin did in fact cause bug 241304. Details there.

This also caused bug 241405 to be filed (though it's not clear how much of that is valid).

> Also, is there a good reason why you don't check the the "body" in > ConstructFrameByDisplayType is at least an HTML node before doing all the work > of PropagateScrollToViewport? Sure, we could do that, but how likely is it that a non-HTML "body" element is going to show up frequently enough to affect performance?

Well.. that depends on the XML dialect in use. Anywhere where one would appear at all, it could appear a whole bunch of times...

True, but I'm having trouble making myself care.

Either way. It's not a huge deal... it's just that jst and I have had a lot of trouble with places that incorrectly made assumptions about stuff on the basis of tagnames, and we'd rather that new code going into the tree used IsContentOfType or namespace checks as needed to make it clear what's going on...

If we don't check it now we'll eventually have to check it anyway when someone files a bug on it...

OK, here's the obvious fix just in case someone files a bug "loading large XML document with lots of mynamespace:body elements is fractionally slower than the same document using the tag name 'bdy' instead of 'body'" :-)

I was thinking more along the lines of "'overflow' on <foo:body> is propagated to the viewport but on <foo:bar> it is not". (Would this happen with _any_ <body> or just ones that have the root element as the parent? If the former, this could get some weird results even if it is an HTML body, depending on what people do to the DOM or in XHTML.)

(In reply to comment #46) > I was thinking more along the lines of "'overflow' on <foo:body> is propagated > to the viewport but on <foo:bar> it is not". That can't happen, because the function that does overflow propagation _does_ check for HTML-ness. The check I was talking about is pure optimization.

Comment on attachment 146907 [details] [diff] [review] fix non-HTML body element performance r+sr=bzbarsky. I still think we should check this in.

I'll check it in.

checked in.

Verified as fix (no crash) on latest 1.7 branch Win06-24, Mac06-30 & Linux06-29 builds. Changing keywords from fixed1.7 to verified1.7. Leave this bug status "as is" until this bug be verified on trunk again...

*** Bug 93520 has been marked as a duplicate of this bug. ***

layout/base/crashtests/234851-1.html layout/base/crashtests/234851-2.html http://hg.mozilla.org/mozilla-central/rev/b0337b6287f3