Closed Bug 1122846 Opened 10 years ago Closed 10 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: 10 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: