Ensure that eXPCOM consumer flag remains when trying to shut down a11y service.

RESOLVED FIXED in Firefox 60

Status

()

enhancement
RESOLVED FIXED
Last year
Last year

People

(Reporter: yzen, Assigned: yzen)

Tracking

({access})

Trunk
mozilla60
Points:
---

Firefox Tracking Flags

(firefox60 fixed)

Details

Attachments

(1 attachment, 1 obsolete attachment)

Assignee

Description

Last year
Right now there are cases where the flag gets reset even though the service is still alive. This problem was exposed by crashes in TV (test verify) tests from bug 1428430.
Assignee

Comment 1

Last year
Posted patch 1443871 patch (obsolete) — Splinter Review
Attachment #8956941 - Flags: review?(surkov.alexander)
Comment on attachment 8956941 [details] [diff] [review]
1443871 patch

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

::: accessible/base/nsAccessibilityService.cpp
@@ +1811,5 @@
>        xpcAccessibilityService::IsInUse() ||
>        accService->HasXPCDocuments()) {
> +    // Still used by XPCOM, ensure that the flag is set.
> +    if (!(nsAccessibilityService::gConsumers & nsAccessibilityService::eXPCOM)) {
> +      accService->SetConsumers(nsAccessibilityService::eXPCOM, false);

looks like a dirty hack :) extra comment for that?

::: accessible/base/nsAccessibilityService.h
@@ +316,5 @@
>  
>    /**
>     * Set accessibility service consumers.
>     */
> +  void SetConsumers(uint32_t aConsumers, bool aNotify = true);

I would prefer to use enum here for better code readability, but it's fine assuming this patch is a temporary fix and later we will figure out why we may have eXPOM flag unset, while xpcAccService is alive

@@ +321,5 @@
>  
>    /**
>     * Unset accessibility service consumers.
>     */
> +  void UnsetConsumers(uint32_t aConsumers, bool aNotify = true);

you don't have any callers that needs aNotify=false, so you don't need this arg

::: accessible/xpcom/xpcAccessibilityService.cpp
@@ +49,5 @@
>    if (mRefCnt > 1) {
> +    if (mShutdownTimer) {
> +      mShutdownTimer->Cancel();
> +      mShutdownTimer = nullptr;
> +    }

does it have to go into this patch as well? or that thing should be/can be landed separately?
Assignee

Comment 3

Last year
Attachment #8956941 - Attachment is obsolete: true
Attachment #8956941 - Flags: review?(surkov.alexander)
Attachment #8957219 - Flags: review?(surkov.alexander)
Comment on attachment 8957219 [details] [diff] [review]
1443871 patch v2

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

::: accessible/base/nsAccessibilityService.cpp
@@ +1808,5 @@
>    if (nsCoreUtils::AccEventObserversExist() ||
>        xpcAccessibilityService::IsInUse() ||
>        accService->HasXPCDocuments()) {
>      // Still used by XPCOM
> +    if (!(nsAccessibilityService::gConsumers & nsAccessibilityService::eXPCOM)) {

technically this condition affects nothing, so you could remove it
Attachment #8957219 - Flags: review?(surkov.alexander) → review+

Comment 5

Last year
Pushed by yura.zenevich@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/9ed1661d198e
ensure eXPCOM consumer flag is always set if the service still exists. r=surkov

Comment 6

Last year
bugherder
https://hg.mozilla.org/mozilla-central/rev/9ed1661d198e
Status: ASSIGNED → RESOLVED
Closed: Last year
Resolution: --- → FIXED
Target Milestone: --- → mozilla60
You need to log in before you can comment on or make changes to this bug.