crash in nsIContent::GetPrimaryFrame() called from mozilla::a11y::TextAttrsMgr::BGColorTextAttr::GetValueFor(mozilla::a11y::Accessible *,unsigned int *)

RESOLVED FIXED in Firefox 48

Status

()

defect
--
critical
RESOLVED FIXED
5 years ago
3 years ago

People

(Reporter: MarcoZ, Assigned: surkov)

Tracking

({crash, regression})

Trunk
mozilla49
x86
Windows NT
Points:
---

Firefox Tracking Flags

(firefox48 fixed, firefox49 fixed)

Details

(crash signature)

Attachments

(1 attachment, 1 obsolete attachment)

This bug was filed from the Socorro interface and is 
report bp-63bc6a23-5853-4b6d-90bf-cc6b22140706.
=============================================================

This is probably related to, but not quite the same, as bug 819291. I was doing the following:

1. Visit http://www.tagesschau.de/inland/nsa-ausschuss-104.html
2. In the video's iframe, click on Settings, and make sure the player is set to HTML, not Flash.
3. Run NVDA.
4. Hit the Play button, and then alt-tab away from Firefox to not hear the ascending beeps from NVDA's progress bar indicator.

The crash occurred right after the video had finished playing.

The frames in this crash report point to the elm variable possibly being NULL here:
http://hg.mozilla.org/mozilla-central/annotate/81691a55e60f/accessible/base/TextAttrs.cpp#l343

Alex, would a simple null check here be the appropriate fix?
Flags: needinfo?(surkov.alexander)
I'd say so however I don't have good ideas what may be a cause. The accessible has to be a text leaf accessilbe which should always have a content and parent element. Perhaps it's a right place to add a check into TTextAttr::Equal.
Flags: needinfo?(surkov.alexander)
(In reply to alexander :surkov from comment #1)
> I'd say so however I don't have good ideas what may be a cause. The
> accessible has to be a text leaf accessilbe which should always have a
> content and parent element. Perhaps it's a right place to add a check into
> TTextAttr::Equal.

Hm, not sure what that would look like, or rather, what I should be comparing to what.
Flags: needinfo?(surkov.alexander)
OK, this is happening more often, and it appears to be something also involving plugins, since PluginContainer also crashes, but I have no reports of those in my about:crashes.

Here's my latest report from just a few minutes ago:
https://crash-stats.mozilla.com/report/index/4a533f26-6150-4a6d-a96c-514c92140711
This is a regression, since it doesn't crash with the same steps in the Firefox 30 release. What happens once I press Enter on the Users clickable in the Google Apps admin console is that focus is put onto a WAI-ARIA TreeView with filtering options, once the new content has finished loading.
Flags: needinfo?(surkov.alexander)
Keywords: regression
Sorry, cleared the needinfo request by accident.
Flags: needinfo?(surkov.alexander)
Summary: crash in nsIContent::GetPrimaryFrame() → crash in nsIContent::GetPrimaryFrame() called from mozilla::a11y::TextAttrsMgr::BGColorTextAttr::GetValueFor(mozilla::a11y::Accessible *,unsigned int *)
(In reply to Marco Zehe (:MarcoZ) from comment #2)
> (In reply to alexander :surkov from comment #1)
> > I'd say so however I don't have good ideas what may be a cause. The
> > accessible has to be a text leaf accessilbe which should always have a
> > content and parent element. Perhaps it's a right place to add a check into
> > TTextAttr::Equal.
> 
> Hm, not sure what that would look like, or rather, what I should be
> comparing to what.

no luck to reproduce it. I would try to add aAccessible->GetContent() check and assertion into TTextAttr::Equal right before GetValueFor call. If problems goes away then it is. Otherwise I would check result of nsCoreUtils::GetDOMElementFor on null
Flags: needinfo?(surkov.alexander)
Crash Signature: [@ nsIContent::GetPrimaryFrame()] → [@ nsIContent::GetPrimaryFrame()] [@ nsIContent::GetPrimaryFrame]
There have been a bunch of these crashes on Aurora 48.0a2 in the last few days, although from just 2 users (one of them in 2 different nightlies).

I took a look at bp-7a47279b-fcfe-436e-a645-d3ddb2160430 , which was one of the crashes submitted, and looked at the minidump, which confirmed that the crash is because nsIContent::GetPrimaryFrame is being called with this=0x0, i.e., because elm is null in the frame above.
Posted patch patch (obsolete) — Splinter Review
Attachment #8747790 - Flags: review?(mzehe)
Attachment #8747790 - Attachment is obsolete: true
Attachment #8747790 - Flags: review?(mzehe)
Attachment #8747796 - Flags: review?(mzehe)
Attachment #8747796 - Flags: review?(mzehe) → review+
Setting Affected Flags in accordance with comment #7. Make sure to get this into Aurora when it lands on Central.
https://hg.mozilla.org/mozilla-central/rev/b77b09e1e420
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla49
Hi Alexander,
Since this patch also affects 48, are you also considering to uplift this patch to 48?
Flags: needinfo?(surkov.alexander)
Comment on attachment 8747796 [details] [diff] [review]
patch + assertions

Approval Request Comment
[Feature/regressing bug #]:unknown, may be an old issue
[User impact if declined]:crashes
[Describe test coverage new/current, TreeHerder]:no automated test coverage, but no crashes on 49 anymore
[Risks and why]: low, bunch of null checks
[String/UUID change made/needed]:no
Flags: needinfo?(surkov.alexander)
Attachment #8747796 - Flags: approval-mozilla-aurora?
Comment on attachment 8747796 [details] [diff] [review]
patch + assertions

Fix a crash, taking it
Attachment #8747796 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Assignee: nobody → surkov.alexander
You need to log in before you can comment on or make changes to this bug.