234851 - crash on messagelabs.com after toggling CSS via bookmarklet

Status: RESOLVED FIXED

(Core :: Web Painting, defect, P1)

Description

[Adam Hauner]

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)

Comment 1

[Boris Zbarsky [:bzbarsky]]

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

Comment 2

[Adam Hauner]

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

Comment 3

[Boris Zbarsky [:bzbarsky]]

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

Comment 4

[Boris Zbarsky [:bzbarsky]]

Attachment: Full stack

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

Comment 5

[Adam Hauner]

Attachment: testcase - one click to crash

Comment 6

[Adam Hauner]

Attachment: testcase - three click to crash

Comment 7

[Adam Hauner]

Is bug 210261 related?

Comment 8

[Boris Zbarsky [:bzbarsky]]

Attachment: A bit smaller (one click to crash)

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

Comment 9

[Boris Zbarsky [:bzbarsky]]

Related like a duplicate...

Comment 10

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

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?

Comment 11

[Boris Zbarsky [:bzbarsky]]

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

Comment 12

[Boris Zbarsky [:bzbarsky]]

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

Comment 13

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

Attachment: testcase

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.

Comment 14

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

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

Comment 15

[Boris Zbarsky [:bzbarsky]]

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.

Comment 16

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

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?

Comment 17

[Boris Zbarsky [:bzbarsky]]

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

Comment 18

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

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.

Comment 19

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

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

Comment 20

[Boris Zbarsky [:bzbarsky]]

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.

Comment 21

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

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.

Comment 22

[Hixie (not reading bugmail)]

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

Comment 23

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

Attachment: here's a start

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

Comment 24

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

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.

Comment 25

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

Attachment: complete fix

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

Comment 26

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

Attachment: dynamic testcase

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

Comment 27

[Boris Zbarsky [:bzbarsky]]

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

Comment 28

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

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.

Comment 29

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

Attachment: revised

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

Comment 30

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

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 31

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

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.

Comment 32

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

Yeah, and we don't want mousewheel to scroll overflow:hidden either.

I'll fix the code as you suggest.

Comment 33

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

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 34

[Asa Dotzler [:asa]]

Comment on attachment 145386 [details] [diff] [review]
revised

a=asa (on behalf of drivers) for checkin to 1.7

Comment 35

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

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

Comment 36

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

Checked into branch.

Comment 37

[Boris Zbarsky [:bzbarsky]]

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? 

Comment 38

[Boris Zbarsky [:bzbarsky]]

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

Comment 39

[Boris Zbarsky [:bzbarsky]]

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

Comment 40

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

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

Comment 41

[Boris Zbarsky [:bzbarsky]]

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

Comment 42

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

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

Comment 43

[Boris Zbarsky [:bzbarsky]]

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

Comment 44

[Hixie (not reading bugmail)]

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

Comment 45

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

Attachment: fix non-HTML body element performance

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'" :-)

Comment 46

[Hixie (not reading bugmail)]

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

Comment 47

[Boris Zbarsky [:bzbarsky]]

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

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 146907 [details] [diff] [review]
fix non-HTML body element performance

r+sr=bzbarsky.	I still think we should check this in.

Comment 49

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

I'll check it in.

Comment 50

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

checked in.

Comment 51

[Karen Huang]

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

Comment 52

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

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

Comment 53

[Bob Clary [:bc] (inactive)]

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