1363447 - make ApplicationAccessible::Shutdown() call AccessibleWrap::Shutdown()

Status: NEW

(Core :: Disability Access APIs, enhancement, P3)

Description

[Trevor Saunders (:tbsaunde)]

The ApplicationAccessible can get shut down by the XPCOM-SHUTDOWN
  notification while the platform still has references to it and can still call
  methods on it.  That means that if we spin the event loop at all after the
  accessibility xpcom shutdown observer runs (which can happen) then the
  platform can currently call methods on a shutdown application accessible.
  The result of that is assertions that ApplicationAccessible::mAppInfo is
  null.  We can deal with this by having ApplicationAccessible::Shutdown() call
  AccessibleWrap::Shutdown() so the application accessible gets marked as
  shutdown.  That means that when the platform tries to call methods on it
  after it has been shut down the generic logic to avoid calling methods on
  defunct accessibles will kick in and we will bail out before calling methods
  on the shut down application accessible.

  One case where the event loop can spin after a11y recieves the xpcom shutdown
  notification is that the web workers xpcom shutdown observer can run after
  the a11y one.  The web worker observer ends up notifying observers of its own
  shutdown, and one of the observers of that notification is AsyncShutdown
  which will spin the event loop during the call to its observer for the
  web workers shutdown message.

Comment 1

[Trevor Saunders (:tbsaunde)]

Attachment: make ApplicationAccessible::Shutdown() call AccessibleWrap::Shutdown()

unfortunately I'm not really sure how to write a test that actually covers this issue well other than one that sets up its own observer which seems like more work than its probably worth.

Comment 2

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

Comment on attachment 8865950 [details] [diff] [review]
make ApplicationAccessible::Shutdown() call AccessibleWrap::Shutdown()

Review of attachment 8865950 [details] [diff] [review]:
-----------------------------------------------------------------

Frightening. I'd sort of prefer comment 1 over this commit message. Can we have a code comment too?

Comment 3

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

Any advice for next steps on this bug?

Comment 4

[alexander :surkov (:asurkov)]

(In reply to David Bolter [:davidb] (NeedInfo me for attention) from comment #3)
> Any advice for next steps on this bug?

I'm sort of confused: the patch is r+ by you. You don't want to take the patch anymore or you want to incorporate some changes into it? What kind of advice do you look for?

Comment 5

[Eitan Isaacson [:eeejay]]

I would like to see a test that exhibits the problem we are fixing. Otherwise, I don't really have an opinion on this one way or the other.

Comment 6

[BugBot [:suhaib / :marco/ :calixte]]

There's a r+ patch which didn't land and no activity in this bug for 2 weeks.
:tbsaunde, could you have a look please?

Comment 7

[BugBot [:suhaib / :marco/ :calixte]]

Clear a needinfo that is pending on an inactive user.

Inactive users most likely will not respond; if the missing information is essential and cannot be collected another way, the bug maybe should be closed as INCOMPLETE.

For more information, please visit auto_nag documentation.