717505 - Crash [@ nsAccessible::VisibilityState] when closing a tab, depending on when accessibility was enabled

Status: RESOLVED FIXED

(Core :: Disability Access APIs, defect)

Description

[Jesse Ruderman]

Attachment: 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]

Attachment: stack trace

Comment 2

[Scoobidiver (away)]

It first appeared in 12.0a1/20120111.
The regression range is:
http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=c713003d3226&tochange=e79ef0ffcb09

More reports at:
https://crash-stats.mozilla.com/report/list?signature=nsAccessible%3A%3AVisibilityState()
https://crash-stats.mozilla.com/report/list?signature=nsAccessible%3A%3AVisibilityState

Comment 3

[Marco Zehe (:MarcoZ)]

I haven't seen this crash myself on Windows using NVDA 2011.3.

Comment 4

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

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)]

(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] (NeedInfo me for attention)]

OK thanks.

amd64 gives me deja vu, but I don't remember the other crash sig.

Comment 7

[Jesse Ruderman]

(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)]

It also happens now on 32-bit builds.

Comment 9

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

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)]

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] (NeedInfo me for attention)]

Doh!

Comment 12

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

Attachment: frame check

Comment 13

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

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)]

(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 (:asurkov)]

(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)]

(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 (:asurkov)]

(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] (NeedInfo me for attention)]

STR never worked for me.

Comment 19

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

(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] (NeedInfo me for attention)]

(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 21

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

https://hg.mozilla.org/integration/mozilla-inbound/rev/778cf04d7c55

Comment 22

[Marco Bonardo [:mak]]

https://hg.mozilla.org/mozilla-central/rev/778cf04d7c55