crash on messagelabs.com after toggling CSS via bookmarklet

RESOLVED FIXED

Status

()

Core
Layout: View Rendering
P1
critical
RESOLVED FIXED
14 years ago
7 years ago

People

(Reporter: Adam Hauner, Assigned: roc)

Tracking

({crash, testcase, verified1.7})

Trunk
crash, testcase, verified1.7
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

(URL)

Attachments

(6 attachments, 4 obsolete attachments)

(Reporter)

Description

14 years ago
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.
(Reporter)

Comment 2

14 years ago
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.....
Assignee: general → roc
Component: Browser-General → Layout: View Rendering
OS: Windows 2000 → All
QA Contact: general → ian
Hardware: PC → All
Created attachment 141717 [details]
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).
(Reporter)

Comment 5

14 years ago
Created attachment 141722 [details]
testcase - one click to crash
(Reporter)

Comment 6

14 years ago
Created attachment 141723 [details]
testcase - three click to crash
(Reporter)

Updated

14 years ago
Keywords: testcase
(Reporter)

Comment 7

14 years ago
Is bug 210261 related?
Created attachment 141724 [details]
A bit smaller (one click to crash)

The key seems to be the scrollframe on the root....
Attachment #141722 - Attachment is obsolete: true
Attachment #141723 - Attachment is obsolete: true
Related like a duplicate...
Priority: -- → P2
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 ***
Status: NEW → RESOLVED
Last Resolved: 14 years ago
Resolution: --- → DUPLICATE
Oops.  That's not what I meant to do.
Blocks: 210261
Status: RESOLVED → REOPENED
Resolution: DUPLICATE → ---
Created attachment 145307 [details]
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.
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; }.
Created attachment 145332 [details] [diff] [review]
here's a start

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.
Created attachment 145384 [details] [diff] [review]
complete fix

This is it. Handles dynamic style changes, BODY, the works.
Created attachment 145385 [details]
dynamic testcase

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.
Attachment #145385 - Attachment is patch: false
Attachment #145385 - Attachment mime type: text/plain → text/html
Created attachment 145386 [details] [diff] [review]
revised

Don't mess with the viewport scroll state in print/print preview
Attachment #145332 - Attachment is obsolete: true
Attachment #145384 - Attachment is obsolete: true
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.
Attachment #145386 - Flags: superreview?(dbaron)
Attachment #145386 - Flags: review?(dbaron)
Blocks: 93520
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.
Attachment #145386 - Flags: superreview?(dbaron)
Attachment #145386 - Flags: superreview+
Attachment #145386 - Flags: review?(dbaron)
Attachment #145386 - Flags: review+
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.
Attachment #145386 - Flags: approval1.7?
Priority: P2 → P1
Blocks: 230554

Comment 34

14 years ago
Comment on attachment 145386 [details] [diff] [review]
revised

a=asa (on behalf of drivers) for checkin to 1.7
Attachment #145386 - Flags: approval1.7? → approval1.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.
Status: REOPENED → RESOLVED
Last Resolved: 14 years ago14 years ago
Resolution: --- → FIXED
Keywords: fixed1.7
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? 
Blocks: 241304
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...
Created attachment 146907 [details] [diff] [review]
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'" :-)
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.
Attachment #146907 - Flags: superreview+
Attachment #146907 - Flags: review+
I'll check it in.
checked in.
Blocks: 241582

Comment 51

13 years ago
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...
Keywords: fixed1.7 → verified1.7
*** Bug 93520 has been marked as a duplicate of this bug. ***

Comment 53

8 years ago
layout/base/crashtests/234851-1.html
layout/base/crashtests/234851-2.html
http://hg.mozilla.org/mozilla-central/rev/b0337b6287f3
Flags: in-testsuite+
Depends on: 618926
You need to log in before you can comment on or make changes to this bug.