Last Comment Bug 234851 - crash on messagelabs.com after toggling CSS via bookmarklet
: crash on messagelabs.com after toggling CSS via bookmarklet
Status: RESOLVED FIXED
: crash, testcase, verified1.7
Product: Core
Classification: Components
Component: Layout: View Rendering (show other bugs)
: Trunk
: All All
: P1 critical (vote)
: ---
Assigned To: Robert O'Callahan (:roc) (Exited; email my personal email if necessary)
: Hixie (not reading bugmail)
Mentors:
http://www.messagelabs.com/home/defau...
: 93520 (view as bug list)
Depends on: 618926
Blocks: 93520 210261 230554 241304 241582
  Show dependency treegraph
 
Reported: 2004-02-18 21:20 PST by Adam Hauner
Modified: 2010-12-13 15:49 PST (History)
6 users (show)
bob: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Full stack (12.64 KB, text/plain)
2004-02-18 21:44 PST, Boris Zbarsky [:bz]
no flags Details
testcase - one click to crash (474 bytes, text/html)
2004-02-18 22:41 PST, Adam Hauner
no flags Details
testcase - three click to crash (452 bytes, text/html)
2004-02-18 22:41 PST, Adam Hauner
no flags Details
A bit smaller (one click to crash) (356 bytes, text/html)
2004-02-18 22:56 PST, Boris Zbarsky [:bz]
no flags Details
testcase (488 bytes, text/html)
2004-04-01 19:51 PST, Robert O'Callahan (:roc) (Exited; email my personal email if necessary)
no flags Details
here's a start (6.15 KB, patch)
2004-04-02 05:59 PST, Robert O'Callahan (:roc) (Exited; email my personal email if necessary)
no flags Details | Diff | Review
complete fix (13.52 KB, patch)
2004-04-03 10:44 PST, Robert O'Callahan (:roc) (Exited; email my personal email if necessary)
no flags Details | Diff | Review
dynamic testcase (971 bytes, text/html)
2004-04-03 10:45 PST, Robert O'Callahan (:roc) (Exited; email my personal email if necessary)
no flags Details
revised (13.67 KB, patch)
2004-04-03 11:29 PST, Robert O'Callahan (:roc) (Exited; email my personal email if necessary)
dbaron: review+
dbaron: superreview+
asa: approval1.7+
Details | Diff | Review
fix non-HTML body element performance (1.17 KB, patch)
2004-04-24 05:06 PDT, Robert O'Callahan (:roc) (Exited; email my personal email if necessary)
bzbarsky: review+
bzbarsky: superreview+
Details | Diff | Review

Description Adam Hauner 2004-02-18 21:20:32 PST
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 [:bz] 2004-02-18 21:32:53 PST
> 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 2004-02-18 21:38:43 PST
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 [:bz] 2004-02-18 21:42:01 PST
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 [:bz] 2004-02-18 21:44:23 PST
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).
Comment 5 Adam Hauner 2004-02-18 22:41:37 PST
Created attachment 141722 [details]
testcase - one click to crash
Comment 6 Adam Hauner 2004-02-18 22:41:59 PST
Created attachment 141723 [details]
testcase - three click to crash
Comment 7 Adam Hauner 2004-02-18 22:45:46 PST
Is bug 210261 related?
Comment 8 Boris Zbarsky [:bz] 2004-02-18 22:56:05 PST
Created attachment 141724 [details]
A bit smaller (one click to crash)

The key seems to be the scrollframe on the root....
Comment 9 Boris Zbarsky [:bz] 2004-02-18 22:58:45 PST
Related like a duplicate...
Comment 10 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2004-04-01 19:44:30 PST
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 [:bz] 2004-04-01 19:47:38 PST

*** This bug has been marked as a duplicate of 210261 ***
Comment 12 Boris Zbarsky [:bz] 2004-04-01 19:48:07 PST
Oops.  That's not what I meant to do.
Comment 13 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2004-04-01 19:51:11 PST
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.
Comment 14 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2004-04-01 19:54:41 PST
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 [:bz] 2004-04-01 20:05:33 PST
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) (Exited; email my personal email if necessary) 2004-04-01 20:40:07 PST
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 [:bz] 2004-04-01 20:48:10 PST
For starters, I think I can't recall what the spec says about this nowadays...
Comment 18 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2004-04-01 20:50:50 PST
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) (Exited; email my personal email if necessary) 2004-04-01 20:52:17 PST
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 [:bz] 2004-04-01 20:54:48 PST
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) (Exited; email my personal email if necessary) 2004-04-01 21:10:13 PST
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) 2004-04-02 01:35:53 PST
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) (Exited; email my personal email if necessary) 2004-04-02 05:59:02 PST
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.
Comment 24 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2004-04-02 06:31:05 PST
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) (Exited; email my personal email if necessary) 2004-04-03 10:44:07 PST
Created attachment 145384 [details] [diff] [review]
complete fix

This is it. Handles dynamic style changes, BODY, the works.
Comment 26 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2004-04-03 10:45:28 PST
Created attachment 145385 [details]
dynamic testcase

This testcase lets you switch through all the combinations dynamically. It
works now!!
Comment 27 Boris Zbarsky [:bz] 2004-04-03 10:48:30 PST
So what behavior does that implement?  Mapping the root's overflow style to the
vieport?
Comment 28 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2004-04-03 11:05:57 PST
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) (Exited; email my personal email if necessary) 2004-04-03 11:29:34 PST
Created attachment 145386 [details] [diff] [review]
revised

Don't mess with the viewport scroll state in print/print preview
Comment 30 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2004-04-03 13:18:34 PST
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-7 (review requests must explain patch) 2004-04-09 14:12:53 PDT
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) (Exited; email my personal email if necessary) 2004-04-09 14:16:04 PDT
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) (Exited; email my personal email if necessary) 2004-04-09 14:20:29 PDT
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] 2004-04-12 16:12:20 PDT
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) (Exited; email my personal email if necessary) 2004-04-12 20:01:20 PDT
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) (Exited; email my personal email if necessary) 2004-04-13 15:30:08 PDT
Checked into branch.
Comment 37 Boris Zbarsky [:bz] 2004-04-22 09:58:22 PDT
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 [:bz] 2004-04-22 11:14:28 PDT
OK, I've verified that this checkin did in fact cause bug 241304. Details there.
Comment 39 Boris Zbarsky [:bz] 2004-04-22 20:12:45 PDT
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) (Exited; email my personal email if necessary) 2004-04-23 20:34:37 PDT
> 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 [:bz] 2004-04-23 21:27:20 PDT
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) (Exited; email my personal email if necessary) 2004-04-23 21:52:04 PDT
True, but I'm having trouble making myself care.
Comment 43 Boris Zbarsky [:bz] 2004-04-23 22:09:50 PDT
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) 2004-04-24 01:04:07 PDT
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) (Exited; email my personal email if necessary) 2004-04-24 05:06:58 PDT
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'" :-)
Comment 46 Hixie (not reading bugmail) 2004-04-24 06:52:05 PDT
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 [:bz] 2004-04-24 08:22:21 PDT
(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 [:bz] 2004-04-24 08:22:42 PDT
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) (Exited; email my personal email if necessary) 2004-04-24 09:53:04 PDT
I'll check it in.
Comment 50 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2004-04-24 10:03:00 PDT
checked in.
Comment 51 Karen Huang 2004-07-02 13:38:55 PDT
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-7 (review requests must explain patch) 2005-04-11 22:19:57 PDT
*** Bug 93520 has been marked as a duplicate of this bug. ***
Comment 53 Bob Clary [:bc:] 2009-05-09 10:50:00 PDT
layout/base/crashtests/234851-1.html
layout/base/crashtests/234851-2.html
http://hg.mozilla.org/mozilla-central/rev/b0337b6287f3

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