The default bug view has changed. See this FAQ.

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

VERIFIED FIXED

Status

()

Core
Layout
--
critical
VERIFIED FIXED
10 years ago
6 years ago

People

(Reporter: Martijn Wargers (dead), Assigned: Neil Deakin)

Tracking

({crash, testcase, verified1.8.1.13})

1.8 Branch
x86
Windows XP
crash, testcase, verified1.8.1.13
Points:
---
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)

Details

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

Attachments

(2 attachments)

(Reporter)

Description

10 years ago
Created attachment 268774 [details]
testcase

See testcase, which crashes Mozilla after 300ms (because it then reloads).

https://crash-reports.mozilla.com/reports/report/index/9e02c635-1cd1-11dc-aee0-001a4bd43ed6
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.
(Reporter)

Comment 2

10 years ago
This is worksforme now, with a tinderbox build just after the patch for bug 279703 landed.
Status: NEW → RESOLVED
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:1.8.0.12 (Firefox 1.5.0.12): TB34576368M
and rv:1.8.1.6 (Firefox 2.0.0.6): TB34576397K

Didn't crash in a 1.8.1.5 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+
enndeakin?
(Assignee)

Comment 7

10 years ago
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.
Status: RESOLVED → REOPENED
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
Status: REOPENED → NEW
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+
(Reporter)

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.
(Reporter)

Updated

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.
(Assignee)

Comment 18

9 years ago
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
(Assignee)

Updated

9 years ago
Attachment #303075 - Flags: approval1.8.1.13?
Comment on attachment 303075 [details] [diff] [review]
check frame type

approved for 1.8.1.13, a=dveditz for release-drivers
Attachment #303075 - Flags: approval1.8.1.13? → approval1.8.1.13+
(Assignee)

Updated

9 years ago
Status: NEW → RESOLVED
Last Resolved: 10 years ago9 years ago
Keywords: fixed1.8.1.13
Resolution: --- → FIXED
(Reporter)

Comment 20

9 years ago
Verified fixed, using:
Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8.1.13pre) Gecko/20080311 BonEcho/2.0.0.13pre
While it did crash using a branch build of 2008-02-02.
Status: RESOLVED → VERIFIED
Keywords: fixed1.8.1.13 → verified1.8.1.13

Updated

9 years ago
Flags: blocking1.8.0.15+

Updated

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

Comment 21

8 years ago
crash test landed
http://hg.mozilla.org/mozilla-central/rev/653797325daf
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.