Closed
Bug 1443871
Opened 6 years ago
Closed 6 years ago
Ensure that eXPCOM consumer flag remains when trying to shut down a11y service.
Categories
(Core :: Disability Access APIs, enhancement)
Core
Disability Access APIs
Tracking
()
RESOLVED
FIXED
mozilla60
Tracking | Status | |
---|---|---|
firefox60 | --- | fixed |
People
(Reporter: yzen, Assigned: yzen)
Details
(Keywords: access)
Attachments
(1 file, 1 obsolete file)
2.79 KB,
patch
|
surkov
:
review+
|
Details | Diff | Splinter Review |
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•6 years ago
|
||
Attachment #8956941 -
Flags: review?(surkov.alexander)
Comment 2•6 years ago
|
||
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•6 years ago
|
||
Attachment #8956941 -
Attachment is obsolete: true
Attachment #8956941 -
Flags: review?(surkov.alexander)
Attachment #8957219 -
Flags: review?(surkov.alexander)
Comment 4•6 years ago
|
||
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
Comment 6•6 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/9ed1661d198e
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla60
You need to log in
before you can comment on or make changes to this bug.
Description
•