793316 - mai_util_get_root() called before nsAccessiblityService::Shutdown is set to false

Status: RESOLVED FIXED

(Core :: Disability Access APIs, defect)

Description

[Trevor Saunders (:tbsaunde)]

after bug 767754 we call ApplicationAccessibleWrap::Init() before setting setting sShutdown to false.  THen ApplicationAccessibleWrap::Init() calls atk_bridge_adaptor_init which calls atk_util_get_root() which calls mai_util_get_root() which checks sShutdown and since it thinks accessibility is shutdown returns gail_get_get_root() or just nullptr either way its the wrong thing.

Comment 1

[alexander :surkov (:asurkov)]

Oh, lazy instantiation worked here. Thank you for taking it.

Comment 2

[Trevor Saunders (:tbsaunde)]

Attachment: patch

Comment 3

[Trevor Saunders (:tbsaunde)]

(In reply to alexander :surkov from comment #1)
> Oh, lazy instantiation worked here. Thank you for taking it.

yeah, we were getting lucky.

I took it because I want my browser to work again ;)

Comment 4

[alexander :surkov (:asurkov)]

Comment on attachment 663636 [details] [diff] [review]
patch

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

::: accessible/src/atk/ApplicationAccessibleWrap.cpp
@@ +441,3 @@
>  
> +  // We've shutdown, try to use gail instead
> +  // (to avoid assert in spi_atk_tidy_windows())

I like we don't reenter while we are initializing and I assume a client is ok our root varies (when we are initializing then it is gail rool, when we are initialized then it is app acc).

Please change comment to 'We've shutdown or we are starting up'. It'd be nicer if you use all 80 chars per line.

Comment 5

[alexander :surkov (:asurkov)]

(In reply to Trevor Saunders (:tbsaunde) from comment #3)
> (In reply to alexander :surkov from comment #1)
> > Oh, lazy instantiation worked here. Thank you for taking it.
> 
> yeah, we were getting lucky.
> 
> I took it because I want my browser to work again ;)

selfish interest is engine of the progress too :)

Comment 6

[Trevor Saunders (:tbsaunde)]

(In reply to alexander :surkov from comment #4)
> Comment on attachment 663636 [details] [diff] [review]
> patch
> 
> Review of attachment 663636 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: accessible/src/atk/ApplicationAccessibleWrap.cpp
> @@ +441,3 @@
> >  
> > +  // We've shutdown, try to use gail instead
> > +  // (to avoid assert in spi_atk_tidy_windows())
> 
> I like we don't reenter while we are initializing and I assume a client is
> ok our root varies (when we are initializing then it is gail rool, when we
> are initialized then it is app acc).
> 
> Please change comment to 'We've shutdown or we are starting up'. It'd be
> nicer if you use all 80 chars per line.

actually, this function can't be called before atk/ApplicationAccessibleWrap.cpp:951 at which point we already have an application accessible.

Comment 7

[alexander :surkov (:asurkov)]

Ok, I see. Perhaps I'd move shutdown = false before we set up the application accessible since it looks closer to what we had.

Comment 8

[Trevor Saunders (:tbsaunde)]

(In reply to alexander :surkov from comment #7)
> Ok, I see. Perhaps I'd move shutdown = false before we set up the
> application accessible since it looks closer to what we had.

I thought about that, but it seems riskier since I'd need to make sure nothing else has weird dependancies on the precise ordering of all this stuff.

Comment 9

[alexander :surkov (:asurkov)]

ok, makes sense

Comment 10

[Trevor Saunders (:tbsaunde)]

landed https://hg.mozilla.org/integration/mozilla-inbound/rev/0daf025d2891

Comment 11

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/0daf025d2891

Comment 12

[Ginn Chen]

+  // We've shutdown, try to use gail instead
+  // (to avoid assert in spi_atk_tidy_windows())
+  // XXX tbsaunde then why didn't we replace the gail atk_util impl?

We did replace the gail atk util impl, that's why we're here.
spi_atk_tidy_windows() is in bridge.

It seems we may not need this hack in newer GNOME.
See:
https://bugzilla.gnome.org/show_bug.cgi?id=548559