Closed Bug 815837 Opened 9 years ago Closed 9 years ago

Investigate if DocAccessible::mDependentIDsHash needs to be Traversed by the cycle collector

Categories

(Core :: Disability Access APIs, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla22

People

(Reporter: mccr8, Assigned: surkov)

Details

Attachments

(1 file, 1 obsolete file)

This contains strong references to nsIContent, which tbsaunde said should always be in the current document. We should at least assert that this holds, and maybe just go ahead and traverse it to be safe.
(In reply to Andrew McCreight [:mccr8] from comment #0)
> This contains strong references to nsIContent, which tbsaunde said should
> always be in the current document.

everything we deal with should be in current document but nevertheless they are cycle collected and *sometimes* I see accessible objects are cycle collected.

> We should at least assert that this
> holds, and maybe just go ahead and traverse it to be safe.

assertion is really nice
Attached patch patch (obsolete) — Splinter Review
just go ahead and traverse them (plus mAnchorContent)
Assignee: nobody → surkov.alexander
Status: NEW → ASSIGNED
Attachment #717779 - Flags: review?(trev.saunders)
Comment on attachment 717779 [details] [diff] [review]
patch

>+CycleCollectorTraverseDepIDsEntry(const nsAString& aKey,
>+                                  DocAccessible::AttrRelProviderArray* aProviders,
>+                                  void* aUserArg)
>+{
>+  nsCycleCollectionTraversalCallback* cb =
>+    static_cast<nsCycleCollectionTraversalCallback*>(aUserArg);
>+
>+  NS_CYCLE_COLLECTION_NOTE_EDGE_NAME(*cb,
>+                                     "dependent ids hash entry of document accessible");

uhm, I think that only makrs the first edge from the loop, shouldn't you put it in the loop? (smaug, mccr8?)

>+
>+  for (uint32_t jdx = 0; jdx < aProviders->Length(); jdx++) {

it seems better to manually hoist Length() out of the loop, I'm not sure the compiler can prove the cc stuff won't effect the array.

also please assert provider->mContent->IsInDoc() preferably fatally because I believe it means a tree update bug.

>+/**
>+ * Traverse the dependent IDs hash table of the document accessible for cycle
>+ * collection.
>+ */
>+inline void
>+ImplCycleCollectionTraverse(nsCycleCollectionTraversalCallback& aCallback,
>+                            DocAccessible::DependentIDsHashtable& aHashtable,
>+                            const char* aName,
>+                            uint32_t aFlags)

I can't imagine how we'll ever have more than the one consumer so just inline it

also, shouldn't you unlink the mDependantIds hashtable?
Attachment #717779 - Flags: review?(trev.saunders) → review-
> uhm, I think that only makrs the first edge from the loop, shouldn't you put it in the loop?

Yeah, I think the NOTE_EDGE needs to be inside the loop.
(In reply to Trevor Saunders (:tbsaunde) from comment #3)

> also, shouldn't you unlink the mDependantIds hashtable?

tmp->mDependentIDsHash.Clear() doesn't work?
Attached patch patch2Splinter Review
Attachment #717779 - Attachment is obsolete: true
Attachment #718873 - Flags: review?(trev.saunders)
(In reply to alexander :surkov from comment #5)
> (In reply to Trevor Saunders (:tbsaunde) from comment #3)
> 
> > also, shouldn't you unlink the mDependantIds hashtable?
> 
> tmp->mDependentIDsHash.Clear() doesn't work?

it should be fine, I just forget we already did that in unlink
Comment on attachment 718873 [details] [diff] [review]
patch2

>+  for (int32_t jdx = aProviders->Length() - 1; jdx >= 0; jdx--) {

there's no reason to iterate backwards here, so please don't.

>+    NS_ASSERTION(provider->mContent->IsInDoc(),
>+                 "Referred content is not in document!");

why not use MOZ_ASSERT to make it fatal?  It seems like if we hit it its a pretty bad bug.
Attachment #718873 - Flags: review?(trev.saunders) → review+
(In reply to Trevor Saunders (:tbsaunde) from comment #8)
> Comment on attachment 718873 [details] [diff] [review]
> patch2
> 
> >+  for (int32_t jdx = aProviders->Length() - 1; jdx >= 0; jdx--) {
> 
> there's no reason to iterate backwards here, so please don't.

local variable saving

> >+    NS_ASSERTION(provider->mContent->IsInDoc(),
> >+                 "Referred content is not in document!");
> 
> why not use MOZ_ASSERT to make it fatal?  It seems like if we hit it its a
> pretty bad bug.

assertions are equally bad imo and they shouldn't happen ever.
(In reply to alexander :surkov from comment #9)
> (In reply to Trevor Saunders (:tbsaunde) from comment #8)
> > Comment on attachment 718873 [details] [diff] [review]
> > patch2
> > 
> > >+  for (int32_t jdx = aProviders->Length() - 1; jdx >= 0; jdx--) {
> > 
> > there's no reason to iterate backwards here, so please don't.
> 
> local variable saving

that seems ... silly, do you want to do that everywhere?  you should atleast use uint32_t to be consistant with return type of Length()

> > >+    NS_ASSERTION(provider->mContent->IsInDoc(),
> > >+                 "Referred content is not in document!");
> > 
> > why not use MOZ_ASSERT to make it fatal?  It seems like if we hit it its a
> > pretty bad bug.
> 
> assertions are equally bad imo and they shouldn't happen ever.

all of them should never happen, but not all of them likely indicate an exploitable bug.
(In reply to Trevor Saunders (:tbsaunde) from comment #10)
> (In reply to alexander :surkov from comment #9)
> > (In reply to Trevor Saunders (:tbsaunde) from comment #8)
> > > Comment on attachment 718873 [details] [diff] [review]
> > > patch2
> > > 
> > > >+  for (int32_t jdx = aProviders->Length() - 1; jdx >= 0; jdx--) {
> > > 
> > > there's no reason to iterate backwards here, so please don't.
> > 
> > local variable saving
> 
> that seems ... silly,

it doesn't see more silly than yours for (uint32_t index = tail - 1; index < tail; index--) :)

> do you want to do that everywhere?

I don't see a problem when it doesn't make a difference where the array to traverse from

>  you should atleast
> use uint32_t to be consistant with return type of Length()

then jdx >= 0 doesn't really work

> > > >+    NS_ASSERTION(provider->mContent->IsInDoc(),
> > > >+                 "Referred content is not in document!");
> > > 
> > > why not use MOZ_ASSERT to make it fatal?  It seems like if we hit it its a
> > > pretty bad bug.
> > 
> > assertions are equally bad imo and they shouldn't happen ever.
> 
> all of them should never happen, but not all of them likely indicate an
> exploitable bug.

what's exploitable here?
(In reply to alexander :surkov from comment #11)
> (In reply to Trevor Saunders (:tbsaunde) from comment #10)
> > (In reply to alexander :surkov from comment #9)
> > > (In reply to Trevor Saunders (:tbsaunde) from comment #8)
> > > > Comment on attachment 718873 [details] [diff] [review]
> > > > patch2
> > > > 
> > > > >+  for (int32_t jdx = aProviders->Length() - 1; jdx >= 0; jdx--) {
> > > > 
> > > > there's no reason to iterate backwards here, so please don't.
> > > 
> > > local variable saving
> > 
> > that seems ... silly,
> 
> it doesn't see more silly than yours for (uint32_t index = tail - 1; index <
> tail; index--) :)

no, I'm suggesting
uint32_t count = Length();
for (uint32_t i = 0; i < count; i++)

> > do you want to do that everywhere?
> 
> I don't see a problem when it doesn't make a difference where the array to
> traverse from
> 
> >  you should atleast
> > use uint32_t to be consistant with return type of Length()
> 
> then jdx >= 0 doesn't really work

true, but this is all crazy, but I don't really care anymore.

> > > > >+    NS_ASSERTION(provider->mContent->IsInDoc(),
> > > > >+                 "Referred content is not in document!");
> > > > 
> > > > why not use MOZ_ASSERT to make it fatal?  It seems like if we hit it its a
> > > > pretty bad bug.
> > > 
> > > assertions are equally bad imo and they shouldn't happen ever.
> > 
> > all of them should never happen, but not all of them likely indicate an
> > exploitable bug.
> 
> what's exploitable here?

I suspect hiting it means you've found something like bug 768243.  but again whatever I don't care enough to bother.
https://hg.mozilla.org/mozilla-central/rev/09d75eb63721
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla22
You need to log in before you can comment on or make changes to this bug.