1410482 - enable AOM for privileged content

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, enhancement, P2)

Comment 1

[alexander :surkov (:asurkov)]

Olli, who's a good candidate to review? You're not accepting those.

Comment 2

[alexander :surkov (:asurkov)]

Attachment: patch

Comment 3

[Olli Pettay [:smaug][[e-mail]]]

I could review next week, if that is ok.

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

Comment 5

[alexander :surkov (:asurkov)]

(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?

Comment 6

[alexander :surkov (:asurkov)]

Attachment: patch

address Olli's comments

Comment 7

[alexander :surkov (:asurkov)]

Attachment: aomenable

apparently attached a wrong patch, fixed

Comment 8

[Olli Pettay [:smaug][[e-mail]]]

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.

Comment 9

[alexander :surkov (:asurkov)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/2602e10160a18c0b998c749d8aa2005fc8ef6614
Bug 1410482 - enable AOM for privileged content, r=smaug

Comment 10

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

Backed out for leaks in devtools tests:

https://hg.mozilla.org/integration/mozilla-inbound/rev/9fddd9d18e1d1d879598b1a1e42e15cc17783cee

Push with failures: https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=2602e10160a18c0b998c749d8aa2005fc8ef6614&filter-resultStatus=usercancel&filter-resultStatus=runnable&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-resultStatus=retry
Different push which also ran asan failure: https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=d84ea38d6fd086a627f0321be28103be88fad175&filter-resultStatus=usercancel&filter-resultStatus=runnable&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-resultStatus=retry

Failure log debug: https://treeherder.mozilla.org/logviewer.html#?job_id=138962123&repo=mozilla-inbound
> TEST-UNEXPECTED-FAIL | leakcheck | tab process: 407504 bytes leaked (APZEventState, ActiveElementManager, AsyncLatencyLogger, BackstagePass, CSPService, ...) 

Failure log asan: https://treeherder.mozilla.org/logviewer.html#?job_id=138978815&repo=mozilla-inbound

Comment 11

[alexander :surkov (:asurkov)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=3da3938bd45e68c28223347f90ff966ae3a6d959

Comment 12

[alexander :surkov (:asurkov)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=176084f0c3c14385eea5b74367bf5b29f297f57a

Comment 13

[alexander :surkov (:asurkov)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=89c6b186172dfc2da9ddefce1820dd3f4c8ea421

Comment 14

[alexander :surkov (:asurkov)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=149e2ff284b896d7290be023094f31839ddb562b

Comment 15

[alexander :surkov (:asurkov)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=e75f2d399cb33c6ba48b22c4c2cc083f73f66b22

Comment 16

[alexander :surkov (:asurkov)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=97b770874efa7467456fd6d42f6f72ed813f43f6

Comment 17

[alexander :surkov (:asurkov)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=b07c454db70c2c92f0c37f79b8a2f02f488358c8

Comment 18

[alexander :surkov (:asurkov)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=fc7d066462148bb27dc43b23fcbd32b9ddaf9467

Comment 19

[alexander :surkov (:asurkov)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=a239468c22aba9149a265d0f874dbae8d188ba92

Comment 20

[alexander :surkov (:asurkov)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=58d6c9954c58d2f04915086df15dc4f8cdd423ff

Comment 21

[alexander :surkov (:asurkov)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=4b4096f2cea3988d56f7ef3db51070bca7b3813a

Comment 22

[alexander :surkov (:asurkov)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=d9040fa9918d99652e42ddf5eab7336b14d842d1

Comment 23

[alexander :surkov (:asurkov)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=6001d9445fb87cf25de09ce0f562ceb66c5f3a7e

Comment 24

[alexander :surkov (:asurkov)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=c10e3ac4fc80b9921193ba2684d35a6038383bde

Comment 25

[alexander :surkov (:asurkov)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=1c81188d5618066332ba623fd3c26ebbc859790d

Comment 26

[alexander :surkov (:asurkov)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=38dde762e3eb24c04821c97a4db3d256747f8136

Comment 27

[alexander :surkov (:asurkov)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=ffe007c4201fdc8236bf0124150677b193d63f1e

Comment 28

[alexander :surkov (:asurkov)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=34317d2fdf9c9954aca9af303fb33d0e653ec457

Comment 29

[alexander :surkov (:asurkov)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=de1d447e08c8e692b80f9ac3ded7079839396ee8

Comment 30

[alexander :surkov (:asurkov)]

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

Comment 31

[Olli Pettay [:smaug][[e-mail]]]

Does someone perhaps create the service after xpcom-shutdown?

Comment 32

[alexander :surkov (:asurkov)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=ac7671a7d94f0f34100cd598cc5f1a0ed360693e

Comment 33

[alexander :surkov (:asurkov)]

(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

Comment 34

[alexander :surkov (:asurkov)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=aae396ae04d4a3b20585b2da6e2fc3222bd91868

Comment 35

[alexander :surkov (:asurkov)]

Olli, any other advice how to approach it, or maybe names of who was working in this area and might have ideas?

Comment 36

[Olli Pettay [:smaug][[e-mail]]]

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?

Comment 37

[alexander :surkov (:asurkov)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=7c7ebc2d27025c49d1ed561bb6eca9cec04b4509

Comment 38

[alexander :surkov (:asurkov)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=bcfe9658d02097f64e8efc9ab5247e6c45325c32

Comment 39

[alexander :surkov (:asurkov)]

seems passing now, not sure what was a fix, will land it

Comment 40

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/8589b95008dd
enable AOM for privileged content, r=smaug

Comment 41

[Stefan Hindli [:stefan_hindli]]

https://hg.mozilla.org/mozilla-central/rev/8589b95008dd