714579 - crash in mozilla::a11y::FocusManager::IsFocused @ nsINode::OwnerDoc

Status: RESOLVED FIXED

(Core :: Disability Access APIs, defect)

Description

[Marco Zehe (:MarcoZ)]

This bug was filed from the Socorro interface and is 
report bp-34af506f-9899-48c3-b4a9-89c4c2111229 .
=============================================================

Comment 1

[Marco Zehe (:MarcoZ)]

Another report from the same community member:
https://crash-stats.mozilla.com/report/index/bp-0335df9e-800e-4803-8865-5871a2120103

Comment 2

[Marco Zehe (:MarcoZ)]

And a third report:
bp-af10f700-40ce-4a38-bb6d-a5ce42120114

Comment 3

[Wayne Mery (:wsmwk)]

I crashed bp-dd78416c-01d3-4a6b-9987-3740f2120121. Don't know why it kicked out this signature, as I don't explicitly use accessibility features. First time I've ever gotten this signature.

0 	xul.dll 	nsINode::OwnerDoc 	obj-firefox/dist/include/nsINode.h:423
1 	xul.dll 	nsXULPopupManager::HidePopupsInDocShell 	
2 	xul.dll 	nsSubDocumentFrame::AttributeChanged 	layout/generic/nsSubDocumentFrame.cpp:763
3 	xul.dll 	nsCSSFrameConstructor::AttributeChanged 	layout/base/nsCSSFrameConstructor.cpp:8266

Per crash-stats reports, nsINode::OwnerDoc appears to be a regression - the earliest report in last 4 weeks is v10.0a2 bp-29f8dbda-af4f-471d-aa86-955122120113. 

However, frame 1 of Marco's citations is different:
bp-af10f700-40ce-4a38-bb6d-a5ce42120114
0	xul.dll	nsINode::OwnerDoc	obj-firefox/dist/include/nsINode.h:423
1	xul.dll	mozilla::a11y::FocusManager::IsFocused	accessible/src/base/FocusManager.cpp:85
2	xul.dll	nsHyperTextAccessible::GetAttributesInternal	accessible/src/html/nsHyperTextAccessible.cpp:1234
3	xul.dll	nsCOMPtr<nsIPersistentProperties>::operator=	obj-firefox/dist/include/nsCOMPtr.h:718
4	xul.dll	nsAccessible::GetAttributes	accessible/src/base/nsAccessible.cpp:1262
5	xul.dll	nsDocAccessible::GetAttributes	accessible/src/base/nsDocAccessible.cpp:343
6	xul.dll	nsAccessibleWrap::get_attributes	accessible/src/msaa/nsAccessibleWrap.cpp:1446

Comment 4

[Marco Zehe (:MarcoZ)]

First time I've seen this myself. For me, this just happened after the update to Mozilla/5.0 (Windows NT 6.1; WOW64; rv:13.0a1) Gecko/20120214 Firefox/13.0a1
Report is https://crash-stats.mozilla.com/report/index/bp-a01629fb-2265-4302-bd69-2764e2120215

Comment 5

[alexander :surkov (:asurkov)]

(In reply to Marco Zehe (:MarcoZ) from comment #4)
> First time I've seen this myself. For me, this just happened after the
> update to Mozilla/5.0 (Windows NT 6.1; WOW64; rv:13.0a1) Gecko/20120214
> Firefox/13.0a1
> Report is
> https://crash-stats.mozilla.com/report/index/bp-a01629fb-2265-4302-bd69-
> 2764e2120215

this one is different and I think can be fixed by putting IsDefunct() check into nsHyperTextAccessible::GetCaretOffset. Does anybody want to take?

Comment 6

[alexander :surkov (:asurkov)]

(In reply to Wayne Mery (:wsmwk) from comment #3)
> I crashed bp-dd78416c-01d3-4a6b-9987-3740f2120121. Don't know why it kicked
> out this signature, as I don't explicitly use accessibility features. First
> time I've ever gotten this signature.

for this one (non accessibility crash) is better to file another bug

Comment 7

[alexander :surkov (:asurkov)]

(In reply to Marco Zehe (:MarcoZ) from comment #2)
> And a third report:
> bp-af10f700-40ce-4a38-bb6d-a5ce42120114

this one sounds like we shutdown document accessible when GetAttributes is running. I don't see any suspicious calls except nsIDOMWindow::GetComputedStyle on HTML body or document element. If this call can result in "pagehide" event of destroy a document's presshell then we shutdown the document accessible.

Boris, is this scenario possible?

Comment 8

[Marco Zehe (:MarcoZ)]

OK, this really has got worse for me starting with the above mentioned build (Feb 14, 2012). I know others have seen it prior to this, but this is when I started seeing this crash myself. It crashed twice within two minutes for me just now.

Comment 9

[alexander :surkov (:asurkov)]

Attachment: fix caretOffset

Comment 10

[Boris Zbarsky [:bzbarsky]]

getComputedStyle on an element in document D can lead to destruction of any presshell in the document tree containing document D.

Though I would not have expected a getComputedStyle call under AttributeChanged to do that; it should silently return possibly-incorrect information instead...

Comment 11

[Hubert Figuiere [:hub]]

Patch reviewed over IRC by trevor.
Committed on behalf of Alex, with approval for David.

https://hg.mozilla.org/integration/mozilla-inbound/rev/be6534bf79e3

Comment 12

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

Note landing was to stop the bleeding (preventing Marco from dogfooding nightlies). BZ's comment 10 probably warrants a separate bug.

Comment 13

[Trevor Saunders (:tbsaunde)]

Comment on attachment 597397 [details] [diff] [review]
fix caretOffset

you should add a check to CAccessibleText::Get_CaretOffset() eventually, but I guess its fine to leave it as is for now since xpcom methods need to check anyway.

Comment 14

[Trevor Saunders (:tbsaunde)]

(In reply to David Bolter [:davidb] from comment #12)
> Note landing was to stop the bleeding (preventing Marco from dogfooding
> nightlies). BZ's comment 10 probably warrants a separate bug.

I'm not sure what the point of this is this bug seems to be  a mix of several bugs where one of those bugs if fixed by the patch in question.

I don't really care if we deal with the other a11y crash here or some other bug.

Comment 15

[alexander :surkov (:asurkov)]

(In reply to Trevor Saunders (:tbsaunde) from comment #13)
> Comment on attachment 597397 [details] [diff] [review]
> fix caretOffset
> 
> you should add a check to CAccessibleText::Get_CaretOffset() eventually, but
> I guess its fine to leave it as is for now since xpcom methods need to check
> anyway.

yes, it'll be handled by bug 539683

Comment 16

[alexander :surkov (:asurkov)]

(In reply to Boris Zbarsky (:bz) from comment #10)

> Though I would not have expected a getComputedStyle call under
> AttributeChanged to do that; it should silently return possibly-incorrect
> information instead...

this isn't it under AttributeChanged, this is under external MSAA call.

Comment 17

[alexander :surkov (:asurkov)]

landed https://hg.mozilla.org/mozilla-central/rev/be6534bf79e3

Comment 18

[Marco Bonardo [:mak]]

https://hg.mozilla.org/mozilla-central/rev/be6534bf79e3

Comment 19

[alexander :surkov (:asurkov)]

reopen until original crash is fixed

Comment 20

[alexander :surkov (:asurkov)]

Attachment: fix attributes crash

Comment 21

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 597861 [details] [diff] [review]
fix attributes crash

Ah, cute.

I wonder whether it makes sense to just expose a non-flushing version of nsComputedDOMStyle....

Comment 22

[Hubert Figuiere [:hub]]

Comment on attachment 597861 [details] [diff] [review]
fix attributes crash

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

just a nit. drive by review.

::: accessible/tests/mochitest/attributes.js
@@ +27,5 @@
>    testAttrsInternal(aAccOrElmOrID, {}, true, aAbsentAttrs);
>  }
>  
>  /**
> + * Test CSS based objec attributes.

nit: typo / spelling

Comment 23

[alexander :surkov (:asurkov)]

(In reply to Boris Zbarsky (:bz) from comment #21)
> Comment on attachment 597861 [details] [diff] [review]
> fix attributes crash
> 
> Ah, cute.
> 
> I wonder whether it makes sense to just expose a non-flushing version of
> nsComputedDOMStyle....

I believe this is correct approach but when I started to look at it then I realized I have more questions than answers so I ended up with porting code to a11y. I'll open a bug for this.

Comment 24

[alexander :surkov (:asurkov)]

(In reply to alexander :surkov from comment #23)
> (In reply to Boris Zbarsky (:bz) from comment #21)
> > Comment on attachment 597861 [details] [diff] [review]
> > fix attributes crash
> > 
> > Ah, cute.
> > 
> > I wonder whether it makes sense to just expose a non-flushing version of
> > nsComputedDOMStyle....
> 
> I believe this is correct approach but when I started to look at it then I
> realized I have more questions than answers so I ended up with porting code
> to a11y. I'll open a bug for this.

I filed bug 728126

Comment 26

[Scoobidiver (away)]

There's a spike in crashes from 13.0a1/20120214. The regression range for the spike is:
http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=c90c9f273cad&tochange=60edf587f4af
There are three possible culprits: bug 724452, bug 725581, and bug 725998.

Does the second patch fix every cases?

Comment 27

[Trevor Saunders (:tbsaunde)]

Comment on attachment 597861 [details] [diff] [review]
fix attributes crash


>   TextUpdater.cpp \
>+  Style.cpp \

shouldn't it come before TextUpdater?

>+  -I$(srcdir)/../../../layout/style \

since bz is fine with that I gues its fine by me.

>+Style::~Style()
>+{
>+}

inline it if you like if it were going to be a heap object I wouldn't care, but destruction of stack objects should be fast :)

>+#include "mozilla/gfx/Types.h"

do you use this?

>+class Style

I might call it StyleInfo, and please comment it.

>+private:
>+  Style(const Style&) MOZ_DELETE;
>+  Style& operator = (const Style&) MOZ_DELETE;

get rid of default constructor taking no args too?

>-#include "nsDocAccessible.h"

we use methods on it in this file no? I'd say keep it since depending on random headers for inclusion can lead to pain

> #include "nsRootAccessible.h"
>+#include "nsEventShell.h"

that's out of order no?

r=me for accessible/src/, Marco mind looking the tests over?

Comment 28

[Marco Zehe (:MarcoZ)]

Comment on attachment 597861 [details] [diff] [review]
fix attributes crash

>+ * Test CSS based objec attributes.

Nit: Typo "object", missing t.

Nit: In accessible/tests/mochitest/attributes/test_obj_css.xul , change all instances of "diplay" to "display", please.
Nit: Is using the % char in the IDs of elements really wise? I'd prefer if we used the word "percent" instead.

r=me with these addressed/explained.

Comment 29

[alexander :surkov (:asurkov)]

(In reply to Scoobidiver from comment #26)
> There's a spike in crashes from 13.0a1/20120214. The regression range for
> the spike is:
> http://hg.mozilla.org/mozilla-central/
> pushloghtml?fromchange=c90c9f273cad&tochange=60edf587f4af
> There are three possible culprits: bug 724452, bug 725581, and bug 725998.
> 
> Does the second patch fix every cases?

bug 725581 was guilty for the crash fixed by first patch
second patch is supposed to fix the original crash
Wayne in comment #3 reported about non-accessibility crashes, for those, another bug should be filed

Comment 30

[alexander :surkov (:asurkov)]

landed with Hub, Trevor and Marco comments addressed https://hg.mozilla.org/integration/mozilla-inbound/rev/6a43d088a2b4

Comment 31

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/6a43d088a2b4

Comment 32

[Scoobidiver (away)]

There are more crashes after the patch landed than before:
https://crash-stats.mozilla.com/report/list?product=Firefox&version=Firefox%3A13.0a1&query_search=signature&query_type=contains&reason_type=contains&range_value=28&range_unit=days&hang_type=any&process_type=any&do_query=1&signature=nsINode%3A%3AOwnerDoc%28%29

Comment 33

[Hubert Figuiere [:hub]]

None of these stack trace seems to have accessibility. I would actually file a new bug for Toolkit.

Comment 35

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

I'm mid-air colliding all over.

I filed bug 730470 since accessibility doesn't seem to appear in the stacks of the new ones. Please reopen if I'm mistaken.