Closed
Bug 975065
Opened 10 years ago
Closed 10 years ago
implement Text accessible text range methods
Categories
(Core :: Disability Access APIs, defect)
Core
Disability Access APIs
Tracking
()
RESOLVED
FIXED
mozilla31
People
(Reporter: surkov, Assigned: surkov)
References
(Blocks 1 open bug)
Details
Attachments
(1 file, 1 obsolete file)
12.29 KB,
patch
|
tbsaunde
:
review+
|
Details | Diff | Splinter Review |
No description provided.
Assignee | ||
Comment 1•10 years ago
|
||
So we need to add TextRange objects into the array what requires blocking bug 982212 fixed. As I understand there are certain technical problems and bug 982212 may be not fixed in nearest future. Trev suggests to add a copy constructor for TextRange class. I lean towards that copy constructor approach will be a bad design and reference counting for TextRange would be preferable. Neil, can I ask you to take a look at this class and give your opinion please?
Flags: needinfo?(neil)
Comment 2•10 years ago
|
||
If you want to avoid the three pairs of addrefs and releases, then you could define a move assignment operator and write mTextRangeArray.AppendElement() = Move(aTextRange); If you don't want to do that then my preference would be for the copy constructor over refcounting for TextRange; there'd be a significant penalty to separately allocating all of your TextRange objects when only a few of them would need to be copied.
Flags: needinfo?(neil)
Assignee | ||
Comment 3•10 years ago
|
||
(In reply to neil@parkwaycc.co.uk from comment #2) > If you want to avoid the three pairs of addrefs and releases, then you could > define a move assignment operator and write > mTextRangeArray.AppendElement() = Move(aTextRange); in that case I still need a copy constructor, right?
Assignee | ||
Comment 4•10 years ago
|
||
(In reply to alexander :surkov from comment #3) > (In reply to neil@parkwaycc.co.uk from comment #2) > > If you want to avoid the three pairs of addrefs and releases, then you could > > define a move assignment operator and write > > mTextRangeArray.AppendElement() = Move(aTextRange); > > in that case I still need a copy constructor, right? got it, that must be nicer than copy constructor approach
Assignee | ||
Comment 5•10 years ago
|
||
there's another problem, since TextRange keeps strong references to accessible objects and cycle collector is not aware of TextRange so it destroys accessible objects no taking into account TextRange objects. I'm curious if cycle collection stuff can be implemented with no ref counting.
Assignee | ||
Comment 6•10 years ago
|
||
Olli, you probably can advise something about cycle collection here
Comment 7•10 years ago
|
||
Hmm, what is the setup here? If TextRange (or the array containing them) is some temporary object which keeps strong references to cycle collectable object, there isn't anything to do. But if TextRange objects stay alive longer, they should be traversed/unlinked.
Assignee | ||
Comment 8•10 years ago
|
||
(In reply to Olli Pettay [:smaug] from comment #7) > Hmm, what is the setup here? If TextRange (or the array containing them) is > some temporary object which keeps strong references to cycle collectable > object, there isn't anything to do. > But if TextRange objects stay alive longer, they should be > traversed/unlinked. TextRange may stay longer as part of its XPCOM wrapper (xpcTextRange class), I guess that class should implement cycle collecting (and then any class that is supposed to keep TextRange for a while).
Comment 9•10 years ago
|
||
What is the setup here? http://mxr.mozilla.org/mozilla-central/source/accessible/src/base/TextRange.h#25 is not refcounted. What keeps it alive in case of xpc?
Assignee | ||
Comment 10•10 years ago
|
||
it's kept by http://mxr.mozilla.org/mozilla-central/source/accessible/src/xpcom/xpcAccessibleTextRange.h#32
Assignee | ||
Comment 11•10 years ago
|
||
in my example if was a mochitest that kept xpcom text range alive
Comment 12•10 years ago
|
||
Aha, it has TextRange as a member variable. Looks like TextRange should be refcounted object and cycle collectable, and never used TextRange mFoo; member variable, or TextRange as a stack variable.
Assignee | ||
Comment 13•10 years ago
|
||
I'm not sure I got second part of your comment. But the idea was: internal TextRange that impelments all logic xpcTextRange that implements XPCOM interface and aggregates internal TextRange uiaTextRange that impelments MS COM interface and aggregates internal TextRange
Comment 14•10 years ago
|
||
(In reply to alexander :surkov from comment #5) > there's another problem, since TextRange keeps strong references to > accessible objects and cycle collector is not aware of TextRange so it > destroys accessible objects no taking into account TextRange objects. I'm > curious if cycle collection stuff can be implemented with no ref counting. I don't think there's any macros for it, but its perfectly possible to do, you probably just have to more or less manually implement Traverse and Unlink. As far as the actual cycle collecting goes dealing with class foo { nsRefPtr<T> x, y, z; }; is the same as dealing with class foo1 { nsRefPtr<T> x, y, z; }; class foo { foo1 mFoo; }; given there memory layout is the same. (In reply to Olli Pettay [:smaug] from comment #12) > Aha, it has TextRange as a member variable. > Looks like TextRange should be refcounted object and cycle collectable, and > never used > TextRange mFoo; member variable, or TextRange as a stack variable. or we can always use it as a member of something else that either doesn't need to be cc'd (e.g. stuff !gecko owns) or have the cc'd object its a member of traverse / unlink it after all conceptually its the same as a couple of refptr members. Now that has the downside that afaik we don't have things to make that super easy, but it shouldn't be super hard either, and maybe we can do some horrible template thing and make it nice.
Assignee | ||
Comment 15•10 years ago
|
||
(In reply to Trevor Saunders (:tbsaunde) from comment #14) > (In reply to alexander :surkov from comment #5) > > there's another problem, since TextRange keeps strong references to > > accessible objects and cycle collector is not aware of TextRange so it > > destroys accessible objects no taking into account TextRange objects. I'm > > curious if cycle collection stuff can be implemented with no ref counting. > > I don't think there's any macros for it, but its perfectly possible to do, > you probably just have to more or less manually implement Traverse and > Unlink. if you volunteer for that then it's perfectly fine with me
Assignee | ||
Comment 16•10 years ago
|
||
I tried to implement cycle collection on xpc TextRange object but it crashes now on destruction time of internal TextRange. Are there any ideas?
Comment 17•10 years ago
|
||
What is the stack trace?
Assignee | ||
Comment 18•10 years ago
|
||
#0 0x000000010319928e in ~nsRefPtr at /Users/alex/mozilla/0411/obj-ff-dbg/widget/cocoa/../../dist/include/nsAutoPtr.h:894 #1 0x0000000103197065 in ~nsRefPtr at /Users/alex/mozilla/0411/obj-ff-dbg/widget/cocoa/../../dist/include/nsAutoPtr.h:892 #2 0x0000000104c0a5d5 in ~TextRange at /Users/alex/mozilla/0411/accessible/src/generic/../base/TextRange.h:22 #3 0x0000000104c03435 in ~TextRange at /Users/alex/mozilla/0411/accessible/src/generic/../base/TextRange.h:22 #4 0x0000000104c2c02c in ~xpcAccessibleTextRange at /Users/alex/mozilla/0411/accessible/src/xpcom/xpcAccessibleTextRange.h:20 #5 0x0000000104c2afe5 in ~xpcAccessibleTextRange at /Users/alex/mozilla/0411/accessible/src/xpcom/xpcAccessibleTextRange.h:20 #6 0x0000000104c29cea in mozilla::a11y::xpcAccessibleTextRange::DeleteCycleCollectable() at /Users/alex/mozilla/0411/accessible/src/xpcom/xpcAccessibleTextRange.cpp:28 #7 0x0000000104c2b141 in mozilla::a11y::xpcAccessibleTextRange::cycleCollection::DeleteCycleCollectable(void*) at /Users/alex/mozilla/0411/accessible/src/xpcom/xpcAccessibleTextRange.h:24 #8 0x0000000101602f9f in ~SnowWhiteKiller at /Users/alex/mozilla/0411/xpcom/base/nsCycleCollector.cpp:2386 #9 0x00000001015ec725 in ~SnowWhiteKiller at /Users/alex/mozilla/0411/xpcom/base/nsCycleCollector.cpp:2379 #10 0x00000001015d764d in nsCycleCollector::FreeSnowWhite(bool) at /Users/alex/mozilla/0411/xpcom/base/nsCycleCollector.cpp:2550 #11 0x00000001015da6bc in nsCycleCollector::BeginCollection(ccType, nsICycleCollectorListener*) at /Users/alex/mozilla/0411/xpcom/base/nsCycleCollector.cpp:3426 #12 0x00000001015da145 in nsCycleCollector::Collect(ccType, js::SliceBudget&, nsICycleCollectorListener*) at /Users/alex/mozilla/0411/xpcom/base/nsCycleCollector.cpp:3288 #13 0x00000001015dc158 in nsCycleCollector_collect(nsICycleCollectorListener*) at /Users/alex/mozilla/0411/xpcom/base/nsCycleCollector.cpp:3841 #14 0x00000001035049de in nsJSContext::CycleCollectNow(nsICycleCollectorListener*, int) at /Users/alex/mozilla/0411/dom/base/nsJSEnvironment.cpp:1936 #15 0x000000010348a36f in nsDOMWindowUtils::GarbageCollect(nsICycleCollectorListener*, int) at /Users/alex/mozilla/0411/dom/base/nsDOMWindowUtils.cpp:1470 #16 0x00000001016bc7ab in NS_InvokeByIndex at /Users/alex/mozilla/0411/xpcom/reflect/xptcall/src/md/unix/xptcinvoke_x86_64_unix.cpp:162
Comment 19•10 years ago
|
||
I have no idea what Move() does to refcounted stuff. Why do you use Move() and not swap which is usually used with nsCOM/RefPtr
Comment 20•10 years ago
|
||
(In reply to alexander :surkov from comment #16) > Created attachment 8401981 [details] [diff] [review] > patch > > I tried to implement cycle collection on xpc TextRange object but it crashes > now on destruction time of internal TextRange. Are there any ideas? for me it asserts because the refcount became negative, but the patch appears reasonable so I'll investigate it.
Comment 21•10 years ago
|
||
ok, so I just spent the afternoon discover somebody is a nub, this crash has nothing to do with cycle collection. If you look at xpcAccessbielTextRange::GetEndContainer() you see more or less *aEndContainer = mRange.mEndContainer which obviously doesn't AddRef the thing mRange.mEndContainer points to like it should. GetStartContainer() suffers from the same bug.
Assignee | ||
Comment 22•10 years ago
|
||
impl is somehow not ready and even broken but it's still good to have as structure fill (I need to extend TextRange interface to understand how best to implement it)
Assignee: nobody → surkov.alexander
Attachment #8401981 -
Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #8403512 -
Flags: review?(trev.saunders)
Assignee | ||
Comment 23•10 years ago
|
||
Trev, plans for review?
Assignee | ||
Updated•10 years ago
|
Flags: needinfo?(trev.saunders)
Comment 24•10 years ago
|
||
Comment on attachment 8403512 [details] [diff] [review] patch2 >+TextRange::Set(HyperTextAccessible* aRoot, why not just use operator =? > >+ TextRange& operator= (TextRange&& aRange) >+ { >+ mRoot = Move(aRange.mRoot); >+ mStartContainer = Move(aRange.mStartContainer); >+ mEndContainer = Move(aRange.mEndContainer); technically using swap() might be better till 980753 is fixed, but whatever. > void > HyperTextAccessible::EnclosingRange(a11y::TextRange& aRange) const > { >+ if (IsTextField()) { >+ aRange.Set(mDoc, const_cast<HyperTextAccessible*>(this), 0, >+ const_cast<HyperTextAccessible*>(this), ChildCount()); >+ } else { >+ aRange.Set(mDoc, mDoc, 0, mDoc, mDoc->ChildCount()); this not a text field case doesn't seem to make much sense. > HyperTextAccessible::SelectionRanges(nsTArray<a11y::TextRange>* aRanges) const > { >+ aRanges->Clear(); can't have anything in it so just assert its empty >+ ErrorResult rv; >+ for (int32_t idx = 0; idx < sel->RangeCount(); idx++) { hoist the call to RangeCount() and it returns uin32_t so use that for idx too. >+ nsRange* DOMRange = sel->GetRangeAt(idx, rv); why not use the version that doesn't take a ErrorResult? > HyperTextAccessible::RangeByChild(Accessible* aChild, > a11y::TextRange& aRange) const > { >+ aRange.Set(mDoc, aChild, 0, aChild, aChild->ChildCount()); why mDoc not this? > HyperTextAccessible::RangeAtPoint(int32_t aX, int32_t aY, > a11y::TextRange& aRange) const > { >+ Accessible* child = mDoc->ChildAtPoint(aX, aY, eDeepestChild); >+ if (child) >+ aRange.Set(mDoc, child, 0, child, child->ChildCount()); same >+// nsISupports and cycle collection >+ >+NS_IMPL_CYCLE_COLLECTION_3(xpcAccessibleTextRange, >+ mRange.mRoot, >+ mRange.mStartContainer, >+ mRange.mEndContainer) have you actually seen this leak? the only way I can see a cycle of strong refs existing involves someone setting a property on a dom node to point at a text range. >+NS_INTERFACE_MAP_BEGIN_CYCLE_COLLECTION(xpcAccessibleTextRange) >+ NS_INTERFACE_MAP_ENTRY(nsIAccessibleTextRange) >+ NS_INTERFACE_MAP_ENTRY_AMBIGUOUS(nsISupports, nsIAccessibleTextRange) why is it ambiguous? >+++ b/accessible/tests/mochitest/moz.build >@@ -21,16 +21,17 @@ DIRS += [ > 'role', > 'scroll', > 'selectable', > 'states', > 'table', > 'text', > 'textattrs', > 'textcaret', >+ 'textrange', just add textrange/a11y.ini to the next list then get rid of textrange/moz.build?
Attachment #8403512 -
Flags: review?(trev.saunders) → review+
Comment 25•10 years ago
|
||
(In reply to alexander :surkov from comment #23) > Trev, plans for review? was on pto ;)
Flags: needinfo?(trev.saunders)
Assignee | ||
Comment 26•10 years ago
|
||
(In reply to Trevor Saunders (:tbsaunde) from comment #25) > (In reply to alexander :surkov from comment #23) > > Trev, plans for review? > > was on pto ;) got it, I missed that somehow
Assignee | ||
Comment 27•10 years ago
|
||
(In reply to Trevor Saunders (:tbsaunde) from comment #24) > >+TextRange::Set(HyperTextAccessible* aRoot, > > why not just use operator =? what object should it take? > > void > > HyperTextAccessible::EnclosingRange(a11y::TextRange& aRange) const > > { > >+ if (IsTextField()) { > >+ aRange.Set(mDoc, const_cast<HyperTextAccessible*>(this), 0, > >+ const_cast<HyperTextAccessible*>(this), ChildCount()); > >+ } else { > >+ aRange.Set(mDoc, mDoc, 0, mDoc, mDoc->ChildCount()); > > this not a text field case doesn't seem to make much sense. I follow IE impl here, i.e. textpattern is implemented by documents and text fields. > > HyperTextAccessible::RangeByChild(Accessible* aChild, > > a11y::TextRange& aRange) const > > { > >+ aRange.Set(mDoc, aChild, 0, aChild, aChild->ChildCount()); > > why mDoc not this? same > >+NS_IMPL_CYCLE_COLLECTION_3(xpcAccessibleTextRange, > >+ mRange.mRoot, > >+ mRange.mStartContainer, > >+ mRange.mEndContainer) > > have you actually seen this leak? the only way I can see a cycle of strong > refs existing involves someone setting a property on a dom node to point at > a text range. iirc that was a cycle collection assertion > > >+NS_INTERFACE_MAP_BEGIN_CYCLE_COLLECTION(xpcAccessibleTextRange) > >+ NS_INTERFACE_MAP_ENTRY(nsIAccessibleTextRange) > >+ NS_INTERFACE_MAP_ENTRY_AMBIGUOUS(nsISupports, nsIAccessibleTextRange) > > why is it ambiguous? just copied/pasted, I believe they didn't have an explicit reason to be ambiguous as well. > >+++ b/accessible/tests/mochitest/moz.build > >@@ -21,16 +21,17 @@ DIRS += [ > > 'role', > > 'scroll', > > 'selectable', > > 'states', > > 'table', > > 'text', > > 'textattrs', > > 'textcaret', > >+ 'textrange', > > just add textrange/a11y.ini to the next list then get rid of > textrange/moz.build? I don't mind, why do we have two options here?
Comment 28•10 years ago
|
||
(In reply to alexander :surkov from comment #27) > (In reply to Trevor Saunders (:tbsaunde) from comment #24) > > >+TextRange::Set(HyperTextAccessible* aRoot, > > > > why not just use operator =? > > what object should it take? just create a temporary? or maybe just make this a wrapper around operator = or the reverse if x = Move(TextRange(blah)) is too ugly > > > void > > > HyperTextAccessible::EnclosingRange(a11y::TextRange& aRange) const > > > { > > >+ if (IsTextField()) { > > >+ aRange.Set(mDoc, const_cast<HyperTextAccessible*>(this), 0, > > >+ const_cast<HyperTextAccessible*>(this), ChildCount()); > > >+ } else { > > >+ aRange.Set(mDoc, mDoc, 0, mDoc, mDoc->ChildCount()); > > > > this not a text field case doesn't seem to make much sense. > > I follow IE impl here, i.e. textpattern is implemented by documents and text > fields. why not other things that are editable like inputs? and why does TextPattern whatever it is matter here? > > >+NS_IMPL_CYCLE_COLLECTION_3(xpcAccessibleTextRange, > > >+ mRange.mRoot, > > >+ mRange.mStartContainer, > > >+ mRange.mEndContainer) > > > > have you actually seen this leak? the only way I can see a cycle of strong > > refs existing involves someone setting a property on a dom node to point at > > a text range. > > iirc that was a cycle collection assertion I don't see how that could make any sense, but I guess it doesn't matter. > > >+++ b/accessible/tests/mochitest/moz.build > > >@@ -21,16 +21,17 @@ DIRS += [ > > > 'role', > > > 'scroll', > > > 'selectable', > > > 'states', > > > 'table', > > > 'text', > > > 'textattrs', > > > 'textcaret', > > >+ 'textrange', > > > > just add textrange/a11y.ini to the next list then get rid of > > textrange/moz.build? > > I don't mind, why do we have two options here? not really sure, though it seems like it would make more sense to remove the one involving more indirection.
Assignee | ||
Comment 29•10 years ago
|
||
(In reply to Trevor Saunders (:tbsaunde) from comment #28) > (In reply to alexander :surkov from comment #27) > > (In reply to Trevor Saunders (:tbsaunde) from comment #24) > > > >+TextRange::Set(HyperTextAccessible* aRoot, > > > > > > why not just use operator =? > > > > what object should it take? > > just create a temporary? or maybe just make this a wrapper around operator = > or the reverse if x = Move(TextRange(blah)) is too ugly I think I like Set even if = tricks are not slower. > > > this not a text field case doesn't seem to make much sense. > > > > I follow IE impl here, i.e. textpattern is implemented by documents and text > > fields. > > why not other things that are editable like inputs? I don't know. That's IE.
Assignee | ||
Comment 30•10 years ago
|
||
http://hg.mozilla.org/integration/mozilla-inbound/rev/28765a60c8be
Comment 31•10 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/28765a60c8be
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla31
You need to log in
before you can comment on or make changes to this bug.
Description
•