enable AOM for privileged content

RESOLVED FIXED in Firefox 62

Status

()

enhancement
P2
normal
RESOLVED FIXED
2 years ago
2 months ago

People

(Reporter: surkov, Assigned: surkov)

Tracking

(Blocks 1 bug)

unspecified
mozilla62
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox62 fixed)

Details

Attachments

(1 attachment, 2 obsolete attachments)

Assignee

Description

2 years ago
No description provided.
Assignee

Comment 1

2 years ago
Olli, who's a good candidate to review? You're not accepting those.
Flags: needinfo?(bugs)
Assignee

Comment 2

2 years ago
Posted patch patch (obsolete) — Splinter Review
I could review next week, if that is ok.

Could you use AddBoolVarCache and not GetBool. The cache is quite a bit faster.
Flags: needinfo?(bugs)
Assignee

Comment 5

2 years ago
(In reply to Olli Pettay [:smaug] (work week Oct 16-20) from comment #3)
> I could review next week, if that is ok.

sure, thank you.

> Could you use AddBoolVarCache and not GetBool. The cache is quite a bit
> faster.

I will, np, I didn't know it may be a hot path. I guess it's invoked not only once when the browser starts up?
Assignee

Comment 6

2 years ago
Posted patch patch (obsolete) — Splinter Review
address Olli's comments
Attachment #8920653 - Attachment is obsolete: true
Attachment #8921044 - Flags: feedback?(bugs)
Assignee

Comment 7

2 years ago
Posted patch aomenableSplinter Review
apparently attached a wrong patch, fixed
Attachment #8921044 - Attachment is obsolete: true
Attachment #8921044 - Flags: feedback?(bugs)
Attachment #8921045 - Flags: feedback?(bugs)
Comment on attachment 8921045 [details] [diff] [review]
aomenable

[Func="mozilla::dom::AccessibleNode::IsAOMEnabled"] isn't probably needed.
I'd expect [Func="AccessibleNode::IsAOMEnabled"] to work too, assuming AccessibleNode uses namespaces and such in the normal way.
Attachment #8921045 - Flags: feedback?(bugs) → review+
Priority: -- → P2
Assignee

Comment 30

a year ago
AccessibilityService is connected/caused with the leaks. If no GetOrCreateService() is called by AOM object, then no leaks.

Good build:
https://hg.mozilla.org/try/rev/1c81188d5618066332ba623fd3c26ebbc859790d
https://treeherder.mozilla.org/#/jobs?repo=try&revision=1c2e746ebbe929bfbdd836e8a7a9f3b97ecd39a4

Bad build:
https://hg.mozilla.org/try/rev/34317d2fdf9c9954aca9af303fb33d0e653ec457
https://treeherder.mozilla.org/#/jobs?repo=try&revision=1f2347d35a57c2af114f1f4382cdc997b874dfdf&selectedJob=158252012

[1] points out to nsComponentManagerImpl. The interesting thing is the logs has a line of:
Marionette	DEBUG	Received observer notification "xpcom-shutdown"
after leak reports. Since we use "xpcom-shutdown" to shutdown accessibility service, then it makes me wonder if this could be a timing issue.

Olli, do you have any suggestions/advices how to investigate the issue further?

[1] https://treeherder.mozilla.org/logviewer.html#?job_id=158252265&repo=try&lineNumber=166708
Flags: needinfo?(surkov.alexander) → needinfo?(bugs)
Does someone perhaps create the service after xpcom-shutdown?
Flags: needinfo?(bugs)
Assignee

Comment 33

a year ago
(In reply to Olli Pettay [:smaug] from comment #31)
> Does someone perhaps create the service after xpcom-shutdown?

unlikely, I tried to crash the browser [1] if somebody makes attempt to initialize AccessibilityService after XPCOM shutdown. No crash [2].

[1] https://treeherder.mozilla.org/#/jobs?repo=try&revision=a9d29ff0f57ce5ae1959ecdc4fbd440179a39b8b&selectedJob=158534669
[2] https://hg.mozilla.org/try/rev/ac7671a7d94f0f34100cd598cc5f1a0ed360693e
Assignee

Comment 35

a year ago
Olli, any other advice how to approach it, or maybe names of who was working in this area and might have ideas?
Flags: needinfo?(bugs)
hmm, so do we leak AccessibleNodes? What is the most recent link a leak log?

I'm trying to recall the ownership model of AccessibleNodes. Why we aren't cycle collecting its member variables?
Flags: needinfo?(bugs)
Assignee

Comment 39

a year ago
seems passing now, not sure what was a fix, will land it
https://hg.mozilla.org/mozilla-central/rev/8589b95008dd
Status: NEW → RESOLVED
Last Resolved: a year ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla62
Component: DOM → DOM: Core & HTML
Product: Core → Core
You need to log in before you can comment on or make changes to this bug.