Closed Bug 1410482 Opened 7 years ago Closed 6 years ago

enable AOM for privileged content

Categories

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

Product:
Core
Component:
DOM: Core & HTML
Type:
enhancement

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla62
Tracking Flags:
Tracking Status
firefox62 --- fixed

People

(Reporter: surkov, Assigned: surkov)

References

Details

Attachments

(1 file, 2 obsolete files)

aomenable
3.29 KB, patch
smaug
: review+
Details | Diff | Splinter Review 
      No description provided.
Olli, who's a good candidate to review? You're not accepting those.
Flags: needinfo?(bugs)
Attached 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)

    
    

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

    
    

    
  

      
      
      
      

        Attached patch
          
          
        patch (obsolete)
        — 
          — Splinter Review
      

    


  
address Olli's comments

        Attachment #8920653 -
        Attachment is obsolete: true

        Attachment #8921044 -
        Flags: feedback?(bugs)

    
    

    
  

      
      
      
      

        Attached patch
          
          
        aomenable
        — 
          — Splinter 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

    
    

    
  
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)

    
    

    
  
(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

    
    

    
  
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)

    
    

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

    
    

    
  
https://hg.mozilla.org/mozilla-central/rev/8589b95008dd
Status: NEW → RESOLVED
Closed: 6 years ago

          status-firefox62:
          --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla62
Component: DOM → DOM: Core & HTML

          You need to log in
          before you can comment on or make changes to this bug.
        






  

    
  







  

    

      
Attachment

      

      
      
      
      
    

    

      

        

          

            
General

            

              Creator: 



            

            
Created: 

            
Updated: 

            
Size: