Closed
Bug 1264852
Opened 8 years ago
Closed 8 years ago
crash in mozilla::a11y::EventTree::Clear
Categories
(Core :: Disability Access APIs, defect)
Tracking
()
RESOLVED
FIXED
mozilla49
People
(Reporter: n.nethercote, Assigned: surkov)
Details
(Keywords: crash, regression)
Crash Data
Attachments
(1 file)
1.72 KB,
patch
|
n.nethercote
:
review+
lizzard
:
approval-mozilla-aurora+
|
Details | Diff | Splinter Review |
This bug was filed from the Socorro interface and is report bp-aefee8ce-46ec-4b5b-994c-b47c72160414. ============================================================= Just 2 crashes so far with this signature, both in Nightly 20160414030247. Both of them only have a single stack frame, oddly enough: > 0 xul.dll mozilla::a11y::EventTree::Clear() accessible/base/EventTree.cpp
Reporter | ||
Comment 1•8 years ago
|
||
surkov, this is in code you touched recently.
Flags: needinfo?(surkov.alexander)
Assignee | ||
Comment 2•8 years ago
|
||
No good ideas. Boris, do you know what it could mean?
Flags: needinfo?(surkov.alexander)
Comment 3•8 years ago
|
||
No, I do not. You could try loading the minidump in a debugger on Windows (or finding someone willing to do that and take a look) and seeing what you can figure out.
Reporter | ||
Comment 4•8 years ago
|
||
There have now been four crashes. They are all stack overflows. https://crash-stats.mozilla.com/report/index/f238bad9-1eb8-484c-9e24-5428d2160417 has a much better stack, which makes it clear that there's some kind of infinite loop during EventTree() destruction.
Reporter | ||
Updated•8 years ago
|
Flags: needinfo?(surkov.alexander)
Comment 5•8 years ago
|
||
Other EventTree signatures showing up: mozilla::a11y::EventTree::~EventTree mozilla::a11y::EventTree::`scalar deleting destructor'' NOTE: all are EXCEPTION_STACK_OVERFLOW (like this bug report)
Assignee | ||
Comment 6•8 years ago
|
||
I don't have good ideas on this, I'm not sure how nsAutoPtr may get into a loop at all. I tried to browsing a day with a11y turning on to catch a crash, but no luck. Can we summon some c++ guru to audit at the code, in case if I miss something evident.
Flags: needinfo?(surkov.alexander)
Reporter | ||
Comment 7•8 years ago
|
||
I took a look. Here's a relevant part of the stack frame. > 0 xul.dll mozilla::a11y::EventTree::Clear() accessible/bas e/EventTree.cpp > 1 xul.dll mozilla::a11y::EventTree::~EventTree() accessible/bas e/EventTree.h > 2 xul.dll mozilla::a11y::EventTree::`scalar deleting destructor' (unsigned int) > 3 xul.dll nsAutoPtr<mozilla::a11y::EventTree>::assign(mozilla::a 11y::EventTree*) obj-firefox/dist/include/nsAutoPtr.h > 4 xul.dll mozilla::a11y::EventTree::Clear() accessible/bas e/EventTree.cpp > 5 xul.dll mozilla::a11y::EventTree::~EventTree() accessible/bas e/EventTree.h Working from frame 5 up: - 5. ~EventTree() calls Clear() - 4. Clear() nulls out mNext, which is an nsAutoPtr<EventTree>. - 3. We end up in nsAutoPtr::assign... - 2. ... which calls |delete| on the EventTree pointed to by mNext. - 1. So enter ~EventTree() again, but for a different EventTree instance. So it looks like it's not infinite recursion, but if the EventTree list structure is long enough we could end up blowing the stack anyway. I suggest putting this into ~EventTree(): fprintf(stderr, "~EventTree(): %p\n", this); and see if it prints out the same address repeatedly (infinite recursion) or different addresses each time (non-infinite recursion). I tried this but I didn't get any output -- I don't know what's involved to trigger the execution of this code. If it is non-infinite recursion, I see two paths forward. - Determine how big the list can get, and if it is getting big, whether this should be happening. - Find a way to destroy the list using iteration -- changing mNext so it's not an nsAutoPtr<> might help.
Flags: needinfo?(surkov.alexander)
Reporter | ||
Updated•8 years ago
|
Crash Signature: [@ mozilla::a11y::EventTree::Clear] → [@ mozilla::a11y::EventTree::Clear]
[@ mozilla::a11y::EventTree::~EventTree]
[@ mozilla::a11y::EventTree::`scalar deleting destructor'']
Comment 8•8 years ago
|
||
Alex, some URLs to try: http://boards.4chan.org/tv/archive http://forum.xda-developers.com/
Comment 9•8 years ago
|
||
This report has a longer stack: https://crash-stats.mozilla.com/report/index/a6fcd834-e67b-49dc-9dcd-057e82160424
Assignee | ||
Comment 10•8 years ago
|
||
I was able to reproduce it once, it's indeed a huge tree, and all nodes are different objects (In reply to Nicholas Nethercote [:njn] from comment #7) > If it is non-infinite recursion, I see two paths forward. > > - Determine how big the list can get, and if it is getting big, whether this > should be happening. It can be as big as DOM tree in time between two layout flushes. All accessible tree changes caused by DOM/style changes are accumulated in that tree. > - Find a way to destroy the list using iteration -- that should make a trick, since Process() called right before Clear() doesn't crash. Btw, not sure I see a reason why. > changing mNext so it's > not > an nsAutoPtr<> might help. can you give details on this?
Flags: needinfo?(surkov.alexander)
Assignee | ||
Comment 11•8 years ago
|
||
clear as we go
Assignee: nobody → surkov.alexander
Attachment #8745074 -
Flags: review?(n.nethercote)
Reporter | ||
Comment 12•8 years ago
|
||
Comment on attachment 8745074 [details] [diff] [review] patch Review of attachment 8745074 [details] [diff] [review]: ----------------------------------------------------------------- Seems plausible. Let's cross our fingers.
Attachment #8745074 -
Flags: review?(n.nethercote) → review+
Assignee | ||
Comment 13•8 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=374e18b3b485
Reporter | ||
Comment 14•8 years ago
|
||
Is it possible to write a test for this?
Assignee | ||
Comment 15•8 years ago
|
||
(In reply to Nicholas Nethercote [:njn] from comment #14) > Is it possible to write a test for this? it should be doable, let me check
Assignee | ||
Comment 16•8 years ago
|
||
(In reply to alexander :surkov from comment #15) > (In reply to Nicholas Nethercote [:njn] from comment #14) > > Is it possible to write a test for this? > > it should be doable, let me check failed to reproduce in artificial test, but I do on a web site snapshot I taken. I filed bug 1268562.
Assignee | ||
Comment 17•8 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/13d3afcb47e455fde5e5c094a31ad540964bdd40 Bug 1264852 - destroy an event tree as it gets processed, r=n.nethercote
Comment 18•8 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/13d3afcb47e4
Status: NEW → RESOLVED
Closed: 8 years ago
status-firefox49:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla49
Assignee | ||
Comment 19•8 years ago
|
||
Comment on attachment 8745074 [details] [diff] [review] patch Approval Request Comment [Feature/regressing bug #]:bug 1261425 [User impact if declined]:crashes [Describe test coverage new/current, TreeHerder]:test coverage presented, however the crash itself doesn't have a test yet [Risks and why]: moderate, a simple patch for not trivial code [String/UUID change made/needed]:no
Attachment #8745074 -
Flags: approval-mozilla-aurora?
Updated•8 years ago
|
Keywords: regression
Comment 20•8 years ago
|
||
Comment on attachment 8745074 [details] [diff] [review] patch Crash fix for regression in 48.
Attachment #8745074 -
Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Comment 21•8 years ago
|
||
bugherder uplift |
https://hg.mozilla.org/releases/mozilla-aurora/rev/f00dc61ce26f
You need to log in
before you can comment on or make changes to this bug.
Description
•