[1.8 branch] Crash [@ DocumentViewerImpl::Destroy] with popup as root element, setting autoPosition and reloading




10 years ago
6 years ago


(Reporter: Martijn Wargers (dead), Assigned: Neil Deakin (mostly unavailable until September))


({crash, testcase, verified1.8.1.13})

1.8 Branch
Windows XP
crash, testcase, verified1.8.1.13
Bug Flags:
blocking1.8.1.12 -
blocking1.8.1.13 +
wanted1.8.1.x +
blocking1.8.0.next +
wanted1.8.0.x +
in-testsuite +

Firefox Tracking Flags

(Not tracked)


(Whiteboard: [sg:critical?] 1.8-branch, crash signature)


(2 attachments)



10 years ago
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.
Yeah, we should retest once that lands, imo.

Comment 2

10 years ago
This is worksforme now, with a tinderbox build just after the patch for bug 279703 landed.
Last Resolved: 10 years ago
Resolution: --- → FIXED
Flags: in-testsuite?
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.)
Flags: blocking1.8.1.6?
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?
Flags: wanted1.8.1.x+
Flags: wanted1.8.0.x+
Whiteboard: [sg:critical?]
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).
Flags: blocking1.8.1.7? → blocking1.8.1.7+
Is this because the root frame is a popup? We could just not allow that.

Root frame should never be a popup.  In this case it'll be a nsRootBoxFrame.
But do popups require the anonymous content created by the nsDocElementBoxFrame?  And does the document element not have an nsDocElementBoxFrame?
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]
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.
Resolution: FIXED → ---
Turning this into a branch-only bug since the trunk was fixed by a trunk-only redesign in another bug. Still need branch wallpaper.
Assignee: nobody → enndeakin
Version: Trunk → 1.8 Branch
Whiteboard: [sg:critical?] → [sg:critical?] 1.8-branch
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.
Flags: blocking1.8.1.8+ → blocking1.8.1.9+

Comment 14

10 years ago
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.
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.


10 years ago
Summary: Crash [@ DocumentViewerImpl::Destroy] with popup as root element, setting autoPosition and reloading → [1.8 branch] Crash [@ DocumentViewerImpl::Destroy] with popup as root element, setting autoPosition and reloading
Neil, are you going to have time to work on a patch for this bug? It's blocking this next branch release.
Whiteboard: [sg:critical?] 1.8-branch → [sg:critical?] 1.8-branch [needs patch]
Flags: blocking1.8.1.13+
Flags: blocking1.8.1.12-
Flags: blocking1.8.1.12+
No work, no choice but to slip this into the next branch release.
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.
Attachment #303075 - Flags: superreview?(roc)
Attachment #303075 - Flags: review?(roc)
Whiteboard: [sg:critical?] 1.8-branch [needs patch] → [sg:critical?] 1.8-branch [needs r/sr=roc]
Attachment #303075 - Flags: superreview?(roc)
Attachment #303075 - Flags: superreview+
Attachment #303075 - Flags: review?(roc)
Attachment #303075 - Flags: review+
Whiteboard: [sg:critical?] 1.8-branch [needs r/sr=roc] → [sg:critical?] 1.8-branch
Attachment #303075 - Flags: approval1.8.1.13?
Comment on attachment 303075 [details] [diff] [review]
check frame type

approved for, a=dveditz for release-drivers
Attachment #303075 - Flags: approval1.8.1.13? → approval1.8.1.13+
Last Resolved: 10 years ago10 years ago
Keywords: fixed1.8.1.13
Resolution: --- → FIXED

Comment 20

10 years ago
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.
Keywords: fixed1.8.1.13 → verified1.8.1.13


10 years ago
Flags: blocking1.8.0.15+


10 years ago
Attachment #303075 - Flags: approval1.8.0.15?
Group: security

Comment 21

8 years ago
crash test landed
Flags: in-testsuite? → in-testsuite+
Crash Signature: [@ DocumentViewerImpl::Destroy]
You need to log in before you can comment on or make changes to this bug.