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

RESOLVED FIXED in mozilla13

Status

()

Core
Disability Access APIs
--
critical
RESOLVED FIXED
6 years ago
5 years ago

People

(Reporter: MarcoZ, Assigned: surkov)

Tracking

({crash, regression})

11 Branch
mozilla13
x86
Windows NT
crash, regression
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(crash signature)

Attachments

(2 attachments)

(Reporter)

Description

6 years ago
This bug was filed from the Socorro interface and is 
report bp-34af506f-9899-48c3-b4a9-89c4c2111229 .
=============================================================
(Reporter)

Updated

6 years ago
Component: Disability Access → Disability Access APIs
Product: Firefox → Core
QA Contact: disability.access → accessibility-apis
(Reporter)

Comment 1

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

Comment 2

5 years ago
And a third report:
bp-af10f700-40ce-4a38-bb6d-a5ce42120114
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
Keywords: regression
(Reporter)

Comment 4

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

Comment 5

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

Comment 6

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

Comment 7

5 years ago
(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?
(Reporter)

Comment 8

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

Comment 9

5 years ago
Created attachment 597397 [details] [diff] [review]
fix caretOffset
Attachment #597397 - Flags: review?(trev.saunders)
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...
Patch reviewed over IRC by trevor.
Committed on behalf of Alex, with approval for David.

https://hg.mozilla.org/integration/mozilla-inbound/rev/be6534bf79e3
Target Milestone: --- → mozilla13

Updated

5 years ago
Attachment #597397 - Flags: review+
Note landing was to stop the bleeding (preventing Marco from dogfooding nightlies). BZ's comment 10 probably warrants a separate bug.
Target Milestone: mozilla13 → ---
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.
Attachment #597397 - Flags: review?(trev.saunders) → review+
(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.
(Assignee)

Comment 15

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

Comment 16

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

Updated

5 years ago
Depends on: 727735
(Assignee)

Updated

5 years ago
Assignee: nobody → surkov.alexander
(Assignee)

Comment 17

5 years ago
landed https://hg.mozilla.org/mozilla-central/rev/be6534bf79e3
https://hg.mozilla.org/mozilla-central/rev/be6534bf79e3
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla13
(Assignee)

Comment 19

5 years ago
reopen until original crash is fixed
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
(Assignee)

Comment 20

5 years ago
Created attachment 597861 [details] [diff] [review]
fix attributes crash
Attachment #597861 - Flags: review?(trev.saunders)
Attachment #597861 - Flags: feedback?(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....
Attachment #597861 - Flags: feedback?(bzbarsky) → feedback+
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
(Assignee)

Comment 23

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

Comment 24

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

Updated

5 years ago
Blocks: 728127
(Assignee)

Updated

5 years ago
No longer depends on: 727735
(Assignee)

Updated

5 years ago
Duplicate of this bug: 694818

Comment 26

5 years ago
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 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?
Attachment #597861 - Flags: review?(trev.saunders)
Attachment #597861 - Flags: review?(marco.zehe)
Attachment #597861 - Flags: review+
(Reporter)

Comment 28

5 years ago
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.
Attachment #597861 - Flags: review?(marco.zehe) → review+
(Assignee)

Comment 29

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

Comment 30

5 years ago
landed with Hub, Trevor and Marco comments addressed https://hg.mozilla.org/integration/mozilla-inbound/rev/6a43d088a2b4
https://hg.mozilla.org/mozilla-central/rev/6a43d088a2b4
Status: REOPENED → RESOLVED
Last Resolved: 5 years ago5 years ago
Resolution: --- → FIXED

Updated

5 years ago
Depends on: 729831

Comment 32

5 years ago
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
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
None of these stack trace seems to have accessibility. I would actually file a new bug for Toolkit.
Duplicate of this bug: 730470
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.
Status: REOPENED → RESOLVED
Last Resolved: 5 years ago5 years ago
Resolution: --- → FIXED

Updated

5 years ago
Summary: crash nsINode::OwnerDoc → crash in mozilla::a11y::FocusManager::IsFocused @ nsINode::OwnerDoc
You need to log in before you can comment on or make changes to this bug.