Closed Bug 1443871 Opened 2 years ago Closed 2 years ago

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

Categories

(Core :: Disability Access APIs, enhancement)

enhancement
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla60
Tracking Status
firefox60 --- fixed

People

(Reporter: yzen, Assigned: yzen)

Details

(Keywords: access)

Attachments

(1 file, 1 obsolete file)

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.
Attached 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?
Attached patch 1443871 patch v2Splinter Review
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+
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
https://hg.mozilla.org/mozilla-central/rev/9ed1661d198e
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla60
You need to log in before you can comment on or make changes to this bug.