757739 - crash in nsAccUtils::MustPrune

Status: VERIFIED FIXED

(Core :: Disability Access APIs, defect)

Description

[Scoobidiver (away)]

There are three crashes from two different users.

Signature 	nsAccUtils::MustPrune More Reports Search
UUID	1eef3c8e-0e2a-4296-9813-bd9952120522
Date Processed	2012-05-22 18:42:07
Uptime	2219
Last Crash	2.3 days before submission
Install Age	37.0 minutes since version was first installed.
Install Time	2012-05-22 18:04:53
Product	Firefox
Version	15.0a1
Build ID	20120522080220
Release Channel	nightly
OS	Mac OS X
OS Version	10.7.4 11E53
Build Architecture	amd64
Build Architecture Info	family 6 model 23 stepping 10
Crash Reason	EXC_BAD_ACCESS / KERN_INVALID_ADDRESS
Crash Address	0x70
App Notes 	
AdapterVendorID: 0x10de, AdapterDeviceID: 0x 8a0GL Context? GL Context+ GL Layers? GL Layers+ 
EMCheckCompatibility	True

Frame 	Module 	Signature 	Source
0 	XUL 	nsAccUtils::MustPrune 	Accessible-inl.h:16
1 	XUL 	nsAccessible::AppendTextTo 	nsAccessible.cpp:2486
2 	XUL 	NotificationController::CreateTextChangeEventFor 	NotificationController.cpp:667
3 	XUL 	NotificationController::QueueEvent 	NotificationController.cpp:117
4 	XUL 	nsDocAccessible::UpdateTreeInternal 	nsDocAccessible.cpp:1717
5 	XUL 	nsDocAccessible::UpdateTree 	nsDocAccessible.cpp:1810
6 	XUL 	nsCSSFrameConstructor::ContentRemoved 	nsCSSFrameConstructor.cpp:7390
7 	XUL 	nsCSSFrameConstructor::RecreateFramesForContent 	nsCSSFrameConstructor.cpp:9099
8 	XUL 	nsCSSFrameConstructor::ProcessRestyledFrames 	nsCSSFrameConstructor.cpp:7906
9 	XUL 	nsCSSFrameConstructor::RestyleElement 	nsCSSFrameConstructor.cpp:8052
10 	XUL 	mozilla::css::RestyleTracker::DoProcessRestyles 	RestyleTracker.cpp:124
11 	XUL 	nsCSSFrameConstructor::ProcessPendingRestyles 	RestyleTracker.h:68
12 	XUL 	PresShell::FlushPendingNotifications 	nsPresShell.cpp:3800
13 	XUL 	nsDocument::FlushPendingNotifications 	nsDocument.cpp:6336
14 	XUL 	nsComputedDOMStyle::GetPropertyCSSValue 	nsComputedDOMStyle.cpp:460
15 	XUL 	nsComputedDOMStyle::GetPropertyValue 	nsComputedDOMStyle.cpp:283
16 	XUL 	nsIDOMCSSStyleDeclaration_GetPropertyValue 	dom_quickstubs.cpp:9949 
...

More reports at:
https://crash-stats.mozilla.com/report/list?signature=nsAccUtils%3A%3AMustPrune

Comment 1

[Marco Zehe (:MarcoZ)]

Sounds like a regression from bug 754869, but this landed only very recently.

Comment 2

[alexander :surkov (:asurkov)]

since we process nsCSSFrameConstructor::ContentRemoved then we fire hide event (note, we do that before we unattach it from tree) so it must have a parent but it doesn't since MustPrune(Parent()) crashes. So if no other ideas then I'd suggest to null check Parent() and assert.

Comment 3

[Scoobidiver (away)]

I added another related signature. More reports at:
https://crash-stats.mozilla.com/report/list?signature=nsAccessible%3A%3ARole

Comment 4

[Scoobidiver (away)]

Here are Windows crashes: https://crash-stats.mozilla.com/report/list?signature=nsAccessible%3A%3ARole%28%29

Comment 5

[alexander :surkov (:asurkov)]

Attachment: patch

Comment 6

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

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

Review of attachment 626373 [details] [diff] [review]:
-----------------------------------------------------------------

r=me. I'd prefer no assert.

Comment 7

[Trevor Saunders (:tbsaunde)]

(In reply to David Bolter [:davidb] from comment #6)
> Comment on attachment 626373 [details] [diff] [review]
> patch
> 
> Review of attachment 626373 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> r=me. I'd prefer no assert.

why no assert? this really should never happen, ideally I'd rather not take this, if we can figure out what's going on here.  However the stacks really don't give me any ideas either, do we have any urls or ideas how to reproduce?   I suspect somehow we operate on bd tree, but I'm not sure how it got that wy off hand.  crash-stats.m.o is just giving me 500s so I'm not really sure what all we know.  I'm pretty sure I could cause the same crash if I removed nsAccessible::GetIndexOf() and replaced it with a direct call to IndexInParent() which seems like something we should do.

Comment 8

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

I should have explicitly added to file a follow up bug. I assumed we wouldn't be happy just null checking.

Comment 9

[Robert Kaiser]

(In reply to Trevor Saunders (:tbsaunde) from comment #7)
> crash-stats.m.o is just giving me 500s so I'm not
> really sure what all we know.

Should be temporary, work is being done on that. Try those for now:
https://crash-stats.mozilla.com/report/list?product=Firefox&signature=nsAccUtils%3A%3AMustPrune
https://crash-stats.mozilla.com/report/list?product=Firefox&signature=nsAccessible%3A%3ARole
https://crash-stats.mozilla.com/report/list?product=Firefox&signature=nsAccessible%3A%3ARole()

Comment 10

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

When is nsCSSFrameConstructor::ContentRemoved called with no aContainer?

Comment 11

[alexander :surkov (:asurkov)]

(In reply to David Bolter [:davidb] from comment #10)
> When is nsCSSFrameConstructor::ContentRemoved called with no aContainer?

on document iirc

Comment 12

[alexander :surkov (:asurkov)]

(In reply to Trevor Saunders (:tbsaunde) from comment #7)
> I'm pretty sure I could cause the same crash
> if I removed nsAccessible::GetIndexOf() and replaced it with a direct call
> to IndexInParent() which seems like something we should do.

can you elaborate that?

Comment 13

[alexander :surkov (:asurkov)]

(In reply to David Bolter [:davidb] from comment #8)
> I should have explicitly added to file a follow up bug. I assumed we
> wouldn't be happy just null checking.

We aren't happy and if somebody have options/ideas then follow up bug makes sense if we don't put a fix right here. Otherwise I don't see a reason to have a follow up.

Comment 14

[Trevor Saunders (:tbsaunde)]

(In reply to alexander :surkov from comment #12)
> (In reply to Trevor Saunders (:tbsaunde) from comment #7)
> > I'm pretty sure I could cause the same crash
> > if I removed nsAccessible::GetIndexOf() and replaced it with a direct call
> > to IndexInParent() which seems like something we should do.
> 
> can you elaborate that?

Well, I'm not actually sure what I was thinking... so, in NotificationController::CreateTextChangeEvent() we call ChildOffset() of the text accessible on the hyper text accessible.  which starts by calling GetIndexOf(textAccessible) on the hyper text accessible.  which should be changed to just textAccessible->IndexInParent(), but I don't see why tht would crash now just return wrong result.

Comment 15

[Trevor Saunders (:tbsaunde)]

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

I'm sort of tempted to try adding

if (textAcc->Parent() != hyperTextAcc)
  return

in CreateTextChangeEvent(), but I'm not having any luck trying to come up with an explanaition.  so r=me I guess :(

Comment 16

[alexander :surkov (:asurkov)]

(In reply to Trevor Saunders (:tbsaunde) from comment #15)

> if (textAcc->Parent() != hyperTextAcc)
>   return
> 
> in CreateTextChangeEvent()

if you meant
if (aEvent->mAccessible->Parent() != textAccessible)

then yes it helps since Parent() is null but in this case we don't hit the assertion I added. Did you mean something else?

Comment 17

[Trevor Saunders (:tbsaunde)]

(In reply to alexander :surkov from comment #16)
> (In reply to Trevor Saunders (:tbsaunde) from comment #15)
> 
> > if (textAcc->Parent() != hyperTextAcc)
> >   return
> > 
> > in CreateTextChangeEvent()
> 
> if you meant
> if (aEvent->mAccessible->Parent() != textAccessible)
> 
> then yes it helps since Parent() is null but in this case we don't hit the
> assertion I added. Did you mean something else?

I think what I meant exactly was

if (aEvent->mAccessible->Parent() != hyperTextAcc)
  return;

with an assert too.  I mostly wanted to be more targeted in when we add computations we shouldn't need.

Comment 18

[alexander :surkov (:asurkov)]

ok, that makes sense

Comment 19

[alexander :surkov (:asurkov)]

(In reply to alexander :surkov from comment #18)
> ok, that makes sense

on the second glace, that check inside CreateTextChangeEvent makes us to not fire an event. I wouldn't do that since I don't understand what's going on here. Perhaps it's worth to go with original patch as is. Trevor?

Comment 20

[Scoobidiver (away)]

It's #8 top crasher over the last 3 days.

Comment 21

[Trevor Saunders (:tbsaunde)]

(In reply to alexander :surkov from comment #19)
> (In reply to alexander :surkov from comment #18)
> > ok, that makes sense
> 
> on the second glace, that check inside CreateTextChangeEvent makes us to not
> fire an event. I wouldn't do that since I don't understand what's going on
> here. Perhaps it's worth to go with original patch as is. Trevor?

sure, I'll land this with the Accessible stuff in a few minutes when try finishes getting green.

However I noticed one of the crash reports mentioned arte.tv where I can reliably reproduce the crash on linux 64 both non optamized and with debugging on.

Comment 22

[alexander :surkov (:asurkov)]

(In reply to Trevor Saunders (:tbsaunde) from comment #21)

> However I noticed one of the crash reports mentioned arte.tv where I can
> reliably reproduce the crash on linux 64 both non optamized and with
> debugging on.

cool, we can fix it then. I'll try it out.

Comment 23

[Trevor Saunders (:tbsaunde)]

(In reply to alexander :surkov from comment #22)
> (In reply to Trevor Saunders (:tbsaunde) from comment #21)
> 
> > However I noticed one of the crash reports mentioned arte.tv where I can
> > reliably reproduce the crash on linux 64 both non optamized and with
> > debugging on.
> 
> cool, we can fix it then. I'll try it out.

nice, I looked a bit, I didn't get to far, but it seems textAccessibles children flags are eUninitializedChildren which I can only see being the case if aEvent->mAccessible was never added to it.  I also saw aEvent->mAccessible->mContent->GetParent() == textAccessible->mContent if that helps.

Comment 24

[alexander :surkov (:asurkov)]

I can reproduce it. So I'll debug it and land the existing patch to stop crashing.

Comment 25

[alexander :surkov (:asurkov)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/563cb4e5d1d5

I'll file another bug for the issue when I get something.

Comment 26

[alexander :surkov (:asurkov)]

I filed bug 759650 as follow up

Comment 27

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/563cb4e5d1d5

Comment 29

[u279076]

Marco, can you please verify this bug is fixed in Firefox 15.0a2 Aurora?

Comment 30

[u279076]

Removing QAWANTED as this is currently being tracked in our verification queries through status/tracking flags. Alex, pleas re-add if there is something more then simple verification that you need.

Comment 31

[Marco Zehe (:MarcoZ)]

This landed before the uplift of 15 to Aurora, so yeah should be fixed. I could never reproduce it myself, so have no manual means to verify, but from inspection, the patch is in Aurora.

Comment 32

[u279076]

I'm not seeing any recent crash reports with this signature. This in addition to comment 31 is the best we can do in terms of verification. We can re-open if this signature reappears.