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

Status: RESOLVED FIXED

(Core :: Disability Access APIs, defect)

Description

[Andrew McCreight [:mccr8]]

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.

Comment 1

[alexander :surkov (:asurkov)]

(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

Comment 2

[alexander :surkov (:asurkov)]

Attachment: patch

just go ahead and traverse them (plus mAnchorContent)

Comment 3

[Trevor Saunders (:tbsaunde)]

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?

Comment 4

[Andrew McCreight [:mccr8]]

> 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.

Comment 5

[alexander :surkov (:asurkov)]

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

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

tmp->mDependentIDsHash.Clear() doesn't work?

Comment 6

[alexander :surkov (:asurkov)]

Attachment: patch2

Comment 7

[Trevor Saunders (:tbsaunde)]

(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 8

[Trevor Saunders (:tbsaunde)]

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.

Comment 9

[alexander :surkov (:asurkov)]

(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.

Comment 10

[Trevor Saunders (:tbsaunde)]

(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.

Comment 11

[alexander :surkov (:asurkov)]

(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?

Comment 12

[Trevor Saunders (:tbsaunde)]

(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.

Comment 13

[alexander :surkov (:asurkov)]

http://hg.mozilla.org/integration/mozilla-inbound/rev/09d75eb63721

Comment 14

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/09d75eb63721