1113407 - clean up remaining proxies when documents are destroyed

Status: RESOLVED FIXED

(Core :: Disability Access APIs, defect)

Comment 1

[Trevor Saunders (:tbsaunde)]

Attachment: cleanup wrappers on doc shutdown

Comment 2

[alexander :surkov (:asurkov)]

Comment on attachment 8538875 [details] [diff] [review]
cleanup wrappers on doc shutdown

Review of attachment 8538875 [details] [diff] [review]:
-----------------------------------------------------------------

it'd be good to get David to take a look at ordering

::: accessible/ipc/DocAccessibleParent.h
@@ +104,5 @@
>  
>    uint32_t AddSubtree(ProxyAccessible* aParent,
>                        const nsTArray<AccessibleData>& aNewTree, uint32_t aIdx,
>                        uint32_t aIdxInParent);
> +  static PLDHashOperator ShutdownAccessibles(ProxyEntry* e, void*);

please rename 'e', it's good to specify the second argument is not used, either by name or by comment

btw, cannot it be inline?

::: accessible/ipc/ProxyAccessible.h
@@ +29,5 @@
>    }
> +  ~ProxyAccessible()
> +  {
> +    MOZ_COUNT_DTOR(ProxyAccessible);
> + MOZ_ASSERT(!mWrapper);

indent

Comment 3

[Trevor Saunders (:tbsaunde)]

> ::: accessible/ipc/DocAccessibleParent.h
> @@ +104,5 @@
> >  
> >    uint32_t AddSubtree(ProxyAccessible* aParent,
> >                        const nsTArray<AccessibleData>& aNewTree, uint32_t aIdx,
> >                        uint32_t aIdxInParent);
> > +  static PLDHashOperator ShutdownAccessibles(ProxyEntry* e, void*);
> 
> please rename 'e', it's good to specify the second argument is not used,
> either by name or by comment

how is it not obvious from not having a name?

> btw, cannot it be inline?

its called indirectly so what would be the point?

Comment 4

[alexander :surkov (:asurkov)]

(In reply to Trevor Saunders (:tbsaunde) from comment #3)
> > > +  static PLDHashOperator ShutdownAccessibles(ProxyEntry* e, void*);
> > 
> > please rename 'e', it's good to specify the second argument is not used,
> > either by name or by comment
> 
> how is it not obvious from not having a name?

for me it may look like a bad or inaccurate style, like if somebody didn't care to give it a name

> > btw, cannot it be inline?
> 
> its called indirectly so what would be the point?

well, I don't much about compiler magic that could happen, so if I see something small then I prefer to see it inlined.

Comment 5

[Trevor Saunders (:tbsaunde)]

(In reply to alexander :surkov from comment #4)
> (In reply to Trevor Saunders (:tbsaunde) from comment #3)
> > > > +  static PLDHashOperator ShutdownAccessibles(ProxyEntry* e, void*);
> > > 
> > > please rename 'e', it's good to specify the second argument is not used,
> > > either by name or by comment
> > 
> > how is it not obvious from not having a name?
> 
> for me it may look like a bad or inaccurate style, like if somebody didn't
> care to give it a name

if they don't name it how can they use it? (hint they can't)

> > > btw, cannot it be inline?
> > 
> > its called indirectly so what would be the point?
> 
> well, I don't much about compiler magic that could happen, so if I see
> something small then I prefer to see it inlined.

it won't be inlined, you'd just add a useless keyword.  And if your building with LTO and the compiler manages to do the constant propigation / function cloning to get rid of the indirect call then it should be smart enough to make its own call on the wizdom of inlining.

Comment 6

[alexander :surkov (:asurkov)]

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

> > > how is it not obvious from not having a name?
> > 
> > for me it may look like a bad or inaccurate style, like if somebody didn't
> > care to give it a name
> 
> if they don't name it how can they use it? (hint they can't)

name is not necessary in header

Comment 7

[Trevor Saunders (:tbsaunde)]

(In reply to alexander :surkov from comment #6)
> (In reply to Trevor Saunders (:tbsaunde) from comment #5)
> 
> > > > how is it not obvious from not having a name?
> > > 
> > > for me it may look like a bad or inaccurate style, like if somebody didn't
> > > care to give it a name
> > 
> > if they don't name it how can they use it? (hint they can't)
> 
> name is not necessary in header

and this is a header.

/* unused */ seems rather ugly, and naming something seems dumb because then people can refer to it.

Comment 8

[alexander :surkov (:asurkov)]

(In reply to Trevor Saunders (:tbsaunde) from comment #7)
> (In reply to alexander :surkov from comment #6)
> > (In reply to Trevor Saunders (:tbsaunde) from comment #5)
> > 
> > > > > how is it not obvious from not having a name?
> > > > 
> > > > for me it may look like a bad or inaccurate style, like if somebody didn't
> > > > care to give it a name
> > > 
> > > if they don't name it how can they use it? (hint they can't)
> > 
> > name is not necessary in header
> 
> and this is a header.

exactly

> /* unused */ seems rather ugly, and naming something seems dumb because then
> people can refer to it.

'unused' at least is explicit and you cannot be blamed being inaccurate

Comment 9

[Trevor Saunders (:tbsaunde)]

(In reply to alexander :surkov from comment #8)
> (In reply to Trevor Saunders (:tbsaunde) from comment #7)
> > (In reply to alexander :surkov from comment #6)
> > > (In reply to Trevor Saunders (:tbsaunde) from comment #5)
> > > 
> > > > > > how is it not obvious from not having a name?
> > > > > 
> > > > > for me it may look like a bad or inaccurate style, like if somebody didn't
> > > > > care to give it a name
> > > > 
> > > > if they don't name it how can they use it? (hint they can't)
> > > 
> > > name is not necessary in header
> > 
> > and this is a header.
> 
> exactly

err, it is not a header and it is a definition not a prototype.

> > /* unused */ seems rather ugly, and naming something seems dumb because then
> > people can refer to it.
> 
> 'unused' at least is explicit and you cannot be blamed being inaccurate

you can certainly blaimed for allowing people to do something silly when you don't have to.

Comment 10

[alexander :surkov (:asurkov)]

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

> > exactly
> 
> err, it is not a header and it is a definition not a prototype.

I commented about prototype

> you can certainly blaimed for allowing people to do something silly when you
> don't have to.

it's not the case

Comment 11

[Trevor Saunders (:tbsaunde)]

(In reply to alexander :surkov from comment #10)
> (In reply to Trevor Saunders (:tbsaunde) from comment #9)
> 
> > > exactly
> > 
> > err, it is not a header and it is a definition not a prototype.
> 
> I commented about prototype

er ok, I don't really care about the prototype.

Comment 12

[David Bolter [:davidb] (NeedInfo me for attention)]

Comment on attachment 8538875 [details] [diff] [review]
cleanup wrappers on doc shutdown

Review of attachment 8538875 [details] [diff] [review]:
-----------------------------------------------------------------

OK. Please add the harmless /* unused */ comment to the second arg in the prototype and fix the indent as per surkov. One question inline:

::: accessible/ipc/DocAccessibleParent.cpp
@@ +159,4 @@
>    MOZ_ASSERT(mChildDocs.IsEmpty(),
>        "why wheren't the child docs destroyed already?");
> +
> +  mAccessibles.EnumerateEntries(ShutdownAccessibles, nullptr);

How did you notice this was necessary?

Comment 13

[Trevor Saunders (:tbsaunde)]

> How did you notice this was necessary?

not doing it can cause crashes that I saw locally.

Comment 14

[Trevor Saunders (:tbsaunde)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/eeeba6f2f1e1

Comment 15

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/eeeba6f2f1e1