Closed
Bug 673403
Opened 13 years ago
Closed 13 years ago
don't rely on non empty result from nsAccessible::GetParent()
Categories
(Core :: Disability Access APIs, defect)
Core
Disability Access APIs
Tracking
()
RESOLVED
FIXED
mozilla8
People
(Reporter: surkov, Assigned: tbsaunde)
References
(Blocks 1 open bug)
Details
(Keywords: access)
Attachments
(1 file, 3 obsolete files)
54.45 KB,
patch
|
Details | Diff | Splinter Review |
1) while you're here rename GetParent() to Parent() to correspond name conversion, that has side effect that guarantees us no one call is missed and makes reviewer work simpler 2) null check for everything that doesn't belong to children traversal
Reporter | ||
Comment 1•13 years ago
|
||
this bug leads to crashes like this (get object attributes when hide event is handled):
xul.dll!nsAccessible::GetLevelInternal() Line 3426 C++
xul.dll!nsAccessible::GroupPosition(int * aGroupLevel, int * aSimilarItemsInGroup, int * aPositionInGroup) Line 1506 C++
xul.dll!nsAccessible::GetAttributes(nsIPersistentProperties * * aAttributes) Line 1340 C++
xul.dll!nsAccessibleWrap::get_attributes(wchar_t * * aAttributes) Line 1455 C++
> AT stack trace
user32.dll!___ClientCallWinEventProc@4() + 0x2a bytes
ntdll.dll!_KiUserCallbackDispatcher@12() + 0x2e bytes
user32.dll!_NtUserNotifyWinEvent@16() + 0xc bytes
xul.dll!nsAccessibleWrap::NotifyWinEvent(unsigned long event, HWND__ * hwnd, long idObjectType, long idObject) Line 188 + 0x10 bytes C++
xul.dll!nsAccessibleWrap::FirePlatformEvent(AccEvent * aEvent) Line 1602 C++
xul.dll!nsAccessibleWrap::HandleAccEvent(AccEvent * aEvent) Line 1537 + 0xe bytes C++
xul.dll!nsHyperTextAccessibleWrap::HandleAccEvent(AccEvent * aEvent) Line 77 + 0x8 bytes C++
xul.dll!nsSVGCircleElement::AddRef() Line 93 + 0xc bytes C++
xul.dll!nsCOMPtr_base::assign_with_AddRef(nsISupports * rawPtr) Line 90 C++
xul.dll!nsEventShell::FireEvent(AccEvent * aEvent) Line 65 C++
xul.dll!nsDocAccessible::ProcessPendingEvent(AccEvent * aEvent) Line 1746 C++
xul.dll!NotificationController::WillRefresh(mozilla::TimeStamp aTime) Line 306 C++
Assignee | ||
Comment 2•13 years ago
|
||
I'll end up bit rotting this a little, but most of it should be fine.
Attachment #547906 -
Flags: review?(surkov.alexander)
Reporter | ||
Comment 3•13 years ago
|
||
Comment on attachment 547906 [details] [diff] [review] pass 1 Review of attachment 547906 [details] [diff] [review]: ----------------------------------------------------------------- r=me with nits fixed ::: accessible/src/atk/nsAccessibleWrap.cpp @@ +831,5 @@ > if (!accWrap) { > return nsnull; > } > > + nsAccessible* accParent = accWrap->Parent(); didn't you considered to fix indentation here? ::: accessible/src/base/TextUpdater.cpp @@ +73,2 @@ > if (!parent) { > NS_ERROR("No parent for text leaf!"); it's time to remove warning because if text update triggers and then node is removed from DOM, then we get this case. Sounds right? ::: accessible/src/base/nsARIAGridAccessible.cpp @@ +829,5 @@ > // siblings cells. > if (role == nsIAccessibleRole::ROLE_GRID_CELL || > role == nsIAccessibleRole::ROLE_ROWHEADER || > role == nsIAccessibleRole::ROLE_COLUMNHEADER) { > + nsAccessible *row = aAccessible->Parent(); nsAccessible* row @@ +1095,5 @@ > > if (IsDefunct()) > return NS_ERROR_FAILURE; > > + nsAccessible *row = Parent(); nsAccessible* row ::: accessible/src/base/nsAccUtils.cpp @@ +107,5 @@ > if (role == nsIAccessibleRole::ROLE_OUTLINEITEM) > return 1; > > if (role == nsIAccessibleRole::ROLE_ROW) { > + nsAccessible *parent = aAccessible->Parent(); nsAccessible* parent ::: accessible/src/base/nsAccessible.cpp @@ +2143,5 @@ > if (view) { > nsIScrollableFrame *scrollFrame = do_QueryFrame(frame); > + nsAccessible* parent = Parent(); > + if (scrollFrame || view->GetWidget() || !frame->GetParent() > + && parent) this is wrong, no accessible parent should mean there's no node_child_of relation in parent chain I think. Operators shouldn't be on line start @@ +3294,5 @@ > // between parent listitem and nested list. > > // Calculate 'level' attribute based on number of parent listitems. > level = 0; > +nsAccessible* parent = nsnull; fix indentation please and maybe it's worth to keep existing code, that allows you do if (parent && level == 0) @@ +3317,5 @@ > for (PRInt32 siblingIdx = 0; siblingIdx < siblingCount; siblingIdx++) { > nsAccessible* sibling = parent->GetChildAt(siblingIdx); > > + nsAccessible* siblingChild = > + sibling->GetChildAt(sibling->GetChildCount() - 1); could you add LastChild and FirstChild to nsAccessible, sounds like nice to have @@ +3319,5 @@ > > + nsAccessible* siblingChild = > + sibling->GetChildAt(sibling->GetChildCount() - 1); > + if (siblingChild > + && siblingChild->Role() == nsIAccessibleRole::ROLE_LIST) operator to the end of line ::: accessible/src/base/nsBaseWidgetAccessible.cpp @@ +238,2 @@ > if (walkUpAcc && walkUpAcc->Role() == nsIAccessibleRole::ROLE_LINK && > walkUpAcc->State() & states::LINKED) { while you are here, please remove walkUpAcc from if statement ::: accessible/src/base/nsDocAccessible.cpp @@ +1499,5 @@ > // this reorder event is processed by parent document then events targeted to > // this document may be fired prior to this reorder event. If this is > // a problem then consider to keep event processing per tab document. > + nsAccessible* parent = Parent(); > + if (!IsRoot() && parent) { no need to check, if you like then you can assert. NotifyOfInitialUpdate can't be called for document is not in subtree ::: accessible/src/base/nsRootAccessible.cpp @@ +698,5 @@ > // It is a top level menuitem. Only fire a focus event when the menu bar > // is active. > return; > } else { > + nsAccessible *containerAccessible = accessible->Parent(); nsAccessible* container @@ +705,5 @@ > // It is not top level menuitem > // Only fire focus event if it is not inside collapsed popup > // and not a listitem of a combo box > if (containerAccessible->State() & states::COLLAPSED) { > + nsAccessible *containerParent = containerAccessible->Parent(); nsAccessible* containerParent @@ +795,5 @@ > > // If ancestor chain of accessibles is not completely visible, > // don't use this one. This happens for example if it's inside > // a background tab (tabbed browsing) > + nsAccessible *parent = accDoc->Parent(); nsAccessible* parent ::: accessible/src/html/nsHTMLFormControlAccessible.cpp @@ +407,5 @@ > // This means we're part of another control, so use parent accessible for name. > // This ensures that a textbox inside of a XUL widget gets > // an accessible name. > + nsAccessible* parent = Parent(); > + NS_ENSURE_TRUE(parent, NS_ERROR_FAILURE); no failure, just if (parent) parent->GetName( ::: accessible/src/html/nsHTMLSelectAccessible.cpp @@ +386,3 @@ > NS_ASSERTION(parent, "No parent!"); > + if (!parent) > + return NS_ERROR_FAILURE; no assertion and NS_OK please ::: accessible/src/html/nsHyperTextAccessible.cpp @@ +622,5 @@ > > // From the descendant, go up and get the immediate child of this hypertext > nsAccessible *childAccAtOffset = nsnull; > while (descendantAcc) { > + nsAccessible *parentAcc = descendantAcc->Parent(); nsAccessible* parent = ::: accessible/src/xul/nsXULFormControlAccessible.cpp @@ +577,1 @@ > NS_ENSURE_TRUE(parent,); if (!parent) return NS_OK; ::: accessible/src/xul/nsXULMenuAccessible.cpp @@ +331,5 @@ > item->GetSelected(&isSelected); > > // Is collapsed? > PRBool isCollapsed = PR_FALSE; > + nsAccessible* parentAcc = Parent(); maybe rename it to parent while you're here @@ +341,5 @@ > > // Selected and collapsed? > if (isCollapsed) { > // Set selected option offscreen/invisible according to combobox state > + nsAccessible* grandParentAcc = parentAcc->Parent(); to grandParent @@ +346,1 @@ > NS_ENSURE_TRUE(grandParentAcc, state); if (!grandParent) return NS_OK @@ +643,1 @@ > NS_ENSURE_TRUE(parent, state); no warning please ::: accessible/src/xul/nsXULTreeGridAccessible.cpp @@ +1015,5 @@ > > if (IsDefunct()) > return NS_OK; > > + CallQueryInterface(mParent->Parent(), aTable); CallQueryInterface crashes on null @@ +1170,5 @@ > if (IsDefunct()) > return NS_ERROR_FAILURE; > > // "table-cell-index" attribute > + nsAccessible* acc = mParent->Parent(); name it grandParent please @@ +1171,5 @@ > return NS_ERROR_FAILURE; > > // "table-cell-index" attribute > + nsAccessible* acc = mParent->Parent(); > + NS_ENSURE_TRUE(acc, NS_ERROR_FAILURE); no warning please @@ +1172,5 @@ > > // "table-cell-index" attribute > + nsAccessible* acc = mParent->Parent(); > + NS_ENSURE_TRUE(acc, NS_ERROR_FAILURE); > + nsCOMPtr<nsIAccessibleTable> tableAccessible = do_QueryInterface(static_cast<nsIAccessible*>(acc)); if 80 chars restriction is broken then please put do_Query on second line
Attachment #547906 -
Flags: review?(surkov.alexander) → review+
Assignee | ||
Comment 4•13 years ago
|
||
> > // Calculate 'level' attribute based on number of parent listitems. > > level = 0; > > +nsAccessible* parent = nsnull; > > fix indentation please > and maybe it's worth to keep existing code, that allows you do > if (parent && level == 0) hoestly I think I like this better, isn't it closer to the normal style of declaring variables where you use them? > > @@ +3317,5 @@ > > for (PRInt32 siblingIdx = 0; siblingIdx < siblingCount; siblingIdx++) { > > nsAccessible* sibling = parent->GetChildAt(siblingIdx); > > > > + nsAccessible* siblingChild = > > + sibling->GetChildAt(sibling->GetChildCount() - 1); > > could you add LastChild and FirstChild to nsAccessible, sounds like nice to > have add both and leave first child unused, or just add last child for now?
Reporter | ||
Comment 5•13 years ago
|
||
(In reply to comment #4) > > > // Calculate 'level' attribute based on number of parent listitems. > > > level = 0; > > > +nsAccessible* parent = nsnull; > > > > fix indentation please > > and maybe it's worth to keep existing code, that allows you do > > if (parent && level == 0) > > hoestly I think I like this better, isn't it closer to the normal style of > declaring variables where you use them? ok, maybe it's worth to check IsBoundToParent() in the beginning (after getting ARIA level). I just don't like that "parent = Parent(); if (!parent) return 0;" > > could you add LastChild and FirstChild to nsAccessible, sounds like nice to > > have > > add both and leave first child unused, or just add last child for now? add both, I think, we have many places where we use GetChildAt(0) so potentially we have lot of usecases, we can start to use as we go.
Assignee | ||
Comment 6•13 years ago
|
||
Attachment #547906 -
Attachment is obsolete: true
Reporter | ||
Comment 7•13 years ago
|
||
Comment on attachment 548162 [details] [diff] [review] patch Review of attachment 548162 [details] [diff] [review]: ----------------------------------------------------------------- ::: accessible/src/atk/nsAccessibleWrap.cpp @@ +826,5 @@ > AtkObject * > getParentCB(AtkObject *aAtkObj) > { > + if (!aAtkObj->accessible_parent) { > + nsAccessibleWrap *accWrap = GetAccessibleWrap(aAtkObj); nit: nsAccessibleWrap* accWrap @@ +837,2 @@ > > + AtkObject *parent = nsAccessibleWrap::GetAtkObject(accParent); AtkObject* parent ::: accessible/src/base/nsAccessible.cpp @@ +3307,3 @@ > } > > + if (level == 0 && (parent = Parent())) { this is sort of crazy style, why not though. But didn't you like to check parent after PRInt32 level = nsAccUtils::GetDefaultLevel(this); and get back to previous code? So all you do here (except renames) is just add if (!IsBoundToParent()) return; @@ +3313,5 @@ > for (PRInt32 siblingIdx = 0; siblingIdx < siblingCount; siblingIdx++) { > nsAccessible* sibling = parent->GetChildAt(siblingIdx); > > + nsAccessible* siblingChild = > + sibling->GetChildAt(sibling->GetChildCount() - 1); so you decided to not introduce Last/FirstChild methods for this? ::: accessible/src/xul/nsXULTreeGridAccessible.cpp @@ +1175,5 @@ > > // "table-cell-index" attribute > + nsAccessible* grandParent = mParent->Parent(); > + if (!grandParent) > + return NS_ERROR_FAILURE; NS_OK
Attachment #548162 -
Flags: review+
Assignee | ||
Comment 8•13 years ago
|
||
> > ::: accessible/src/base/nsAccessible.cpp > @@ +3307,3 @@ > > } > > > > + if (level == 0 && (parent = Parent())) { > > this is sort of crazy style, why not though. But didn't you like to check > parent after PRInt32 level = nsAccUtils::GetDefaultLevel(this); and get back > to previous code? So all you do here (except renames) is just add I think I'd like to keep the change to nsAccessible* parent = this; while (parent = parent->Parent()) { parent->DoSomething(); } but I wouldn't have any issues with just assigning to parent in the if and testing IsBoundToParent() at the beginning. > > if (!IsBoundToParent()) > return; > > @@ +3313,5 @@ > > for (PRInt32 siblingIdx = 0; siblingIdx < siblingCount; siblingIdx++) { > > nsAccessible* sibling = parent->GetChildAt(siblingIdx); > > > > + nsAccessible* siblingChild = > > + sibling->GetChildAt(sibling->GetChildCount() - 1); > > so you decided to not introduce Last/FirstChild methods for this? tbh I forgot, I'll fix >
Reporter | ||
Comment 9•13 years ago
|
||
(In reply to comment #8) > I think I'd like to keep the change to > nsAccessible* parent = this; > while (parent = parent->Parent()) { > parent->DoSomething(); > } > > but I wouldn't have any issues with just assigning to parent in the if and > testing IsBoundToParent() at the beginning. ok, up to you
Assignee | ||
Comment 10•13 years ago
|
||
final cleanup (hopefully)
Reporter | ||
Comment 11•13 years ago
|
||
Comment on attachment 548192 [details] [diff] [review] patch Review of attachment 548192 [details] [diff] [review]: ----------------------------------------------------------------- r=me with fixed ::: accessible/src/base/nsAccessible.cpp @@ +2142,5 @@ > nsIView *view = frame->GetViewExternal(); > if (view) { > nsIScrollableFrame *scrollFrame = do_QueryFrame(frame); > + if (scrollFrame || view->GetWidget() || !frame->GetParent()) > + return nsRelUtils::AddTarget(aRelationType, aRelation, Parent()); I realize AddTarget has null check but is how does it come with your relation patch? @@ +3277,5 @@ > // its level. > level = 1; > > + nsAccessible* parent = this; > + while ((parent = Parent())) { should fix it too ::: accessible/src/base/nsAccessible.h @@ +306,5 @@ > */ > PRBool HasChildren() { return !!GetChildAt(0); } > > /** > + * Return first / last / next/previous sibling of the accessible. nit: first/last/next, no spaces @@ +312,5 @@ > inline nsAccessible* NextSibling() const > { return GetSiblingAtOffset(1); } > inline nsAccessible* PrevSibling() const > { return GetSiblingAtOffset(-1); } > + inline nsAccessible* FirstChild() const @@ +313,5 @@ > { return GetSiblingAtOffset(1); } > inline nsAccessible* PrevSibling() const > { return GetSiblingAtOffset(-1); } > + inline nsAccessible* FirstChild() > + { return GetChildCount() != 0 ? GetChildAt(0) : nsnull; } two spaces indentation @@ +314,5 @@ > inline nsAccessible* PrevSibling() const > { return GetSiblingAtOffset(-1); } > + inline nsAccessible* FirstChild() > + { return GetChildCount() != 0 ? GetChildAt(0) : nsnull; } > + inline nsAccessible* LastChild() const
Attachment #548192 -
Flags: review+
Reporter | ||
Comment 12•13 years ago
|
||
never mind about const
Assignee | ||
Comment 13•13 years ago
|
||
Attachment #548162 -
Attachment is obsolete: true
Attachment #548192 -
Attachment is obsolete: true
Reporter | ||
Comment 14•13 years ago
|
||
Comment on attachment 548196 [details] [diff] [review] patch to land Review of attachment 548196 [details] [diff] [review]: ----------------------------------------------------------------- few more nits, sorry :) ::: accessible/src/atk/nsAccessibleWrap.cpp @@ +891,5 @@ > if (!accWrap) { > return -1; > } > > + nsAccessible *parent = accWrap->Parent(); nit: nsAccessible* parent please ::: accessible/src/base/nsDocAccessible.cpp @@ +1503,4 @@ > if (!IsRoot()) { > nsRefPtr<AccEvent> reorderEvent = > + new AccEvent(nsIAccessibleEvent::EVENT_REORDER, parent, eAutoDetect, > + AccEvent::eCoalesceFromSameSubtree); why do you need local variable for parent since it's used one time only ::: accessible/src/html/nsHTMLTableAccessible.cpp @@ +1530,5 @@ > > + if (aRelationType == nsIAccessibleRelation::RELATION_LABEL_FOR) { > + nsAccessible* parent = Parent(); > + if (parent) > + return nsRelUtils::AddTarget(aRelationType, aRelation, parent); but here you do Parent() testing (you get rid it for node_child_of relation) ::: accessible/src/html/nsHyperTextAccessible.cpp @@ +625,2 @@ > while (descendantAcc) { > + nsAccessible *parentAcc = descendantAcc->Parent(); nit: nsAccessible* parentAcc ::: accessible/src/mac/nsAccessibleWrap.mm @@ +284,5 @@ > > already_AddRefed<nsIAccessible> > nsAccessibleWrap::GetUnignoredParent() > { > + nsAccessibleWrap *parentWrap = static_cast<nsAccessibleWrap*>(Parent()); nit: nsAccessibleWrap* parentWrap @@ +317,5 @@ > while (parent) { > if (nsAccUtils::MustPrune(parent)) > return PR_TRUE; > > + parent = parent->Parent(); I thought you prefer ;) nsAccessible* parent = this; while ((parent = parent->Parent())
Assignee | ||
Comment 15•13 years ago
|
||
> ::: accessible/src/base/nsDocAccessible.cpp > @@ +1503,4 @@ > > if (!IsRoot()) { > > nsRefPtr<AccEvent> reorderEvent = > > + new AccEvent(nsIAccessibleEvent::EVENT_REORDER, parent, eAutoDetect, > > + AccEvent::eCoalesceFromSameSubtree); > > why do you need local variable for parent since it's used one time only I'd gues its an artifact from checking that it wasn't null, but I haven't checked that. > > ::: accessible/src/html/nsHTMLTableAccessible.cpp > @@ +1530,5 @@ > > > > + if (aRelationType == nsIAccessibleRelation::RELATION_LABEL_FOR) { > > + nsAccessible* parent = Parent(); > > + if (parent) > > + return nsRelUtils::AddTarget(aRelationType, aRelation, parent); > > but here you do Parent() testing (you get rid it for node_child_of relation) I realized it wasn't needed after writing this, I gues I can rm. > > @@ +317,5 @@ > > while (parent) { > > if (nsAccUtils::MustPrune(parent)) > > return PR_TRUE; > > > > + parent = parent->Parent(); > > I thought you prefer ;) > nsAccessible* parent = this; > while ((parent = parent->Parent()) there wasn't much reason to change it and leaving it is easier
Assignee | ||
Comment 16•13 years ago
|
||
landed http://hg.mozilla.org/integration/mozilla-inbound/79bba9b9beb5
Comment 17•13 years ago
|
||
http://hg.mozilla.org/mozilla-central/rev/79bba9b9beb5
Status: NEW → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla8
Comment 18•13 years ago
|
||
(In reply to alexander surkov from comment #0) > 1) while you're here rename GetParent() to Parent() Strange, because I always thought that Gecko convention was that GetParent() could return null but Parent() wasn't allowed to.
Reporter | ||
Comment 19•13 years ago
|
||
(In reply to neil@parkwaycc.co.uk from comment #18) > (In reply to alexander surkov from comment #0) > > 1) while you're here rename GetParent() to Parent() > Strange, because I always thought that Gecko convention was that GetParent() > could return null but Parent() wasn't allowed to. Occasionally I didn't know about this convention. We had a chat (iirc Trevor, Marco and me) where we agreed to remove 'Get' from getter methods. That was a reason I asked here to rename GetParent to Parent. I saw examples in Gecko where both cases are used and I guess I thought that 'Get' methods is sort of old style. While NodeInfo() and GetParent() of nsIContent follows this convention but it appears nsTArray::SafeElementAt doesn't if you pass nsnull as fallback. Is it sort of exception?
Reporter | ||
Updated•13 years ago
|
Assignee: nobody → trev.saunders
Comment 20•13 years ago
|
||
Please consider applying the patch to 7. I am experiencing segfaults because of this in 6 and 7rc5 and 7rc6. They disable me from using an unpatched build (it segfaults when I create a new tab). This is on Ubuntu 10.04, nothing special. This was not the case on 11.04. If you do not want to apply the whole patch (which is ~1k lines of renames and if's) then please at least fix it this way: In accessible/src/html/nsHTMLFormControlAccessible.cpp: in nsHTMLTextFieldAccessible::GetNameInternal(nsAString& aName): change if (mContent->GetBindingParent()) { // XXX: bug 459640 // There's a binding parent. // This means we're part of another control, so use parent accessible for name. // This ensures that a textbox inside of a XUL widget gets // an accessible name. nsAccessible* parent = GetParent(); parent->GetName(aName); } to: if (mContent->GetBindingParent()) { // XXX: bug 459640 // There's a binding parent. // This means we're part of another control, so use parent accessible for name. // This ensures that a textbox inside of a XUL widget gets // an accessible name. nsAccessible* parent = GetParent(); if(parent) parent->GetName(aName); } This is a one-line fix. Hopefully there will be no problem getting it in, even though understandably this comes late. I hope this goes through as it is a very disabling bug... and also (I hunted this down independently from Alexander and Trevor) my first fix for FF! :)
You need to log in
before you can comment on or make changes to this bug.
Description
•