312784 - crash setting display:none on grid rows [@ nsGrid::GetPrefRowHeight]

Status: VERIFIED FIXED

(Core :: XUL, defect, P1)

Description

[François Gagné]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8b5) Gecko/20051017 Firefox/1.4.1
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8b5) Gecko/20051017 Firefox/1.4.1

Firefox crash on about:config when changing a giving boolean with gmail notifier
0.5.2.1 (TB 10782990)

Reproducible: Always

Steps to Reproduce:
1.Install Gmail Notifier 0.5.2.1
(http://ftp.mozilla.org/pub/mozilla.org/extensions/gmail_notifier/gmail_notifier-0.5.2.1-fx+mz.xpi)
2.Restart Firefox to finish the installation
3.Log in your gmail account (using the icon on the status bar)
4.Put the mouse over the icon to get the tooltip
5.Open a new tab with about:config
6.Put 'gm' in the filter
7.Quick double click a couple of times on 'gm-notifier.ui.folderview' to switch
it from true to false, to false to true, it should crash after 2-3 times

Comment 1

[Adam Guthrie]

Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9a1) Gecko/20051016
Firefox/1.6a1 ID:2005101619

TB10793385Q - I could only reproduce this once, but it appears that your stack
and my stack are different. 

Comment 2

[Ria Klaassen (not reading all bugmail)]

Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8b5) Gecko/20051017
Firefox/1.4.1 ID:2005101723

It crashes but I see no talkback, only MS. Tried it on two computers with the
same result.
Once I have clicked on gm-notifier.ui.folderview, whether or not it crashes, the
process firefox.exe stays in the taskmanager after closing Firefox.

Comment 3

[François Gagné]

reply to comment #1, probably because it's done on different branch (I tested
the 1.8 branch and you tested the trunk)

Comment 4

[Scott MacGregor]

I only see a single talkback report with this crash in talkback. If this were a
top crasher we'd consider it but this late in the game we aren't holding up the
release for non criticial top crashes. 

Comment 5

[Jesse Ruderman]

TB10782990 looks like nonsense skidmarks to me: it doesn't make sense that SVG
code would be called, and one of the functions on the stack is a leaf function.
 So it's not surprising that mscott can't find other crashes with the same
signature.  That happens sometimes with some types of crashes; I don't know the
details about why it happens.

TB10793385Q looks more likely to have something to do with this crash. 
TB10793385Q has a random address at the top, followed by 
nsGrid::GetPrefRowHeight.  This is more plausible -- about:config does use a
listbox and I've seen many listbox-related crashes in nsGrid functions.  

Unfortunately, the random-address-at-the-top makes it hard to determine the
number of matching crashes (because of bug 304768 / bug 304769) to tell whether
this is a topcrash.

This crash might be exploitable by web content to execute arbitrary code.  The
crash seems to have more to do with <listbox>, which can be used by web content,
than the chrome parts of about:config.

If this crash is a regression, a regression range would be useful.

Comment 6

[François Gagné]

Nope it's not a regression it crash way back to 1.0 (I must have a gift to fall
on bug like this)

Error message given when crashing with Firefox 1.0 and 1.0.7 (no talkback)

Microsoft Visual C++ Runtion Library

Program: C:\PROGRA~1\MOZILL~1\firefox.exe

Firefox 1.0.7

R6025
- pure virtual call

Comment 7

[François Gagné]

Tested Mozilla Suite 1.6, 1.7 and they crash.

Comment 8

[François Gagné]

sry for bugspam:

Microsoft not so usefull explanation of R6025
http://msdn.microsoft.com/library/default.asp?url=/library/en-us/vccore/html/R6025.asp

I read a couple of forum post of people having the R6025 problem (found by a
search on google) and one on them was saying that running a vc6 debug build was
suppose to assert instead of crashing. Worth a try on 1.0.7 ?

Comment 9

[Asa Dotzler [:asa]]

Any chance this was fixed over at bug 311710 ? If not, BZ, can you look into this? 

Comment 10

[Jesse Ruderman]

The fix for bug 311710 was checked in on Oct 16, and this bug was reported with
Oct 17 builds (but is not a recent regression).  So the fix for bug 311710
didn't fix (or cause) this bug.

Comment 11

[Boris Zbarsky [:bzbarsky]]

So what's going on here is that the whole grid row group is getting
display:none.  We simply have no provisions for this -- we notify the parent's
layout manager that it went away, but that's nsBoxLayout and does nothing with
the notification.  So now we have dangling pointers to deleted row frames in
nsGrid, and when we try to mess with them we crash.

I don't really understand what this code is doing, so I have no idea how to fix
it, much less fix safely.  :(

Comment 12

[[e-mail]]

(In reply to comment #5)
>about:config does use a listbox
It does? That's news to me...

(In reply to comment #11)
>So what's going on here is that the whole grid row group is getting display:none.
Um... which grid row group?

>I don't really understand what this code is doing
Join the crew ;-)

Comment 13

[Boris Zbarsky [:bzbarsky]]

> Um... which grid row group?

There's a grid row group in the overlay which this extension installs.  Then it
has a pref observer for the pref we're toggling which does, when notified of a
pref change (gm-notifier.js):

    case gGMailNotifier.wm_prefs.PREF_USE_FOLDERVIEW:
      var showFolders =
gGMailNotifier.wm_prefs.getBoolPref(gGMailNotifier.wm_prefs.PREF_USE_FOLDERVIEW);
      document.getElementById("gm-tooltip-row").hidden = !showFolders;

where the thing with gm-tooltip-row as the ID is found in gm-core-overlay.xul;
the relevant part looks like:

        <grid id="gm-notifier-tooltip-labels" hidden="true"
style="font-size:0.8em; padding:5px;">
          <columns>
            <column/>
            <column/>
          </columns>
          <rows id="gm-tooltip-row">
          </rows>
        </grid>

I'll attach a minimal testcase that doesn't need this extension at all.

Comment 14

[Boris Zbarsky [:bzbarsky]]

Attachment: Testcase

This has a slightly different stack, but I think it's the same issue.

Comment 15

[Boris Zbarsky [:bzbarsky]]

Attachment: Actual testcase

Comment 16

[Asa Dotzler [:asa]]

not a topcrasher and it doesn't look like anyone's suggested it's exploitable
and we're already shipping this bug so not going to block on it.

Comment 17

[Jesse Ruderman]

neil and bz, sorry for the incorrect comment.  about:config is a tree, not a
listbox.

Asa, I think this is exploitable and we can't easily tell whether this is a
topcrash.  See comment 5.

Comment 18

[Boris Zbarsky [:bzbarsky]]

> and it doesn't look like anyone's suggested it's exploitable

Dangling pointers (see comment 11) mean this is most likely exploitable.

Comment 19

[Boris Zbarsky [:bzbarsky]]

Fixed by checkin for bug 313173

Comment 20

[Jesse Ruderman]

WFM with the Mac build from
http://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/2005-10-21-15-trunk/.  I
was able to reproduce this crash in a build from yesterday (before the patch
went in).  I'm not sure I should mark this bug as verified, because François
originally reported this bug on Windows.

Comment 21

[Jesse Ruderman]

Assuming bug 313173 comment 17, "Fixed on branch.", applies to this bug too.

Comment 22

[Boris Zbarsky [:bzbarsky]]

Er... yes, it does.  Thanks, Jesse!

Comment 23

[Bob Clary [:bc] (inactive)]

no crash firefox 1.5 rc2 winxp/linux

Comment 24

[Boris Zbarsky [:bzbarsky]]

Fixed on branches by checkin of bug 313173

Comment 25

[Jay Patel [:jay]]

v.fixed on 1.0.8 Aviary with Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.7.12) Gecko/20060209 Firefox/1.0.7

Comment 26

[Bob Clary [:bc] (inactive)]

crash test landed
http://hg.mozilla.org/mozilla-central/rev/2057466b414a