Last Comment Bug 384871 - [1.8 branch] Crash [@ DocumentViewerImpl::Destroy] with popup as root element, setting autoPosition and reloading
: [1.8 branch] Crash [@ DocumentViewerImpl::Destroy] with popup as root element...
[sg:critical?] 1.8-branch
: crash, testcase, verified1.8.1.13
Product: Core
Classification: Components
Component: Layout (show other bugs)
: 1.8 Branch
: x86 Windows XP
-- critical (vote)
: ---
Assigned To: Neil Deakin
: Jet Villegas (:jet)
Depends on: 279703
  Show dependency treegraph
Reported: 2007-06-18 03:48 PDT by Martijn Wargers [:mwargers]
Modified: 2011-06-13 10:01 PDT (History)
14 users (show)
dveditz: blocking1.8.1.12-
dveditz: blocking1.8.1.13+
dveditz: wanted1.8.1.x+
dveditz: wanted1.8.0.x+
bob: in‑testsuite+
See Also:
Crash Signature:
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---

testcase (254 bytes, application/vnd.mozilla.xul+xml)
2007-06-18 03:48 PDT, Martijn Wargers [:mwargers]
no flags Details
check frame type (6.06 KB, patch)
2008-02-13 10:58 PST, Neil Deakin
roc: review+
roc: superreview+
dveditz: approval1.8.1.13+
Details | Diff | Splinter Review

Description User image Martijn Wargers [:mwargers] 2007-06-18 03:48:59 PDT
Created attachment 268774 [details]

See testcase, which crashes Mozilla after 300ms (because it then reloads).
0  	nsStyleCounterData::`vector deleting destructor'(unsigned int)
1 	nsStyleContent::~nsStyleContent()
2 	nsStyleContent::Destroy(nsPresContext *)
3 	nsResetStyleData::Destroy(unsigned int,nsPresContext *)
4 	nsCachedStyleData::Destroy(unsigned int,nsPresContext *)
5 	nsRuleNode::~nsRuleNode()
6 	nsRuleNode::Destroy()
7 	nsStyleSet::Shutdown(nsPresContext *)
8 	PresShell::Destroy()
9 	DocumentViewerImpl::Destroy()

It also crashes branch builds, so I'm marking it security sensitive for now.
I guess this might depend on bug 279703.
Comment 1 User image Boris Zbarsky [:bz] (still a bit busy) 2007-06-18 08:39:08 PDT
Yeah, we should retest once that lands, imo.
Comment 2 User image Martijn Wargers [:mwargers] 2007-07-04 12:56:54 PDT
This is worksforme now, with a tinderbox build just after the patch for bug 279703 landed.
Comment 3 User image Samuel Sidler (old account; do not CC) 2007-07-16 17:57:49 PDT
It's fine on trunk, but still crashes on branch. Not sure if the crash is s-s, but  requesting blocking anyway. (Should probably be wanted not blocking.)
Comment 4 User image Daniel Veditz [:dveditz] 2007-08-01 19:45:29 PDT
Crashes rv: (Firefox TB34576368M
and rv: (Firefox TB34576397K

Didn't crash in a debug build though so it was hard to see what's going on. I can't see bug 279703 landing on the 1.8 branches, would there be a simpler localized fix?
Comment 5 User image Daniel Veditz [:dveditz] 2007-08-21 15:40:54 PDT
dbaron, roc, bz: any nominations for who should look into a branch wallpaper patch for this? We're not going to re-design XUL popups on the old branch (bug 279703, 500K patch).
Comment 6 User image Robert O'Callahan (:roc) (email my personal email if necessary) 2007-08-21 16:28:54 PDT
Comment 7 User image Neil Deakin 2007-08-22 18:53:22 PDT
Is this because the root frame is a popup? We could just not allow that.

Comment 8 User image Boris Zbarsky [:bz] (still a bit busy) 2007-08-22 20:21:36 PDT
Root frame should never be a popup.  In this case it'll be a nsRootBoxFrame.
Comment 9 User image David Baron :dbaron: ⌚️UTC-8 2007-08-22 21:03:56 PDT
But do popups require the anonymous content created by the nsDocElementBoxFrame?  And does the document element not have an nsDocElementBoxFrame?
Comment 10 User image Boris Zbarsky [:bz] (still a bit busy) 2007-08-22 21:42:35 PDT
If I take out the reload, the full frame tree is:

Viewport(-1)@0x85e84bc [view=0x85bc9c8] {0,0,8540,5334} [state=00042010] [content=(nil)] [sc=0x866f980] pst=:-moz-viewport<
  Box(popup)(-1)@0x85e8550 {0,0,8540,5334} [state=81c40080] [content=0x85ff340] [sc=0x85e85c8] pst=:-moz-canvas<
    DocElementBox(popup)(-1)@0x85e8814 {0,0,8540,5334} [state=81a40080] [content=0x85ff340] [sc=0x85e87b8]<
      PopupSet(popupgroup)(-1)@0x85e8914 next=0x85e8c5c {42,42,8456,0} [state=80c000a0] [content=0x85bd978] [sc=0x85e86d4]<>
      Placeholder(tooltip)(-1)@0x85e8c5c {42,42,8456,0} outOfFlowFrame=MenuPopup(tooltip)(-1)@0x85e8b9c
      Box(arrowscrollbox)(-1)@0x85e8f20 {42,42,8456,5250} [state=808400a0] [content=0x85bb2a8] [sc=0x85e8a00]<
        Box(autorepeatbutton)(-1)@0x85e91b0 next=0x86756f0 {14,14,8414,0} [state=804000a0] [content=0x867d170] [sc=0x85e914c]<
          ImageBox(image)(-1)@0x85e934c {0,0,0,0} [state=000000a0] [content=0x867b7c8]
        XULScroll(scrollbox)(-1)@0x86756f0 [view=0x867c568] next=0x8675c68 {0,0,8456,5250} [state=80842090] [content=0x867dc00] [sc=0x85e903c]<
          Box(scrollbox)(-1)@0x8675684 [view=0x867c6f8] {0,0,8456,5250} [state=808020b8] [content=0x867dc00] [sc=0x867580c] pst=:-moz-scrolled-content<
            Box(box)(-1)@0x867599c {0,0,8456,0} [state=808000a0] [content=0x867a450] [sc=0x8675940]<>
        Box(autorepeatbutton)(-1)@0x8675c68 {14,5264,8414,0} [state=804000a0] [content=0x867d518] [sc=0x8675c04]<
          ImageBox(image)(-1)@0x8675d7c {0,0,0,0} [state=000000a0] [content=0x867c338]
Comment 11 User image Boris Zbarsky [:bz] (still a bit busy) 2007-08-22 21:47:32 PDT
So we're crashing because:

(gdb) frame
#7  0xb6276980 in nsCachedStyleData::Destroy (this=0x87c9bb0, aBits=0, 
    aContext=0x8642ff0) at ../../../mozilla/layout/style/nsRuleNode.h:231
231           mResetData->Destroy(aBits, aContext);
(gdb) p *mResetData
$2 = {mBackgroundData = 0x0, mPositionData = 0x10000, mTextResetData = 0x0, 
  mDisplayData = 0x87c9ce0, mContentData = 0x0, mUIResetData = 0x858b584, 
  mTableData = 0x0, mMarginData = 0x0, mPaddingData = 0x0, mBorderData = 0x0, 
  mOutlineData = 0x0, mXULData = 0x0, mSVGResetData = 0x0, mColumnData = 0x0}

Note the mPositionData.  That's not a good value to be having.
Comment 12 User image Daniel Veditz [:dveditz] 2007-09-26 13:49:59 PDT
Turning this into a branch-only bug since the trunk was fixed by a trunk-only redesign in another bug. Still need branch wallpaper.
Comment 13 User image Daniel Veditz [:dveditz] 2007-10-03 17:25:31 PDT
still no patch, we need to go with the fixes we have so people will unfortunately suffer from this one for a while longer -> next security release.
Comment 14 User image Martijn Wargers [:mwargers] 2007-10-03 17:59:31 PDT
Just 78 crashes were found with Talkback Search where the Stack Signature contains 'documentviewerimpl::destroy', so I don't think there are many people suffering from this.
Comment 15 User image Daniel Veditz [:dveditz] 2007-10-03 22:19:13 PDT
suffer in the sense of "remain vulnerable". I wouldn't expect to see a lot of crashes like this unless it's being actively exploited. Web sites which accidentally trigger this are likely to develop workarounds by trial and error when their customers complain.
Comment 16 User image Samuel Sidler (old account; do not CC) 2008-01-18 15:00:16 PST
Neil, are you going to have time to work on a patch for this bug? It's blocking this next branch release.
Comment 17 User image Daniel Veditz [:dveditz] 2008-01-28 11:17:34 PST
No work, no choice but to slip this into the next branch release.
Comment 18 User image Neil Deakin 2008-02-13 10:58:06 PST
Created attachment 303075 [details] [diff] [review]
check frame type

Seems that the methods in nsPopupBoxObject.cpp static cast the result of GetPrimaryFrame to a menupopupframe without checking what type of frame it really is, in this case a DocElementBoxFrame.
Comment 19 User image Daniel Veditz [:dveditz] 2008-02-15 11:22:53 PST
Comment on attachment 303075 [details] [diff] [review]
check frame type

approved for, a=dveditz for release-drivers
Comment 20 User image Martijn Wargers [:mwargers] 2008-03-12 02:34:05 PDT
Verified fixed, using:
Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv: Gecko/20080311 BonEcho/
While it did crash using a branch build of 2008-02-02.
Comment 21 User image Bob Clary [:bc:] 2009-04-24 10:41:45 PDT
crash test landed

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