Closed Bug 729831 Opened 9 years ago Closed 9 years ago

Crash @ mozilla::a11y::StyleInfo::Margin

Categories

(Core :: Disability Access APIs, defect)

13 Branch
All
Windows 7
defect
Not set
critical

Tracking

()

RESOLVED FIXED
mozilla13
Tracking Status
firefox13 - ---

People

(Reporter: Jamie, Assigned: surkov)

References

Details

(Keywords: crash, regression, reproducible)

Crash Data

Attachments

(2 files)

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.
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.
Blocks: 714579
Crash Signature: mozilla::a11y::StyleInfo::Margin(mozilla::css::Side, nsAString_internal&) → [@ mozilla::a11y::StyleInfo::Margin(mozilla::css::Side, nsAString_internal&)]
Keywords: topcrash
Hardware: x86_64 → All
Summary: Crash [@ mozilla::a11y::StyleInfo::Margin(mozilla::css::Side, nsAString_internal&) ] → Crash @ mozilla::a11y::StyleInfo::Margin
Version: Trunk → 13 Branch
Alexander, obviously it looks like we're failing here:
mElement->GetPrimaryFrame()->GetUsedMargin().Side(aSide);

Dunno why.
Assignee: nobody → surkov.alexander
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.
Attached patch patchSplinter Review
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.
Attachment #600850 - Flags: review?(trev.saunders)
(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.

==
Attachment #600850 - Flags: review?(trev.saunders) → review+
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.
https://hg.mozilla.org/mozilla-central/rev/fdd55a46661c
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla13
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.
Keywords: reproducible
(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.
Depends on: 732389
(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.
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!
(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.
It's #13 top crasher in 13.0a1.
don't calculate CSS object attributes on accessible unbound from the tree and for not primary accessibels (since it's senseless).
Attachment #604315 - Flags: review?(marco.zehe)
Comment on attachment 604315 [details] [diff] [review]
patch:catch unbounds

r=me.
Attachment #604315 - Flags: review?(marco.zehe) → review+
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.
(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
> > >+  // 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.
(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.
(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 :)
https://hg.mozilla.org/mozilla-central/rev/887d0e0873f9
Status: REOPENED → RESOLVED
Closed: 9 years ago9 years ago
Resolution: --- → FIXED
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.
This doesn't appear to be a top crash any longer on FF13 and 14.
You need to log in before you can comment on or make changes to this bug.