1745024 - Initializing the accessibility service leaves vSync enabled

Status: RESOLVED FIXED

(Core :: Graphics, defect, P4)

Description

[Florian Quèze [:florian]]

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
);

Comment 1

[Brad Werth [:bradwerth]]

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

Comment 2

[Florian Quèze [:florian]]

Attachment: potential browser-chrome mochitest

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?

Comment 3

[Florian Quèze [:florian]]

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.

Comment 4

[Florian Quèze [:florian]]

Attachment: Bug 1745024 - avoid keeping an 'Accessibility notifications' refresh observer for initial about:blank documents, r=smaug,morgan.

Comment 5

[Florian Quèze [:florian]]

(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 :-).

Comment 6

[Brad Werth [:bradwerth]]

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

Comment 7

[Florian Quèze [:florian]]

Jamie, is this review still on your radar?

Comment 8

[James Teh [:Jamie]]

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

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/a30460ae6d16
avoid keeping an 'Accessibility notifications' refresh observer for initial about:blank documents, r=Jamie.

Comment 10

[Natalia Csoregi [:nataliaCs]]

https://hg.mozilla.org/mozilla-central/rev/a30460ae6d16