271945 - crash when an image located within an anchor has a style of "display:-moz-popup" - M17x [@ nsMenuPopupFrame::RelayoutDirtyChild]

Status: RESOLVED FIXED

(Core :: XUL, defect)

Description

[Ryan Jackson]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.7.5) Gecko/20041107 Firefox/1.0
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.7.5) Gecko/20041107 Firefox/1.0

The web site http://80.4.61.62/moz-crash.html has an image embedded in an anchor
with the style property : display:-moz-popup. According to the web site when
viewed in IE, it says that it's due to a overflow buffer problem.

Reproducible: Always
Steps to Reproduce:
1. Visit http://80.4.61.62/moz-crash.html
Actual Results:  
1. Firefox crashes.

Expected Results:  
2. Firefox should not crash.

Comment 1

[Daniel Veditz [:dveditz] (OOO: back Aug 26)]

Attachment: copy of given testcase

Copy of testcase from website in case it goes away. I've changed the img src
from a relative url to one that will resolve on www.mozilla.org. I didn't see
any problems when the image didn't resolve.

I don't see a crash, I get a sort of hang instead. The menus will drop down,
cursor will change shape as I mouseover things in both chrome and content, but
it doesn't respond to clicks anywhere.

Comment 2

[Daniel Veditz [:dveditz] (OOO: back Aug 26)]

I do see a crash in Mozilla 1.7.2 (actually Netscape 7.2) down in layout. Will
try to get a talkback trace for Mozilla proper.

Comment 3

[Ryan Jackson]

Strange, the site no longer causes Firefox to crash on me. It now causes Firefox
to hang on me, when the browser is closed Firefox will remain as a proccess in
the background (then if you go to start up Firefox again it will ask which
profile you want and say that your default profile is locked).


(In reply to comment #1)
> Created an attachment (id=167326)
> copy of given testcase
> 
> Copy of testcase from website in case it goes away. I've changed the img src
> from a relative url to one that will resolve on www.mozilla.org. I didn't see
> any problems when the image didn't resolve.
> 
> I don't see a crash, I get a sort of hang instead. The menus will drop down,
> cursor will change shape as I mouseover things in both chrome and content, but
> it doesn't respond to clicks anywhere.

Comment 4

[Daniel Veditz [:dveditz] (OOO: back Aug 26)]

With 1.8a5 the crash I get is
http://talkback-public.mozilla.org/talkback/fastfind.jsp?search=2&type=iid&id=2241191

Access violation
nsMenuPopupFrame::RelayoutDirtyChild 
[c:/builds/tinderbox/MozillaTrunk/WINNT_5.0_Clobber/mozilla/layout/xul/base/src/nsMenuPopupFrame.cpp,
line 350]
nsIFrame::MarkDirtyChildren 
[c:/builds/tinderbox/MozillaTrunk/WINNT_5.0_Clobber/mozilla/layout/xul/base/src/nsBox.cpp,
line 310]
nsBoxFrame::RemoveFrame 
[c:/builds/tinderbox/MozillaTrunk/WINNT_5.0_Clobber/mozilla/layout/xul/base/src/nsBoxFrame.cpp,
line 1173]
nsContainerFrame::ReplaceFrame 
[c:/builds/tinderbox/MozillaTrunk/WINNT_5.0_Clobber/mozilla/layout/html/base/src/nsContainerFrame.cpp,
line 410]
nsCSSFrameConstructor::CantRenderReplacedElement 
[c:/builds/tinderbox/MozillaTrunk/WINNT_5.0_Clobber/mozilla/layout/html/style/src/nsCSSFrameConstructor.cpp,
line 10576]
nsFrameManager::HandlePLEvent 
[c:/builds/tinderbox/MozillaTrunk/WINNT_5.0_Clobber/mozilla/layout/html/base/src/nsFrameManager.cpp,
line 888]
PL_HandleEvent 
[c:/builds/tinderbox/MozillaTrunk/WINNT_5.0_Clobber/mozilla/xpcom/threads/plevent.c,
line 693]
_md_EventReceiverProc 
[c:/builds/tinderbox/MozillaTrunk/WINNT_5.0_Clobber/mozilla/xpcom/threads/plevent.c,
line 1434]
0x778b0c24
0x006e006f

I don't see an overflowed buffer, but it does appear to be operating on garbage
pointers. Maybe a lifetime issue from deCOMtamination. This routine (though not
this stack) is roughly top crash #20 on the Mozilla 1.7 branch, doesn't show up
in the Firefox 1.0 talkbacks at all.

Clearing security sensitive flag to get more eyeballs here. If someone figures
out a way to actually exploit this then we can hide it again later.

Comment 5

[Matthew Bonner]

It does not matter where the image points to and the outcome is unpredictable to
what Firefox does.

I tried using other words with similar patterns and this is the only word I
woulc find that caused this problem. This word should be ignored by Firefox
altogether since the DTD does not define the use of this attribute.

Comment 6

[Daniel Veditz [:dveditz] (OOO: back Aug 26)]

Which DTD? -moz-popup is supported as an extension of CSS. Attributes with the
"-moz" prefix are for internal or experimental use (for example, we had rounded
border attributes, that were replaced with the standard attribute names when
adopted by the working group). Web authors are advised not to use them as they
can and will change, plus they make web content specific to a single browser
family which we advocate against. But they will not be ignored (and of course
they shouldn't crash).

Comment 7

[Matthew Bonner]

Sorry, my error since there is no DTD for CSS, I must have been confused and
thinking about attributes when I added that comment.

When I said ignored, I meant the browser should not parse the word if it does
not know how to handle the word correctly and not that this bug should be ignored.

Comment 8

[[e-mail]]

Basically the popup frame (which is what display: -moz-popup; creates) assumes
its in a menu or a popupset... if the CSS frame constructor can't enforce this
then we'll have to sprinkle null checks throughout the code.

Comment 9

[Boris Zbarsky [:bzbarsky]]

The CSS frame constructor _could_ enforce it, in theory.  The problem is that
the frame constructor has no way of knowing silly implementation details of all
the XUL frames, especially since these have a way of changing.  Maybe we should
have an nsIFrame api to deal with this?

Apart from that, someone needs to decide exactly how XUL frame construction
_should_ work and specify it.  Then we should implement it.  I'm sorta tired of
hacking it a little bit at a time in haphazard ways.

Comment 10

[[e-mail]]

Attachment: First crack at a patch

bz, like this?

Comment 11

[Boris Zbarsky [:bzbarsky]]

Yeah, somewhat like this.  With an XXX comment that the null-check is there
because the assert can be triggered and that frame construction should probably
be fixed...

Comment 12

[[e-mail]]

Attachment: Don't use GetParentBox

I wasn't sure why these three places use GetParentBox; nowhere else does.

Comment 13

[Jay Patel [:jay]]

Adding M17x to summary and topcrash keyword for tracking, this has been showing
up in Talkback for the past few Mozilla 1.7.x releaes.

Comment 14

[Boris Zbarsky [:bzbarsky]]

Neil, is that patch ready to go, then?

Comment 15

[[e-mail]]

I can't honestly remember. I'm sure that my build doesn't crash though.

Comment 16

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 167540 [details] [diff] [review]
Don't use GetParentBox

r+sr=bzbarsky, though it looks like some of this code could be refactored a
tad... but that can happen later.

Comment 17

[[e-mail]]

Fix checked in.

Comment 18

[Boris Zbarsky [:bzbarsky]]

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