Closed Bug 1122846 Opened 9 years ago Closed 9 years ago

aria-owns attribute causes Firefox to hang.

Categories

(Core :: Disability Access APIs, defect)

Product:
Core
Component:
Disability Access APIs
Version:
35 Branch
Platform:
x86_64
Windows 8.1
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla38

People

(Reporter: AmazingJaze, Assigned: surkov)

References

(Blocks 1 open bug)

Details

Attachments

(3 files)

Testcase from comment#0
240 bytes, text/html
Details
patch
9.26 KB, patch
Details | Diff | Splinter Review
patch
9.33 KB, patch
davidb
: review+
Details | Diff | Splinter Review 
User Agent: Mozilla/5.0 (Windows NT 6.3; WOW64; Trident/7.0; Touch; .NET4.0E; .NET4.0C; .NET CLR 3.5.30729; .NET CLR 2.0.50727; .NET CLR 3.0.30729; Tablet PC 2.0; InfoPath.3; rv:11.0) like Gecko

Steps to reproduce:

1) Create a simple HTML page with the following markup and open it in the firefox browser.

<!doctype html>
<html>
<head>
    <title>HTML Template</title>
</head>
<body>
    <div id="testdiv" role="menu">
        <button role="menuitem" aria-owns="testdiv">
            crash
        </button>
    </div>
</body>
</html>

2) Click on the button labeled "crash", or just press the TAB key on the keyboard


Actual results:

Firefox hangs and is permanently unresponsive. 

If you try to debug the problem with F12, the F12 debugger does not throw any "caught" or "uncaught" exceptions and also becomes permanently unresponsive.


Expected results:

Firefox does not lock up. Other browsers (Chrome and IE) do not have this problem. Firefox appears to be caught in an inifinite loop from the memory usage in windows task manager.
I do not see a hang or a crash with Firefox35 or Firefox38 (nightly) on windows7

Reporter: 
Do you see the issue with the testcase link in this bug report ?
Can you please also test in the Firefox safemode ?
- https://support.mozilla.org/en-US/kb/troubleshoot-firefox-issues-using-safe-mode
Flags: needinfo?(AmazingJaze)
I do see the issue with the test case link and in Firefox safe mode. I am running Windows 8.1 Update 1 and the repro is 100% for me across 3 different machines.
Flags: needinfo?(AmazingJaze)
You should just experience a hang, no crash, after clicking the button. The entire window becomes unresponsive for me.
Component: Untriaged → Disability Access APIs
Product: Firefox → Core
This is INVALID markup: http://www.w3.org/TR/wai-aria/states_and_properties#aria-owns
"Authors SHOULD NOT use aria-owns as a replacement for the DOM hierarchy. If the relationship is represented in the DOM, do not use aria-owns."

You are creating the button as a child of the div. Referencing the div via aria-owns in this context is illegal, since you are already representing the DOM hierarchy properly.

However, having said that, we should probably still not freeze. Alex, is there a way we can protect against this authoring error?
Blocks: aria
Flags: needinfo?(surkov.alexander)
Status: UNCONFIRMED → NEW
Ever confirmed: true
Flags: needinfo?(surkov.alexander)
Attached patch patch — — Splinter Review 
Assignee: nobody → surkov.alexander
Status: NEW → ASSIGNED

        Attachment #8551353 -
        Flags: review?(dbolter)

    
    

    
  
Comment on attachment 8551353 [details] [diff] [review]
patch

>+ARIAOwnedByIterator::Next()
>+{

I'm not convinced its a good idea to kind of support multiple things claiming to own an object like this.

>+class ARIAOwnedByIterator : public RelatedAccIterator {

MOZ_FINAL

>+  ARIAOwnedByIterator();
>+  ARIAOwnedByIterator(const ARIAOwnedByIterator&);
>+  ARIAOwnedByIterator& operator = (const ARIAOwnedByIterator&);

= delete

>+class ARIAOwnsIterator : public AccIterable {

MOZ_FINAL

why not inherit IDRefIterator?

>+public:
>+  ARIAOwnsIterator(Accessible* aOwner);

explicit

>+private:
>+  ARIAOwnsIterator();
>+  ARIAOwnsIterator(const ARIAOwnsIterator&);
>+  ARIAOwnsIterator& operator = (const ARIAOwnsIterator&);

= delete

>+  Accessible* mOwner;

it can be const Accessible* const right?

"checkbox5");
>       testRelation("descr3", RELATION_DESCRIPTION_FOR, "checkbox5");
>       testRelation("checkbox5", RELATION_DESCRIBED_BY, ["descr2", "descr3"]);
> 
>       // aria_owns, multiple relations
>       testRelation("treeitem1", RELATION_NODE_CHILD_OF, "tree");
>       testRelation("treeitem2", RELATION_NODE_CHILD_OF, "tree");
> 
>+      // aria-owns, bad relations
>+      testRelation("ariaowns_container", RELATION_NODE_CHILD_OF, null);
>+      testRelation("ariaowns_self", RELATION_NODE_CHILD_OF, null);
>+      testRelation("ariaowns_sibling", RELATION_NODE_CHILD_OF, "ariaowns_self");
>+
>+      testRelation("ariaowns_container", RELATION_NODE_PARENT_OF, null);
>+      testRelation("ariaowns_self", RELATION_NODE_PARENT_OF, "ariaowns_sibling");
>+      testRelation("ariaowns_sibling", RELATION_NODE_PARENT_OF, null);
>+
>       // 'node child of' relation for outlineitem role
>       testRelation("treeitem3", RELATION_NODE_CHILD_OF, "tree");
>       testRelation("treeitem4", RELATION_NODE_CHILD_OF, "tree");
>       testRelation("treeitem5", RELATION_NODE_CHILD_OF, "treeitem4");
>       testRelation("treeitem6", RELATION_NODE_CHILD_OF, "tree");
>       testRelation("treeitem7", RELATION_NODE_CHILD_OF, "treeitem6");
>       testRelation("tree2_ti1", RELATION_NODE_CHILD_OF, "tree2");
>       testRelation("tree2_ti1a", RELATION_NODE_CHILD_OF, "tree2_ti1");
>@@ -305,16 +314,22 @@
>     <div role="treeitem" id="treeitem4" aria-level="1">Green</div>
>     <div role="treeitem" id="treeitem5" aria-level="2">Light green</div>
>     <div role="treeitem" id="treeitem6" aria-level="1">Green2</div>
>     <div role="group">
>       <div role="treeitem" id="treeitem7">Super light green</div>
>     </div>
>   </div>
> 
>+  <div id="ariaowns_container">
>+    <div id="ariaowns_self"
>+         aria-owns="aria_ownscontainer ariaowns_self ariaowns_sibling"></div>
>+  </div>
>+  <div id="ariaowns_sibling"></div>
>+
>   <div aria-owns="simplegrid-ownrow" role="grid" id="simplegrid">
>     <div role="row" id="simplegrid-row1" aria-level="1">
>       <div role="gridcell">cell 1,1</div>
>       <div role="gridcell">cell 1,2</div>
>     </div>
>     <div role="row" id="simplegrid-row2" aria-level="1">
>       <div role="gridcell">cell 2,1</div>
>       <div role="gridcell">cell 2,2</div>

    
    

    
  
Comment on attachment 8551353 [details] [diff] [review]
patch

Review of attachment 8551353 [details] [diff] [review]:
-----------------------------------------------------------------

Could you please describe the solution with a few sentences?

::: accessible/tests/mochitest/relations/test_general.html
@@ +322,5 @@
> +  <div id="ariaowns_container">
> +    <div id="ariaowns_self"
> +         aria-owns="aria_ownscontainer ariaowns_self ariaowns_sibling"></div>
> +  </div>
> +  <div id="ariaowns_sibling"></div>

It isn't strictly a sibling is it? (More like an uncle)

    
    

    
  
(In reply to David Bolter [:davidb] from comment #8)

> Could you please describe the solution with a few sentences?

don't let aria-owns to point to the parent

    
    

    
  

      
      
      
      

        Attached patch
          
          
        patch
        — 
          — Splinter Review
      

    


  
with nits fixed

        Attachment #8551881 -
        Flags: review?(dbolter)

    
    

    
  
Comment on attachment 8551881 [details] [diff] [review]
patch

Review of attachment 8551881 [details] [diff] [review]:
-----------------------------------------------------------------

r=me since this fixes the issue and does add any multi-owner support than we already had.

::: accessible/base/AccIterator.h
@@ +284,5 @@
> +  ARIAOwnsIterator(const ARIAOwnsIterator&) = delete;
> +  ARIAOwnsIterator& operator = (const ARIAOwnsIterator&) = delete;
> +
> +  IDRefsIterator mIter;
> +  Accessible* mOwner;

nit: Trevor mentioned making this const.

        Attachment #8551881 -
        Flags: review?(dbolter) → review+

    
    

    
  
(In reply to David Bolter [:davidb] from comment #11)

> > +  IDRefsIterator mIter;
> > +  Accessible* mOwner;
> 
> nit: Trevor mentioned making this const.

those are initialized by 'this' from non const methods

    
    

    
  
ignore it

    
    

    
  
(In reply to Trevor Saunders (:tbsaunde) from comment #7)

> why not inherit IDRefIterator?

because I don't want to reveal other methods from IDRefIterator

    
    

    
  
(In reply to alexander :surkov from comment #14)
> (In reply to Trevor Saunders (:tbsaunde) from comment #7)
> 
> > why not inherit IDRefIterator?
> 
> because I don't want to reveal other methods from IDRefIterator

protected / private inheritance is a thing, but I guess it doesn't really matter.

    
    

    
  
Comment on attachment 8551353 [details] [diff] [review]
patch

Review of attachment 8551353 [details] [diff] [review]:
-----------------------------------------------------------------

Could you please describe the solution with a few sentences?

::: accessible/tests/mochitest/relations/test_general.html
@@ +322,5 @@
> +  <div id="ariaowns_container">
> +    <div id="ariaowns_self"
> +         aria-owns="aria_ownscontainer ariaowns_self ariaowns_sibling"></div>
> +  </div>
> +  <div id="ariaowns_sibling"></div>

It isn't strictly a sibling is it? (More like an uncle)

        Attachment #8551353 -
        Flags: review?(dbolter)

    
    

    
  
https://hg.mozilla.org/mozilla-central/rev/084810ea6887
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla38

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






  

    
  







  

    

      
Attachment

      

      
      
      
      
    

    

      

        

          

            
General

            

              Creator: 



            

            
Created: 

            
Updated: 

            
Size: