310919 - CSS in userContent.css crashes on XML (rss v2.0) display

Status: VERIFIED FIXED

(Core :: Layout, defect, P2)

Description

[Ulrik Sverdrup]

User-Agent:       Mozilla/5.0 (Macintosh; U; PPC Mac OS X Mach-O; en-US; rv:1.9a1) Gecko/20051001 Camino/1.0+
Build Identifier: Mozilla/5.0 (Macintosh; U; PPC Mac OS X Mach-O; en-US; rv:1.9a1) Gecko/20051001 Camino/1.0+

I found a reproducable bug that Quits Firefox (1.5b1) and Camino (trunk-Oct 1)
when displaying an XML document (RSS)

Reproducible: Always

Steps to Reproduce:
1. Put the following line in your userContent.css: 
rss * { display: block;}
2. Load the url above ( http://joi.ito.com/index.xml )


Actual Results:  
When the page is downloaded, just before display, the browser Quits and leaves
no trace (no console messages, no crashreporter, no Talkback (not even on
restartup))

Expected Results:  
The XML should be rendered with the css rule applied. Or at least the crash
should be detectable.

Comment 1

[Samuel Sidler (old account; do not CC)]

Since this crasher existed in 1.5b1, it's also on the branch. Marking as such
for more visible finding.

Also, changing severity to critical and adding crash keyword.

Comment 2

[Samuel Sidler (old account; do not CC)]

Confirming with 2005092804 branch build of Camino.

TB10111047W

Comment 3

[Samuel Sidler (old account; do not CC)]

First thread of crash stack below.

Thread 0 Crashed:
0   org.mozilla.camino       	0x0038fc0c nsFrame::BoxReflow(nsBoxLayoutState&,
nsPresContext*, nsHTMLReflowMetrics&, nsHTMLReflowState const&, unsigned&, int,
int, int, int, int) + 612
1   org.mozilla.camino       	0x0038f7e4 nsFrame::DoLayout(nsBoxLayoutState&) + 224
2   org.mozilla.camino       	0x00431e08 nsIFrame::Layout(nsBoxLayoutState&) + 52
3   org.mozilla.camino       	0x004daca8
nsBoxFrame::LayoutChildAt(nsBoxLayoutState&, nsIFrame*, nsRect const&) + 176
4   org.mozilla.camino       	0x0053f104
nsGfxScrollFrameInner::LayoutScrollbars(nsBoxLayoutState&, nsRect const&, nsRect
const&, nsRect const&) + 1072
5   org.mozilla.camino       	0x0053bde8
nsHTMLScrollFrame::Reflow(nsPresContext*, nsHTMLReflowMetrics&,
nsHTMLReflowState const&, unsigned&) + 1548
6   org.mozilla.camino       	0x00458e2c
nsContainerFrame::ReflowChild(nsIFrame*, nsPresContext*, nsHTMLReflowMetrics&,
nsHTMLReflowState const&, int, int, unsigned, unsigned&) + 148
7   org.mozilla.camino       	0x00536450 ViewportFrame::Reflow(nsPresContext*,
nsHTMLReflowMetrics&, nsHTMLReflowState const&, unsigned&) + 280
8   org.mozilla.camino       	0x002fae54 PresShell::InitialReflow(int, int) + 516
9   org.mozilla.camino       	0x004c4ad4 nsContentSink::StartLayout(int) + 208
10  org.mozilla.camino       	0x0047565c nsXMLContentSink::StartLayout() + 144
11  org.mozilla.camino       	0x00474248 nsXMLContentSink::DidBuildModel() + 456
12  org.mozilla.camino       	0x001c3c2c nsExpatDriver::DidBuildModel(unsigned,
int, nsIParser*, nsIContentSink*) + 56
13  org.mozilla.camino       	0x001a9618 nsParser::DidBuildModel(unsigned) + 120
14  org.mozilla.camino       	0x001aa8b0 nsParser::ResumeParse(int, int, int) + 592
15  org.mozilla.camino       	0x001abb00 nsParser::OnStopRequest(nsIRequest*,
nsISupports*, unsigned) + 192
16  org.mozilla.camino       	0x0020f420
nsDocumentOpenInfo::OnStopRequest(nsIRequest*, nsISupports*, unsigned) + 124
17  org.mozilla.camino       	0x000f0898
nsHttpChannel::OnStopRequest(nsIRequest*, nsISupports*, unsigned) + 680
18  org.mozilla.camino       	0x000cc8c0 nsInputStreamPump::OnStateStop() + 160
19  org.mozilla.camino       	0x000cc448
nsInputStreamPump::OnInputStreamReady(nsIAsyncInputStream*) + 128
20  libxpcom_core.dylib      	0x00e87194
nsAStreamCopier::PostContinuationEvent_Locked() + 1240
21  libxpcom_core.dylib      	0x00e4aa20 PL_HandleEvent + 36
22  libxpcom_core.dylib      	0x00e4a944 PL_ProcessPendingEvents + 128
23  com.apple.CoreFoundation 	0x9074bc8c __CFRunLoopDoSources0 + 384
24  com.apple.CoreFoundation 	0x9074b1bc __CFRunLoopRun + 452
25  com.apple.CoreFoundation 	0x9074ac3c CFRunLoopRunSpecific + 268
26  com.apple.HIToolbox      	0x93129ac0 RunCurrentEventLoopInMode + 264
27  com.apple.HIToolbox      	0x931290cc ReceiveNextEventCommon + 244
28  com.apple.HIToolbox      	0x93128fc0 BlockUntilNextEventMatchingListInMode + 96
29  com.apple.AppKit         	0x93631e44 _DPSNextEvent + 384
30  com.apple.AppKit         	0x93631b08 -[NSApplication
nextEventMatchingMask:untilDate:inMode:dequeue:] + 116
31  com.apple.AppKit         	0x9362e06c -[NSApplication run] + 472
32  com.apple.AppKit         	0x9371e8bc NSApplicationMain + 452
33  org.mozilla.camino       	0x0000aa54 start + 432
34  org.mozilla.camino       	0x0000a8d4 start + 48

Comment 4

[Ria Klaassen (not reading all bugmail)]

Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8b5) Gecko/20051002
Firefox/1.4.1 ID:2005100221

TB10111082K
TB10111002G

Crashes on trunk and branch.

Comment 5

[Mark Mentovai]

Regression since Gecko 1.7, does not occur in Firefox 1.0.7 - this should be on
someone's radar.

Comment 6

[Asa Dotzler [:asa]]

This doesn't look like a branch topcrasher. It's number 509 in firefox 1.5b1 and
there's only one nightly incident from the branch. There does seem to be a fair
number of recent _trunk_ nightly crashers here.

Jay, can you dig into this a bit more? If it does look like a branch topcrasher,
please re-nominate (also, note that talkback is about 20K incidents behind so
that might be masking some branch incidents.)

Comment 7

[Boris Zbarsky [:bzbarsky]]

Attachment: Fix

The problem is that this is styling the scrollcorner to be a block, which
crashes when we try to call XUL layout methods on it in nsGfxScrollFrame. 
Scrollbars are not a problem because they create a special frame anyway,
instead of constructing by display.

I _think_ this is the safest patch I can do here, but it does prevent
style="display: none" from working on scrollcorners.  If that's not acceptable,
I'd have to change nsCSSFrameConstructor to always construct a box here based
on tagname...

Comment 8

[Asa Dotzler [:asa]]

boris, dbaron, neil, I'm not seeing this high on the branch crash list. Do you
guys think the branch code is equally effected and if so, and this needs to land
on the branch, what's the risk associated with this patch?

Comment 9

[Boris Zbarsky [:bzbarsky]]

The branch code is affected, yes.  This will only bite users who either change
their user stylesheet in a silly way or install an extension that does it for
them (which is easy for extensions now with nsIStyleSheetService).

The risk associated with the patch is pretty minimal, in my opinion...

Comment 10

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

Seems like this will still crash if the scrollcorner has hidden="true", since
the page will override the 'display:none'.

Comment 11

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

(Which could be fixed by removing the :not() and adding a separate hidden="true"
rule following.)


And I agree this patch is low risk.

Comment 12

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

That said, the real problem here is bug 286895.

(Are scrollcorners hidden="true" when both scrollbars are hidden?  If so, then
that's a case we probably need to worry about if we actually want to fix this.)

Comment 13

[Boris Zbarsky [:bzbarsky]]

Yeah, I could do a separate hidden="true" rule.

Not sure what you mean by the second part of comment 12, David.

Comment 14

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

Just trying to figure out when a scrollcorner has hidden="true".

Comment 15

[Boris Zbarsky [:bzbarsky]]

Ah.  I don't really know whether it ever would, really; I was acting on the
assumption that it might sometimes and that we shouldn't break those cases.  So
I agree we should do a separate scrollcorner[hidden="true"] rule here.

Comment 16

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

sr=dbaron with the change in comment 11

Comment 17

[Boris Zbarsky [:bzbarsky]]

Attachment: Updated to dbaron's comments

Comment 18

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 198517 [details] [diff] [review]
Updated to dbaron's comments

This has r=dbaron, sr=neil.  It's a pretty safe crash fix.

If it's too late for 1.8b5, I'd say just take this for rc1 instead...

Comment 19

[Boris Zbarsky [:bzbarsky]]

Fixed on trunk.

Comment 20

[Asa Dotzler [:asa]]

we'll try to get this early in RC1.

Comment 21

[Asa Dotzler [:asa]]

Comment on attachment 198517 [details] [diff] [review]
Updated to dbaron's comments

moving request to 1.8rc1 since 1.8b5 has shipped.

Comment 22

[Asa Dotzler [:asa]]

Comment on attachment 198517 [details] [diff] [review]
Updated to dbaron's comments

We'll watch talkback and if this shows up as more of a problem, we'll
reconsider. Too late for taking _any_ risk for non-topcrashers at this poing.

Comment 23

[Samuel Sidler (old account; do not CC)]

Comment on attachment 198517 [details] [diff] [review]
Updated to dbaron's comments

This has been on the trunk for three months now and is considered low risk. While it's not a topcrasher, if there are any extensions that wish to change the style sheet in a weird way, they'll run into this.

Is there a good reason for *not* taking this on the branch?

Requesting approval for 1.8.0.1 and 1.8.0.2. If it's too late for 1.8.0.1 at least we can get it in the next security/stability release.

Comment 24

[Boris Zbarsky [:bzbarsky]]

If this goes into 1.8.0.x, it should probably go into 1.8.x as well.

Comment 25

[Samuel Sidler (old account; do not CC)]

Comment on attachment 198517 [details] [diff] [review]
Updated to dbaron's comments

Yes, forgot to request approval for 1.8.1.

Comment 26

[Daniel Veditz [:dveditz]]

Comment on attachment 198517 [details] [diff] [review]
Updated to dbaron's comments

a=dveditz for drivers

Comment 27

[Boris Zbarsky [:bzbarsky]]

Fixed on both branches.

Comment 28

[Marcia Knous [:marcia]]

verified fixed on the branch using Mozilla/5.0 (Macintosh; U; PPC Mac OS X Mach-O; en-US; rv:1.8.0.1) Gecko/20060111 Firefox/1.5.0.1. I followed the steps reproduce and do not see any crash, and the XML renders fine. 

Comment 29

[Tracy Walker [:tracy]]

verified with 2.0b2 builds from 20060821