Last Comment Bug 717505 - Crash [@ nsAccessible::VisibilityState] when closing a tab, depending on when accessibility was enabled
: Crash [@ nsAccessible::VisibilityState] when closing a tab, depending on when...
Status: RESOLVED FIXED
: crash, regression, testcase
Product: Core
Classification: Components
Component: Disability Access APIs (show other bugs)
: Trunk
: All All
: -- critical (vote)
: mozilla12
Assigned To: David Bolter [:davidb]
:
: alexander :surkov
Mentors:
Depends on:
Blocks: 594645 591363
  Show dependency treegraph
 
Reported: 2012-01-11 19:44 PST by Jesse Ruderman
Modified: 2012-01-24 04:47 PST (History)
7 users (show)
See Also:
Crash Signature:
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
testcase (requires extension for timing) (239 bytes, text/html)
2012-01-11 19:44 PST, Jesse Ruderman
no flags Details
stack trace (5.65 KB, text/plain)
2012-01-11 19:45 PST, Jesse Ruderman
no flags Details
frame check (1.04 KB, patch)
2012-01-20 12:29 PST, David Bolter [:davidb]
roc: review+
Details | Diff | Splinter Review

Description Jesse Ruderman 2012-01-11 19:44:17 PST
Created attachment 587927 [details]
testcase (requires extension for timing)

This testcase requires https://www.squarefree.com/extensions/domFuzzLite3.xpi, but I think it's possible to trigger the bug with a less insane timing sequence.
Comment 1 Jesse Ruderman 2012-01-11 19:45:17 PST
Created attachment 587928 [details]
stack trace
Comment 3 Marco Zehe (:MarcoZ) 2012-01-12 05:18:15 PST
I haven't seen this crash myself on Windows using NVDA 2011.3.
Comment 4 David Bolter [:davidb] 2012-01-12 07:02:10 PST
I'm trying to recreate on windows. Jesse do I just keep pressing the button that opens another tab and closes it? (I have the fuzzer extension)
Comment 5 Scoobidiver (away) 2012-01-12 07:12:39 PST
(In reply to David Bolter [:davidb] from comment #4)
> I'm trying to recreate on windows.
Windows crashes are restricted to 64-bit builds:
https://crash-stats.mozilla.com/report/list?signature=nsAccessible%3A%3AVisibilityState%28%29
Comment 6 David Bolter [:davidb] 2012-01-12 07:37:41 PST
OK thanks.

amd64 gives me deja vu, but I don't remember the other crash sig.
Comment 7 Jesse Ruderman 2012-01-12 09:48:07 PST
(In reply to David Bolter [:davidb] from comment #4)
> I'm trying to recreate on windows. Jesse do I just keep pressing the button
> that opens another tab and closes it? (I have the fuzzer extension)

Just once. If you don't hit the bug the first time, you'll have to restart Firefox before trying again, because "enabling accessibility" is permanent for the process.
Comment 8 Scoobidiver (away) 2012-01-20 09:20:52 PST
It also happens now on 32-bit builds.
Comment 9 David Bolter [:davidb] 2012-01-20 09:33:26 PST
Robert, given the stacks linked in comment 2, do you think this is a bad frame situation (how)?
Comment 10 Robert O'Callahan (:roc) (email my personal email if necessary) 2012-01-20 12:14:36 PST
No, I don't think it is.

VisibilityState() has this code:

  nsAccessible* accessible = this;
  do {
    // We don't want background tab page content to be aggressively invisible.
    // Otherwise this foils screen reader virtual buffer caches.
    roles::Role role = accessible->Role();
    if (role == roles::PROPERTYPAGE || role == roles::PANE)
      break;

    nsIFrame* frame = accessible->GetFrame();
    if (!frame)
      return vstates;

    const nsIView* view = frame->GetView();
    if (view && view->GetVisibility() == nsViewVisibility_kHide)
      return vstates;
    
  } while (accessible = accessible->Parent());

  nsIFrame* frame = GetFrame();
  ...
  const nsSize frameSize = frame->GetSize();

So if 'this' has role PROPERTYPAGE or PANE, we exit the loop immediately, and then if the accessible has no frame, we crash. That's probably your bug.
Comment 11 David Bolter [:davidb] 2012-01-20 12:16:22 PST
Doh!
Comment 12 David Bolter [:davidb] 2012-01-20 12:29:59 PST
Created attachment 590300 [details] [diff] [review]
frame check
Comment 13 David Bolter [:davidb] 2012-01-20 12:43:52 PST
Comment on attachment 590300 [details] [diff] [review]
frame check

Robert, we have an IsDefunct check in nsAccessible::State (earlier in the call stack) and at some point after that the frame for the propertypage/pane goes bad it seems.

Do you know where we might be causing frame mutation?

(If it has to do with view's that might go away with Timonthy's work)
Comment 14 Robert O'Callahan (:roc) (email my personal email if necessary) 2012-01-20 23:36:16 PST
(In reply to David Bolter [:davidb] from comment #13)
> Robert, we have an IsDefunct check in nsAccessible::State (earlier in the
> call stack) and at some point after that the frame for the propertypage/pane
> goes bad it seems.

IsDefunct only checks for null mContent. It's totally possible to have an mContent but no frame.
Comment 15 alexander :surkov 2012-01-21 00:21:18 PST
(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #14)
> (In reply to David Bolter [:davidb] from comment #13)
> > Robert, we have an IsDefunct check in nsAccessible::State (earlier in the
> > call stack) and at some point after that the frame for the propertypage/pane
> > goes bad it seems.
> 
> IsDefunct only checks for null mContent. It's totally possible to have an
> mContent but no frame.

definitely, the crash happens when a11y events are fired, in case of hide event we are almost guaranteed there's no frame for event target.
Comment 16 Trevor Saunders (:tbsaunde) 2012-01-21 10:17:30 PST
(In reply to alexander :surkov from comment #15)
> (In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #14)
> > (In reply to David Bolter [:davidb] from comment #13)
> > > Robert, we have an IsDefunct check in nsAccessible::State (earlier in the
> > > call stack) and at some point after that the frame for the propertypage/pane
> > > goes bad it seems.
> > 
> > IsDefunct only checks for null mContent. It's totally possible to have an
> > mContent but no frame.

but, mContent not being null implies we haven't shut the accessible down.  Which is actually the case until after the event is fired which I seem to have forgotten and is a little concerning.

> definitely, the crash happens when a11y events are fired, in case of hide
> event we are almost guaranteed there's no frame for event target.
I wonder why the atk object doesn't already exist if this is a hide event.but if its a hide event why where we trying to create an  atk object?
Comment 17 alexander :surkov 2012-01-21 11:01:34 PST
(In reply to Trevor Saunders (:tbsaunde) from comment #16)

> > definitely, the crash happens when a11y events are fired, in case of hide
> > event we are almost guaranteed there's no frame for event target.
> I wonder why the atk object doesn't already exist if this is a hide
> event.but if its a hide event why where we trying to create an  atk object?

good question, that shouldn't happen. since we have a test case then I guess David as assignee could look what happens here. or someone else.
Comment 18 David Bolter [:davidb] 2012-01-21 11:30:35 PST
STR never worked for me.
Comment 19 David Bolter [:davidb] 2012-01-23 07:39:39 PST
(In reply to alexander :surkov from comment #15)
> (In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #14)
> > (In reply to David Bolter [:davidb] from comment #13)
> > > Robert, we have an IsDefunct check in nsAccessible::State (earlier in the
> > > call stack) and at some point after that the frame for the propertypage/pane
> > > goes bad it seems.
> > 
> > IsDefunct only checks for null mContent. It's totally possible to have an
> > mContent but no frame.
> 
> definitely, the crash happens when a11y events are fired, in case of hide
> event we are almost guaranteed there's no frame for event target.

Indeed.

I think this patch should land, and I plan to land it shortly. Investigation can still continue by someone who can recreate (as time permits).
Comment 20 David Bolter [:davidb] 2012-01-23 07:42:58 PST
(In reply to alexander :surkov from comment #17)
> (In reply to Trevor Saunders (:tbsaunde) from comment #16)
> 
> > > definitely, the crash happens when a11y events are fired, in case of hide
> > > event we are almost guaranteed there's no frame for event target.
> > I wonder why the atk object doesn't already exist if this is a hide
> > event.but if its a hide event why where we trying to create an  atk object?
> 
> good question, that shouldn't happen. since we have a test case then I guess
> David as assignee could look what happens here. or someone else.

I don't understand the question. I think I need more details.
Comment 22 Marco Bonardo [::mak] 2012-01-24 04:47:12 PST
https://hg.mozilla.org/mozilla-central/rev/778cf04d7c55

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