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
Depends on: 279703
  Show dependency treegraph
Reported: 2007-06-18 03:48 PDT by Martijn Wargers [:mwargers] (gone per 2016-05-31 :-( )
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] (gone per 2016-05-31 :-( )
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 | Review

Description Martijn Wargers [:mwargers] (gone per 2016-05-31 :-( ) 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 Boris Zbarsky [:bz] 2007-06-18 08:39:08 PDT
Yeah, we should retest once that lands, imo.
Comment 2 Martijn Wargers [:mwargers] (gone per 2016-05-31 :-( ) 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 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 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 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 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2007-08-21 16:28:54 PDT
Comment 7 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 Boris Zbarsky [:bz] 2007-08-22 20:21:36 PDT
Root frame should never be a popup.  In this case it'll be a nsRootBoxFrame.
Comment 9 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 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 Boris Zbarsky [:bz] 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 Boris Zbarsky [:bz] 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 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 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 Martijn Wargers [:mwargers] (gone per 2016-05-31 :-( ) 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 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 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 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 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 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 Martijn Wargers [:mwargers] (gone per 2016-05-31 :-( ) 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 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.