471493 - crash [@ nsPropertyTable::GetPropertyInternal(nsPropertyOwner, unsigned short, nsIAtom*, int, unsigned int*)]

Status: VERIFIED FIXED

(Core :: Disability Access APIs, defect)

Description

[Wayne Mery (:wsmwk)]

crash [@ nsPropertyTable::GetPropertyInternal]

#1 crasher by far for 3.0b1 and 3.0b2pre, but it might be only a couple people.
uptime range is a few seconds to a few hours.

bp-1570f8df-8f56-407b-ac6c-298e72081226
nsPropertyTable::GetPropertyInternal	content/base/src/nsPropertyTable.cpp:190
nsPropertyTable::GetProperty	content/base/src/nsPropertyTable.h:113
nsIFrame::GetProperty	layout/generic/nsFrame.cpp:6016
nsIFrame::GetView	layout/generic/nsFrame.cpp:3461
nsIFrame::GetClosestView	layout/generic/nsFrame.cpp:5499
nsIFrame::GetScreenRectInAppUnits	layout/generic/nsFrame.cpp:3568
nsIFrame::GetScreenRect	layout/generic/nsFrame.cpp:3553
nsIFrame::GetScreenRectExternal	layout/generic/nsFrame.cpp:3548
nsXULTreeAccessible::GetChildAtPoint	accessible/src/xul/nsXULTreeAccessible.cpp:336
nsXULTreeAccessible::GetDeepestChildAtPoint	accessible/src/xul/nsXULTreeAccessible.cpp:363
nsAccessible::GetChildAtPoint	accessible/src/base/nsAccessible.cpp:1154
nsXULTreeAccessible::GetChildAtPoint	accessible/src/xul/nsXULTreeAccessible.cpp:352
nsXULTreeAccessible::GetDeepestChildAtPoint	accessible/src/xul/nsXULTreeAccessible.cpp:363
nsAccessible::GetChildAtPoint	accessible/src/base/nsAccessible.cpp:1154

Comment 1

[Phil Ringnalda (:philor)]

or bp-499f3919-79e7-4aea-8524-5d2fc2081226 if you prefer your nsXULTreeAccessible::GetChildAtPoint -> nsPropertyTable::GetPropertyInternal crashes in Firefox rather than Thunderbird.

Comment 2

[Marco Zehe (:MarcoZ)]

Alex, can you take a look? You reimplemented this function IIRC in the Gecko 1.9.1 timeframe.

Comment 3

[Ginn Chen]

ah, stack overflow

Comment 4

[alexander :surkov (:asurkov)]

Should be bug 455442.

Comment 5

[alexander :surkov (:asurkov)]

Attachment: patch

Comment 6

[alexander :surkov (:asurkov)]

Comment on attachment 365143 [details] [diff] [review]
patch

asking review from Rob for crashtests part

Comment 7

[Marco Zehe (:MarcoZ)]

Comment on attachment 365143 [details] [diff] [review]
patch

r=me for the tests part. Thanks!

Comment 8

[David Bolter [:davidb] (NeedInfo me for attention)]

Comment on attachment 365143 [details] [diff] [review]
patch

This looks like it gets rid of the mutual recursion (stack overflow) thanks!

(Will be nice to have layout.js for the bug I'm working on)

>+++ b/accessible/tests/mochitest/layout.js
>@@ -0,0 +1,55 @@
>+/**
>+ * Tests if the given accessible at the given point is expected.
>+ *
>+ * @param aIdentifier        [in] accessible identifier
>+ * @param aX                 [in] x coordinate of the point relative accessible
>+ * @param aY                 [in] y coordinate of the point relative accessible
>+ * @param aTakeDeepestChild  [in] points whether deepest or nearest child should

Nit: I would prefer "findDeepestChild" instead of "aTakeDeepestChild" (find and replace).

r=me

Comment 9

[Rob Campbell [:rc] (:robcee)]

Comment on attachment 365143 [details] [diff] [review]
patch

this looks ok to me.

Comment 10

[alexander :surkov (:asurkov)]

http://hg.mozilla.org/mozilla-central/rev/acdb66fda343

Comment 11

[Mike Beltzner [:beltzner, not reading bugmail]]

Comment on attachment 365143 [details] [diff] [review]
patch

a191=beltzner

Comment 12

[Dão Gottwald [:dao]]

Alexander, are you going to land this on branch?

Comment 13

[alexander :surkov (:asurkov)]

(In reply to comment #12)
> Alexander, are you going to land this on branch?

Thank for reminder. Checked in http://hg.mozilla.org/releases/mozilla-1.9.1/rev/65ea3b70fd00

Comment 14

[Marco Zehe (:MarcoZ)]

Verified fixed in Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.1b4pre) Gecko/20090422 Shiretoko/3.5b4pre (.NET CLR 3.5.30729)

Comment 15

[Wayne Mery (:wsmwk)]

v.fixed ... no such crashes in 3.0b3 and recent 3.0b3pre

nsPropertyTable::GetPropertyInternal(nsPropertyOwner, unsigned short, nsIAtom*, int, unsigned int*)

Comment 16

[Boris Zbarsky [:bzbarsky]]

This isn't in-testsuite+: the crashtest.list is not added to the global crashtest manifest.

Which is a good thing, since this crashtest uses enablePrivilege and hence would just hang the test machines.

Comment 17

[alexander :surkov (:asurkov)]

(In reply to comment #16)
> This isn't in-testsuite+: the crashtest.list is not added to the global
> crashtest manifest.
> 
> Which is a good thing, since this crashtest uses enablePrivilege and hence
> would just hang the test machines.

Boris, what should I change to avoid hangs?

Comment 19

[Daniel Holbert [:dholbert]]

(In reply to comment #17)
> > Which is a good thing, since this crashtest uses enablePrivilege and hence
> > would just hang the test machines.
> 
> Boris, what should I change to avoid hangs?

I'd suggest converting the crashtest into a mochitest, since those *can* use enablePrivilege.