Closed Bug 793316 Opened 7 years ago Closed 7 years ago

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

Categories

(Core :: Disability Access APIs, defect)

All
Linux
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla18

People

(Reporter: tbsaunde, Assigned: tbsaunde)

References

Details

Attachments

(1 file)

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.
Oh, lazy instantiation worked here. Thank you for taking it.
Attached patch patchSplinter Review
Attachment #663636 - Flags: review?(surkov.alexander)
(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 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.
Attachment #663636 - Flags: review?(surkov.alexander) → review+
(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 :)
(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.
Ok, I see. Perhaps I'd move shutdown = false before we set up the application accessible since it looks closer to what we had.
(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.
ok, makes sense
https://hg.mozilla.org/mozilla-central/rev/0daf025d2891
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla18
+  // 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
Blocks: 794243
You need to log in before you can comment on or make changes to this bug.