Last Comment Bug 714579 - crash in mozilla::a11y::FocusManager::IsFocused @ nsINode::OwnerDoc
: crash in mozilla::a11y::FocusManager::IsFocused @ nsINode::OwnerDoc
Status: RESOLVED FIXED
: crash, regression
Product: Core
Classification: Components
Component: Disability Access APIs (show other bugs)
: 11 Branch
: x86 Windows NT
: -- critical (vote)
: mozilla13
Assigned To: alexander :surkov
:
Mentors:
: 694818 (view as bug list)
Depends on: 729831
Blocks: 728127
  Show dependency treegraph
 
Reported: 2012-01-01 23:15 PST by Marco Zehe (:MarcoZ)
Modified: 2012-02-24 23:29 PST (History)
11 users (show)
See Also:
Crash Signature:
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
fix caretOffset (822 bytes, patch)
2012-02-15 07:11 PST, alexander :surkov
tbsaunde+mozbugs: review+
hub: review+
Details | Diff | Splinter Review
fix attributes crash (25.35 KB, patch)
2012-02-16 09:18 PST, alexander :surkov
tbsaunde+mozbugs: review+
mzehe: review+
bzbarsky: feedback+
Details | Diff | Splinter Review

Description Marco Zehe (:MarcoZ) 2012-01-01 23:15:44 PST
This bug was filed from the Socorro interface and is 
report bp-34af506f-9899-48c3-b4a9-89c4c2111229 .
=============================================================
Comment 1 Marco Zehe (:MarcoZ) 2012-01-03 09:52:29 PST
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) 2012-01-16 00:00:51 PST
And a third report:
bp-af10f700-40ce-4a38-bb6d-a5ce42120114
Comment 3 Wayne Mery (:wsmwk, NI for questions) 2012-01-21 08:26:27 PST
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) 2012-02-15 05:47:58 PST
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 2012-02-15 06:10:57 PST
(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 2012-02-15 06:29:06 PST
(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 2012-02-15 06:32:44 PST
(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) 2012-02-15 06:51:50 PST
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 2012-02-15 07:11:37 PST
Created attachment 597397 [details] [diff] [review]
fix caretOffset
Comment 10 Boris Zbarsky [:bz] 2012-02-15 08:51:38 PST
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] 2012-02-15 09:17:02 PST
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] 2012-02-15 09:21:02 PST
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) 2012-02-15 11:24:56 PST
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) 2012-02-15 11:26:37 PST
(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 2012-02-15 18:49:52 PST
(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 2012-02-15 19:04:57 PST
(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 2012-02-16 02:51:29 PST
landed https://hg.mozilla.org/mozilla-central/rev/be6534bf79e3
Comment 18 Marco Bonardo [::mak] 2012-02-16 02:55:03 PST
https://hg.mozilla.org/mozilla-central/rev/be6534bf79e3
Comment 19 alexander :surkov 2012-02-16 02:56:58 PST
reopen until original crash is fixed
Comment 20 alexander :surkov 2012-02-16 09:18:34 PST
Created attachment 597861 [details] [diff] [review]
fix attributes crash
Comment 21 Boris Zbarsky [:bz] 2012-02-16 09:50:28 PST
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] 2012-02-16 11:52:25 PST
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 2012-02-16 19:29:26 PST
(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 2012-02-16 21:54:25 PST
(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 25 alexander :surkov 2012-02-16 22:50:31 PST
*** Bug 694818 has been marked as a duplicate of this bug. ***
Comment 26 Scoobidiver (away) 2012-02-17 06:37:36 PST
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) 2012-02-17 07:08:00 PST
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) 2012-02-17 07:22:52 PST
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 2012-02-17 08:19:14 PST
(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 2012-02-20 07:42:48 PST
landed with Hub, Trevor and Marco comments addressed https://hg.mozilla.org/integration/mozilla-inbound/rev/6a43d088a2b4
Comment 31 Ed Morley [:emorley] 2012-02-21 08:53:36 PST
https://hg.mozilla.org/mozilla-central/rev/6a43d088a2b4
Comment 33 Hubert Figuiere [:hub] 2012-02-24 15:19:48 PST
None of these stack trace seems to have accessibility. I would actually file a new bug for Toolkit.
Comment 34 Bill Gianopoulos [:WG9s] 2012-02-24 15:24:32 PST
*** Bug 730470 has been marked as a duplicate of this bug. ***
Comment 35 David Bolter [:davidb] 2012-02-24 15:25:40 PST
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.

Note You need to log in before you can comment on or make changes to this bug.