Closed Bug 1745024 Opened 8 months ago Closed 3 months ago

Initializing the accessibility service leaves vSync enabled

Categories

(Core :: Graphics, defect, P4)

defect

Tracking

()

RESOLVED FIXED
102 Branch
Tracking Status
firefox102 --- fixed

People

(Reporter: florian, Assigned: florian)

References

Details

Attachments

(2 files)

My try runs for bug 1742842 show that tests in the accessible/tests/browser/ folder leave vSync enabled at the end of the test.

I tried to produce a reduced testcase, and it turns out that this line in the common.js file is what keeps vSync enabled forever:

var gAccService = Cc["@mozilla.org/accessibilityService;1"].getService(
  nsIAccessibilityService
);

Wild. I'm looking at some vsync bugs and I'll investigate this one also.

Assignee: nobody → bwerth
Severity: -- → S4
Priority: -- → P4

Here is a browser-chrome mochitest to easily reproduce the issue.

I captured a profile of it: https://share.firefox.dev/3M892pY (./mach test browser/base/content/test/performance/browser_vsync_accessibility.js --profiler), and it reveals that the content process has an "Accessibility notifications [Display]" RefreshObserver, added with this stack:

(root) []
start [dyld]
0x694 (in plugin-container) []
XRE_InitChildProcess []
XRE_InitChildProcess(int, char**, XREChildData const*) [XUL]
MessageLoop::Run() [XUL]
XRE_RunAppShell() [XUL]
nsAppShell::Run() [XUL]
nsBaseAppShell::Run() [XUL]
MessageLoop::Run() [XUL]
mozilla::ipc::MessagePump::Run(base::MessagePump::Delegate*) [XUL]
NS_ProcessNextEvent(nsIThread*, bool) [XUL]
nsThread::ProcessNextEvent(bool, bool*) [XUL]
mozilla::detail::RunnableFunction<mozilla::TaskController::InitializeInternal()::$_1>::Run() [XUL]
mozilla::TaskController::ProcessPendingMTTask(bool) [XUL]
mozilla::TaskController::ExecuteNextTaskOnlyMainThreadInternal(mozilla::detail::BaseAutoLock<mozilla::Mutex&> const&) [XUL]
Task PContent::Msg_ActivateA11y []
mozilla::TaskController::DoExecuteNextTaskOnlyMainThreadInternal(mozilla::detail::BaseAutoLock<mozilla::Mutex&> const&) [XUL]
mozilla::RunnableTask::Run() [XUL]
mozilla::ipc::MessageChannel::MessageTask::Run() [XUL]
mozilla::ipc::MessageChannel::DispatchMessage(mozilla::ipc::ActorLifecycleProxy*, IPC::Message&&) [XUL]
mozilla::ipc::MessageChannel::DispatchAsyncMessage(mozilla::ipc::ActorLifecycleProxy*, IPC::Message const&) [XUL]
PContent::Msg_ActivateA11y []
mozilla::dom::PContentChild::OnMessageReceived(IPC::Message const&) [XUL]
mozilla::dom::ContentChild::RecvActivateA11y(unsigned int const&, unsigned int const&) [XUL]
GetOrCreateAccService(unsigned int) [XUL]
nsAccessibilityService::Init() [XUL]
mozilla::a11y::ApplicationAccessible::Init() [XUL]
mozilla::a11y::DocManager::CreateDocOrRootAccessible(mozilla::dom::Document*) [XUL]
mozilla::a11y::DocAccessible::Init() [XUL]
mozilla::a11y::NotificationController::NotificationController(mozilla::a11y::DocAccessible*, mozilla::PresShell*) [XUL]
mozilla::PresShell::AddRefreshObserver(nsARefreshObserver*, mozilla::FlushType, char const*) [XUL]
nsRefreshDriver::AddRefreshObserver(nsARefreshObserver*, mozilla::FlushType, char const*) [XUL]

Does this help?

Flags: needinfo?(bwerth)

I've done a bit of debugging. There is code to remove the refresh observer at https://searchfox.org/mozilla-central/rev/d34f9713ae128a3138c2b70d8041a535f1049d19/accessible/base/NotificationController.cpp#989, but it doesn't run, because mDocument->HasLoadState(DocAccessible::eCompletelyLoaded) returns false. The value of mLoadState is 1 (eTreeConstructed).

Reading the code around, https://searchfox.org/mozilla-central/rev/d34f9713ae128a3138c2b70d8041a535f1049d19/accessible/generic/DocAccessible.cpp#392-394 reminded me bug 1748466. Changing the test if (mDocumentNode->GetReadyStateEnum() == dom::Document::READYSTATE_COMPLETE) to add || mDocumentNode->IsInitialDocument() similar to what was done in bug 1748466 makes the test I attached in comment 2 pass.

I don't know if this is the right fix, but it could be.

(In reply to Florian Quèze [:florian] from comment #3)

I don't know if this is the right fix, but it could be.

Hopefully try and reviewers will know :-).

Florian, thanks for staying on this. I haven't advanced this at all.

Assignee: bwerth → florian
Flags: needinfo?(bwerth)

Jamie, is this review still on your radar?

Flags: needinfo?(jteh)

Thanks for the ping. I'm so sorry. This somehow totally slipped off my radar. 😳

Flags: needinfo?(jteh)
Pushed by fqueze@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/a30460ae6d16
avoid keeping an 'Accessibility notifications' refresh observer for initial about:blank documents, r=Jamie.
Status: NEW → RESOLVED
Closed: 3 months ago
Resolution: --- → FIXED
Target Milestone: --- → 102 Branch
You need to log in before you can comment on or make changes to this bug.