729831 - Crash @ mozilla::a11y::StyleInfo::Margin

Status: RESOLVED FIXED

(Core :: Disability Access APIs, defect)

Description

[James Teh [:Jamie]]

Str:
1. Start Firefox and NVDA on a system where Firefox is the default browser.
2. Press Windows+r to open the Run dialog.
3. Type any URL (e.g. http://www.mozilla.org/) and press enter.
Result: Crash!

Opening a link from another application has the same result.

bp-630e190f-1f66-40e3-934e-279d92120223
bp-05b6a43e-9f9e-4496-9e86-638d12120223
bp-9c5f7162-9dc5-4a27-b5ab-5b36b2120223

I suspect this was introduced in either the 2012-02-22 or 2012-02-21 builds.

Comment 1

[Scoobidiver (away)]

The stack looks like:
Frame 	Module 	Signature [Expand] 	Source
0 	xul.dll 	mozilla::a11y::StyleInfo::Margin 	accessible/src/base/StyleInfo.cpp:109
1 	xul.dll 	nsAccessible::GetAttributesInternal 	accessible/src/base/nsAccessible.cpp:1466
2 	rsaenh.dll 	AesCtrRng_Update 	
3 	xul.dll 	nsHyperTextAccessible::GetAttributesInternal 	accessible/src/html/nsHyperTextAccessible.cpp:1218
4 	xul.dll 	nsPersistentProperties::QueryInterface 	xpcom/ds/nsPersistentProperties.cpp:529
5 	xul.dll 	nsPersistentProperties::Create 	xpcom/ds/nsPersistentProperties.cpp:523
6 	xul.dll 	nsPersistentProperties::Create 	xpcom/ds/nsPersistentProperties.cpp:525
7 	xul.dll 	mozilla::GenericFactory::CreateInstance 	obj-firefox/xpcom/build/GenericFactory.cpp:48
8 	xul.dll 	nsComponentManagerImpl::CreateInstanceByContractID 	xpcom/components/nsComponentManager.cpp:1064
9 	xul.dll 	nsRefPtr<nsPresContext>::~nsRefPtr<nsPresContext> 	obj-firefox/dist/include/nsAutoPtr.h:908
10 	xul.dll 	nsComponentManagerImpl::CreateInstanceByContractID 	xpcom/components/nsComponentManager.cpp:1079
11 	xul.dll 	CallCreateInstance 	obj-firefox/xpcom/build/nsComponentManagerUtils.cpp:170
12 	xul.dll 	nsCreateInstanceByContractID::operator 	obj-firefox/xpcom/build/nsComponentManagerUtils.cpp:210
13 	xul.dll 	nsCOMPtr_base::assign_from_helper 	obj-firefox/xpcom/build/nsCOMPtr.cpp:152
14 	xul.dll 	nsCOMPtr<nsIPersistentProperties>::operator= 	obj-firefox/dist/include/nsCOMPtr.h:718
15 	xul.dll 	nsAccessible::GetAttributes 	accessible/src/base/nsAccessible.cpp:1317

(In reply to James Teh [:Jamie] from comment #0)
> I suspect this was introduced in either the 2012-02-22 or 2012-02-21 builds.
It first appeared in 13.0a1/20120222 and is #7 top crasher over the last day.
The regression range is:
http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=be559203ece8&tochange=373c710112e6
It's likely a regression from bug 714579.

Comment 2

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

Alexander, obviously it looks like we're failing here:
mElement->GetPrimaryFrame()->GetUsedMargin().Side(aSide);

Dunno why.

Comment 3

[alexander :surkov (:asurkov)]

So the crash happens on document accessible that wasn't loaded yet, no frames were created yet. We create a document accessible whenever someone asks for it so that AT can get it early. It seems like we need to not expose the created document to AT until it's ready. That'll be a right fix.

Comment 4

[alexander :surkov (:asurkov)]

Attachment: patch

Let's do not create document without root frame that makes us to delay the document creation. This makes me a bit worry if we can miss some DOM events like DOM focus but we shouldn't I think - let's keep an eye on it.

Comment 5

[Trevor Saunders (:tbsaunde)]

(In reply to alexander :surkov from comment #4)
> Created attachment 600850 [details] [diff] [review]
> patch
> 
> Let's do not create document without root frame that makes us to delay the
> document creation. This makes me a bit worry if we can miss some DOM events
> like DOM focus but we shouldn't I think - let's keep an eye on it.

==

Comment 6

[alexander :surkov (:asurkov)]

for the record what happens here:
we create document accessible, NotificationController of parent document attaches it to the tree, then NotificationController of created document does initial tree update (which does nothing because no frames at this point) and then it fires reorder event. NVDA gets it and things the document is ready and asks for object attributes.

Comment 7

[alexander :surkov (:asurkov)]

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

Comment 8

[Matt Brubeck (:mbrubeck)]

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

Comment 9

[Scoobidiver (away)]

There are still crashes in 13.0a1/20120229:
https://crash-stats.mozilla.com/report/list?signature=mozilla%3A%3Aa11y%3A%3AStyleInfo%3A%3AMargin%28mozilla%3A%3Acss%3A%3ASide%2C%20nsAString_internal%26%29

Comment 10

[alexander :surkov (:asurkov)]

stacks aren't very descriptive, it seems crash happens during cycle collection. However I see we can crash when for example HTML body goes away so the document has stale state.

Comment 11

[James Teh [:Jamie]]

You can reproduce this crash by visiting this URL with NVDA running: http://blogs.creativecow.net/blog/6056/behind-the-scenes-of-the-new-fast-track-c-series-interfaces-part-3
bp-314ab979-49a2-4754-a90c-ca6592120301

Comment 12

[alexander :surkov (:asurkov)]

(In reply to James Teh [:Jamie] from comment #11)
> You can reproduce this crash by visiting this URL with NVDA running:
> http://blogs.creativecow.net/blog/6056/behind-the-scenes-of-the-new-fast-
> track-c-series-interfaces-part-3
> bp-314ab979-49a2-4754-a90c-ca6592120301

Thanks, Jamie. I can reproduce it.

Comment 13

[alexander :surkov (:asurkov)]

(In reply to James Teh [:Jamie] from comment #11)
> You can reproduce this crash by visiting this URL with NVDA running:
> http://blogs.creativecow.net/blog/6056/behind-the-scenes-of-the-new-fast-
> track-c-series-interfaces-part-3

it seems image map accessible tree doesn't get updated, I filed bug 732389.

Comment 14

[James Teh [:Jamie]]

Same crash, but just in case it has a different cause: bp-9406fe91-1246-4c13-ab56-e59852120305
Str:
1. With NVDA running, visit the pdf.js demo here: http://mozilla.github.com/pdf.js/web/viewer.html
2. Wait until the pdf document finishes loading.
Crash!

Comment 15

[alexander :surkov (:asurkov)]

(In reply to James Teh [:Jamie] from comment #14)
> Str:
> 1. With NVDA running, visit the pdf.js demo here:
> http://mozilla.github.com/pdf.js/web/viewer.html
> 2. Wait until the pdf document finishes loading.
> Crash!

Thanks, I can reproduce it. Need to figure our the fix.

Comment 16

[Scoobidiver (away)]

It's #13 top crasher in 13.0a1.

Comment 17

[alexander :surkov (:asurkov)]

Attachment: patch:catch unbounds

don't calculate CSS object attributes on accessible unbound from the tree and for not primary accessibels (since it's senseless).

Comment 18

[Marco Zehe (:MarcoZ)]

Comment on attachment 604315 [details] [diff] [review]
patch:catch unbounds

r=me.

Comment 19

[alexander :surkov (:asurkov)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/887d0e0873f9

Comment 20

[Trevor Saunders (:tbsaunde)]

Comment on attachment 604315 [details] [diff] [review]
patch:catch unbounds

>+  // Don't calculate CSS-based object attributes when no frame (i.e.
>+  // the accessible is not unattached form three) or when the accessible is not

I'm not sure I follow the is not unattached form bit.

>+  // primary for node (like list bullet or XUL tree items).

then shouldn't you test we don't crash for xul tree items too?

btw I'm not sure why the diff is so big, it seems like there was little actual change, but it looks fine to me.

Comment 21

[alexander :surkov (:asurkov)]

(In reply to Trevor Saunders (:tbsaunde) from comment #20)
> Comment on attachment 604315 [details] [diff] [review]
> patch:catch unbounds
> 
> >+  // Don't calculate CSS-based object attributes when no frame (i.e.
> >+  // the accessible is not unattached form three) or when the accessible is not
> 
> I'm not sure I follow the is not unattached form bit.

should be "the accessible is unattached from tree".

> >+  // primary for node (like list bullet or XUL tree items).
> 
> then shouldn't you test we don't crash for xul tree items too?

we don't crash, I added a check to not expose those attrs for non primary accessibles while I'm here. Sorry for mixing bugs.

> btw I'm not sure why the diff is so big, it seems like there was little
> actual change, but it looks fine to me.

I was needed to move draggable stuffs before CSS-based attrs getting

Comment 22

[Trevor Saunders (:tbsaunde)]

> > >+  // Don't calculate CSS-based object attributes when no frame (i.e.
> > >+  // the accessible is not unattached form three) or when the accessible is not
> > 
> > I'm not sure I follow the is not unattached form bit.
> 
> should be "the accessible is unattached from tree".

makes sense now :)

> > >+  // primary for node (like list bullet or XUL tree items).
> > 
> > then shouldn't you test we don't crash for xul tree items too?
> 
> we don't crash, I added a check to not expose those attrs for non primary
> accessibles while I'm here. Sorry for mixing bugs.

its cool, but shouldn't we still have tests we don't expose css stuffs on non-primary nodes?

> > btw I'm not sure why the diff is so big, it seems like there was little
> > actual change, but it looks fine to me.
> 
> I was needed to move draggable stuffs before CSS-based attrs getting

yeah, makes sense.

Comment 23

[alexander :surkov (:asurkov)]

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

> > > then shouldn't you test we don't crash for xul tree items too?
> > 
> > we don't crash, I added a check to not expose those attrs for non primary
> > accessibles while I'm here. Sorry for mixing bugs.
> 
> its cool, but shouldn't we still have tests we don't expose css stuffs on
> non-primary nodes?

we have (listitem bullet), we don't have a tests for accessible not in tree. Since Firefox 13 branching is too close then I filed for review what I had. I'll do follow up to make things the way I liked them to see.

Comment 24

[Trevor Saunders (:tbsaunde)]

(In reply to alexander :surkov from comment #23)
> (In reply to Trevor Saunders (:tbsaunde) from comment #22)
> 
> > > > then shouldn't you test we don't crash for xul tree items too?
> > > 
> > > we don't crash, I added a check to not expose those attrs for non primary
> > > accessibles while I'm here. Sorry for mixing bugs.
> > 
> > its cool, but shouldn't we still have tests we don't expose css stuffs on
> > non-primary nodes?
> 
> we have (listitem bullet), we don't have a tests for accessible not in tree.
> Since Firefox 13 branching is too close then I filed for review what I had.
> I'll do follow up to make things the way I liked them to see.

sure, I agree with landing what you had, and just wanted to point out what could be improved :)

Comment 25

[Marco Bonardo [:mak]]

https://hg.mozilla.org/mozilla-central/rev/887d0e0873f9

Comment 26

[Marco Zehe (:MarcoZ)]

This bug is in Firefox 13, it landed on march 10, 3 days before 13 was up-lifted to Aurora. The only thing QA can do is to verify that the crashes indeed disappeared from stats in the last month on 13 and 14 branches.

Comment 27

[Alex Keybl [:akeybl]]

This doesn't appear to be a top crash any longer on FF13 and 14.